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

Processing model re-write based on MutationObserver. #47

Merged
merged 8 commits into from
Oct 9, 2015
Merged

Conversation

mpb
Copy link
Collaborator

@mpb mpb commented Oct 7, 2015

Fixes #35
Fixes #43

@mpb
Copy link
Collaborator Author

mpb commented Oct 7, 2015

@domenic, PTAL and see if this addresses your concerns with the processing model.
(It's kind of a large change, so I may have missed something.)
Thanks!

text: DOMHighResTimeStamp; type: typedef; urlPrefix: http://www.w3.org/TR/hr-time/
text: margin; type: attribute; url: http://www.w3.org/TR/CSS21/box.html#propdef-margin
text: length; type: attribute; url: http://www.w3.org/TR/css3-values/#lengths
urlPrefix: http://www.w3.org/TR/hr-time/; type: typedef; text: DOMHighResTimeStamp
Copy link

Choose a reason for hiding this comment

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

Drive-by comment: prefer EDs to TRs. Also Web IDL 1 is dead; link to http://heycam.github.io/webidl/ instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, will update the webidl link.
When I look for these specs, the TRs seem to be what turns up. Were there ones in particular you had in mind?

Copy link

Choose a reason for hiding this comment

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

TRs have an unfortunate amount of google-juice, it's true. But they are outdated and should not be used as a basis for implementations. So, everywhere I see /TR/ in this list, it's better to navigate to that URL and click the "editor's draft" link, and use that instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@domenic
Copy link

domenic commented Oct 8, 2015

Looking very good over all. This model is much easier for me to understand at least. I hope it doesn't mismatch the implementation too much. But it definitely clarifies a lot of things. Thanks for taking the time.

callback IntersectionObserverCallback = void (sequence<IntersectionObserverEntry> entries, IntersectionObserver observer)
</pre>
This callback will be invoked when there are changes to <i>target</i>'s intersection with <i>root</i>, as per the <a>processing model</a>.
<h3 id='2-1-intersection-observer-callback'>
Copy link
Member

Choose a reason for hiding this comment

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

Don't put section numbers in the ID. If you ever move/add/delete sections, you'll either have to change the ID (bad) or have the number be wrong (bad).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

list of idle request callbacks</a> with an appropriate
<li>If <i>unit</i>'s <a>IntersectionObserverTaskQueued</a> flag is set
to true, return.</li>
<li>Set <i>unit<\i>'s <a>IntersectionObserverTaskQueued</a> flag to
Copy link

Choose a reason for hiding this comment

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

typo \ vs. /

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

text: length; type: attribute; url: http://www.w3.org/TR/css3-values/#lengths
urlPrefix: http://www.w3.org/TR/hr-time/; type: typedef; text: DOMHighResTimeStamp
url: http://www.w3.org/TR/CSS21/box.html#propdef-margin; type: attribute; text: margin
url: http://www.w3.org/TR/css3-values/#lengths; type: attribute; text: length
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, "length" is not an IDL attribute. It look like you want the <length> production - autolink that with <<length>>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@mpb
Copy link
Collaborator Author

mpb commented Oct 9, 2015

Thanks for the reviews. Anything else outstanding, or does this look good to merge now?

@domenic
Copy link

domenic commented Oct 9, 2015

This looks great! Thanks very much for your attention to detail. This is a really solid spec now from my perspective. (Which, admittedly, is more focused on general spec integration than on the domain-specific issues.)

mpb added a commit that referenced this pull request Oct 9, 2015
Processing model re-write based on MutationObserver.
@mpb mpb merged commit ee23acf into master Oct 9, 2015
@mpb
Copy link
Collaborator Author

mpb commented Oct 9, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants