Careful, UrlReplacements isn't a generic variable substitution library. I added an assert that the "protocol" of the url remains the same (because they were all URLs before now), but that doesn't apply here. So if the default value had something like RANDOM:X:Y, this would break because of the changing "protocol".

We should split UrlReplacements into a core class (VarSubstitutions?), and leave the URL specific code out of it.

Yeah that makes sense.

I can get started in moving VarSubstitutions to a separate service. My guess is for any call to that service directly (outside of url-replacement-impl.js) shouldn't allow URL replacements then?

My guess is for any call to that service directly (outside of url-replacement-impl.js) shouldn't allow URL replacements then?

Hm?

I meant to say that we'd need a way to prevent direct calls to var-sub service with a URL to avoid circumventing the protocol assert.

We can presubmit and whitelist Forms and UrlReplacements.

Sent a PR #6257

Done.

Timer#timeoutPromise takes a second "race" promise.

That one rejects though when timeout happens, I guess I could call the same callback in both then and catch clauses, or is there a better way to do this?

Ahh, this is fine then.

Ack

This changes the method signature from void return. Consider returning null in other cases and updating return type to ?Promise.

Dropped this return since we don't use it.

Nit: This would be more readable if xhrUrl was declared closer to its usage and the isHeadOrGet conditional wasn't duped below for body. E.g.

let xhrUrl, body;
if (isHeadOrGet) {
  xhrUrl = addParamsToUrl(this.xhrAction_, this.getFormAsObject_());
  body = new FormData(this.form_);
} else {
  xhrUrl = this.xhrAction_;
}
return this.xhr_.fetchJson(xhrUrl, {
  body: body,

Done

Typo?

Dropped

Flip please.

Done