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

Make fragment loop not depend on spin the event loop #4440

Merged
merged 4 commits into from
Jun 28, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Mar 22, 2019

This only seems worth doing if we can address all of #3387 I think, as it's quite a bit more involved wording-wise. (It does seem a little weird that the current setup gives both the networking task source. I would have expected the DOM manipulation task source or some such for finding the fragment.)


/browsing-the-web.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would be in favor of merging this ASAP as I think it's a significant clarity improvement just by being more explicit, e.g. using clearer looping constructs.

source Show resolved Hide resolved
source Outdated
</ol>

<p>The <span>task source</span> for this <span data-x="concept-task">task</span> is the
<p>The <span>task source</span> for these <span data-x="concept-task">tasks</span> is the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would rather move us toward a style of stating the task source in the same line as the "queue a task" (i.e. "queue a task on the networking task source to..."). But I can understand if you want to do that as a larger sweeping change instead of one by one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer making "queue a task" more explicit about its arguments first.

source Show resolved Hide resolved
@annevk annevk marked this pull request as ready for review March 29, 2019 11:05
@domenic
Copy link
Member

domenic commented May 24, 2019

@annevk shall we merge this?

@annevk
Copy link
Member Author

annevk commented May 27, 2019

Reading this again it seems like the new version doesn't protect against multiple tasks being queued that might potentially do the same thing. What is desired seems to be that at the end of the task a new task is queued to the "in parallel" thread to run the steps again. We could perhaps emulate this with a second taskRun variable or some such upon which the "in parallel" thread waits for a change.

@domenic
Copy link
Member

domenic commented May 29, 2019

Reading this again it seems like the new version doesn't protect against multiple tasks being queued that might potentially do the same thing.

You mean, because navigate happened twice? I think that's what the "If the Document object has no parser..." business prevents. Either way, I don't really see what changed that would make things different before this PR versus after.

Let's take a step back. The intention of this section seems to be basically to, at some later time, queue a task to scroll to the fragment, if doing so still makes sense. If that doesn't work, try again later.

Here's my attempt at reframing the spec-code to match that more Englishey description.

  1. The navigation algorithm has now matured.

  2. Try to scroll to the fragment for the Document.

To try to scroll to the fragment for Document document,

  1. In parallel,

    1. Wait a user-agent-defined amount of time.

    2. Queue a task on the neworking task source to run these steps:

      1. If document has no parser, or its parser has stopped parsing, or the user agent has reason to believe the user is no longer interested in scrolling to the fragment, then abort these steps.

      2. Otherwise, scroll to the fragment given in document's URL. If this does not find an indicated part of the document, then try to scroll to the fragment for document.

How does that sound?

@annevk
Copy link
Member Author

annevk commented Jun 7, 2019

I meant because of the "while true". I think your rephrasing would work.

@domenic
Copy link
Member

domenic commented Jun 13, 2019

@annevk did another take based on my above comment; let me know what you think.

@annevk annevk merged commit e62a7be into master Jun 28, 2019
@annevk annevk deleted the annevk/modern-fragment-loop branch June 28, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants