Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure Refresh pragma always uses a URL record #2865

Merged
merged 5 commits into from Aug 2, 2017

Conversation

2 participants
@annevk
Copy link
Member

commented Jul 24, 2017

Before this change you could sometimes end up passing a string to navigate. This also reduces the number of jumps and confines the remaining to a single block.

Ensure Refresh pragma always uses a URL record
Before this change you could sometimes end up passing a string to navigate. This also reduces the number of jumps and confines the remaining to a single block.
@domenic
Copy link
Member

left a comment

A couple minor things.

source Outdated
<li><p>If another <code>meta</code> element with an <code
data-x="attr-meta-http-equiv">http-equiv</code> attribute in the <span
data-x="attr-meta-http-equiv-refresh">Refresh state</span> has already been successfully
processed (i.e. when it was inserted the user agent processed it and reached the step labeled
<i>end</i>), then return.</p></li>
processed (i.e., when it was inserted the user agent processed it), then return.</p></li>

This comment has been minimized.

Copy link
@domenic

domenic Jul 26, 2017

Member

I think this reduces clarity a bit. Labeling steps for the purpose of later referencing them is OK, IMO; it's just jumping to them that we want to move away from.

In particular, this seems to change the behavior if any early returns are encountered.

This comment has been minimized.

Copy link
@annevk

annevk Jul 27, 2017

Author Member

How can you have an early return though? The algorithm is fully synchronous. Also, we'll have to remove this once we add support for the Refresh header. This will be rewritten.

This comment has been minimized.

Copy link
@domenic

domenic Jul 28, 2017

Member

Many steps in the algorithm return early, without reaching the "end" step. For example, line 14296, line 14340, etc. We don't want to bail on a second iteration of the algorithm if we had an early return at one of those steps in a previous iteration of the algorithm.

This could be clearer by storing a boolean somewhere explicitly, I suppose. Then only the step labeled "end" would set the boolean to true, but those other returns would not.

This comment has been minimized.

Copy link
@annevk

annevk Jul 29, 2017

Author Member

Oh, I didn't realize those would end up ignored. I guess I should look at the tests. And yeah, maybe I should just introduce the boolean here. We'll need it for the header for sure.

This comment has been minimized.

Copy link
@annevk

annevk Jul 31, 2017

Author Member

Done.

source Outdated
<li><p>Set <var>url</var> to the substring of <var>input</var> from the <span>code point</span>
at <var>position</var> to the end of the string.</p></li>
<ol>
<li><p>Let <var>url</var> to the substring of <var>input</var> from the

This comment has been minimized.

Copy link
@domenic

domenic Jul 26, 2017

Member

s/to/be

This comment has been minimized.

Copy link
@domenic

domenic Jul 26, 2017

Member

Maybe "urlString" or "providedURLString" would be better

This comment has been minimized.

Copy link
@domenic

domenic Aug 1, 2017

Member

Missed "s/to/be", let me push a fix.

annevk and others added some commits Jul 31, 2017

@domenic

domenic approved these changes Aug 1, 2017

Copy link
Member

left a comment

LGTM with some questions

source Outdated
<li><p><i>Parse</i>: <span data-x="parse a url">Parse</span> <var>url</var> relative to
<var>document</var>. If that fails, return. Otherwise, let <var>urlRecord</var> be the
<span>resulting URL record</span>.</p></li>
<li><p>Set <var>document</var>'s <span>refresh flag</span>.</p></li>

This comment has been minimized.

Copy link
@domenic

domenic Aug 1, 2017

Member

Now that this is stated more clearly, I am not sure it is accurate. For example, what if the user cancels the redirect? Then we are setting the flag forever. Ah well, it's probably fine...

This comment has been minimized.

Copy link
@annevk

annevk Aug 2, 2017

Author Member

It seems okay to prevent spamming the user with further questions. The only thing the UA could reasonably offer here I think is to follow the original one anyway.

This comment has been minimized.

Copy link
@domenic

domenic Aug 2, 2017

Member

Well, what if a new refresh meta is inserted into the document?

source Outdated
@@ -14282,24 +14282,22 @@ people expect to have work and what is necessary.

<p>This pragma acts as timed redirect.</p>

<p>A <code>Document</code> object has an associated <dfn data-dfn-for="Document">refresh
flag</dfn>. It is unset unless stated otherwise.</p>

This comment has been minimized.

Copy link
@domenic

domenic Aug 1, 2017

Member

I know I keep asking this repeatedly, so sorry for forgetting... but we are sticking with flags for now, instead of booleans? With the idea being that one day we do an update all at once?

@annevk

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

To answer boolean <> flag. I suggest we go with booleans wherever we can and it doesn't make the existing construct too ugly. And converting flags to booleans should also be fair game. We'll have to watch out with exported flags though. Those will require some effort.

source Outdated
@@ -14282,24 +14282,22 @@ people expect to have work and what is necessary.

<p>This pragma acts as timed redirect.</p>

<p>A <code>Document</code> object has an associated <dfn data-dfn-for="Document">will
refresh</dfn> (a boolean). It is false unless stated otherwise.</p>

This comment has been minimized.

Copy link
@domenic

domenic Aug 2, 2017

Member

Given the specialized usage, this seems like not a great name, i.e. it doesn't get set if document.reload() is called. Maybe "will meta-refresh" or similar. Somehow this didn't feel as problematic with the flag.

Also maybe "It is initially false."

This comment has been minimized.

Copy link
@annevk

annevk Aug 2, 2017

Author Member

meta-refresh feels wrong because of the HTTP header. If you want more obscure, "will declaratively refresh"?

This comment has been minimized.

Copy link
@domenic

domenic Aug 2, 2017

Member

Good point. Yeah, I think "will declaratively refresh" works.

Feel free to merge with that change.

@annevk annevk merged commit 64ea795 into master Aug 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@annevk annevk deleted the annevk/refresh-pragma-bug branch Aug 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.