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

Eliminate paint timing monkey patching #3923

Merged
merged 12 commits into from
Aug 16, 2018
Merged

Eliminate paint timing monkey patching #3923

merged 12 commits into from
Aug 16, 2018

Conversation

tdresser
Copy link
Contributor

@tdresser tdresser commented Aug 15, 2018

This eliminates paint timing monkey patching, and fixes some minor issues with when we mark paint timing.

This is necessary for us to correctly report event duration for the event timing spec. Issue here.

@domenic - you took a look at this here.


/acknowledgements.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )

@tdresser
Copy link
Contributor Author

Hmmm, ./build.sh passes fine. Any idea why Travis would be failing when my local build doesn't?

@domenic
Copy link
Member

domenic commented Aug 15, 2018

If you look at the Travis logs, you'll see that the CI does more-expensive HTML conformance checking which isn't done at build time. In particular:

"file:/whatwg/output/commit-snapshots/ff1957c7c9e9d6ceb99ef2afa56cc43276b61379/index.html":62695.263-62695.265: error: Element “p” not allowed as child of element “ol” in this context. (Suppressing further errors from this subtree.)
"file:/whatwg/output/commit-snapshots/ff1957c7c9e9d6ceb99ef2afa56cc43276b61379/index.html":62699.27-62699.40: error: Element “p” not allowed as child of element “ol” in this context. (Suppressing further errors from this subtree.)

@tdresser
Copy link
Contributor Author

Thanks, fixed the issue.

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.

@tdresser, this is quite nice; I'm really happy with what we came up with here.

I pushed a few separate fixup commits, separated for easier review. Note the source formatting one for future reference, and check me that "present" -> "presentable" was correct.

There's a separate issue with paint timing where you're passing "document" to the "mark paint timing" algorithm, but that spec is not very clear about how things are scoped (document? browsing context? global?). I'll file that issue over there, although I'm pretty sure it won't impact this. So I'm OK merging this as soon as you confirm my changes.

Copy link
Contributor Author

@tdresser tdresser left a comment

Choose a reason for hiding this comment

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

LGTM % nit.

source Outdated
@@ -89002,7 +89002,7 @@ dictionary <dfn>PromiseRejectionEventInit</dfn> : <span>EventInit</span> {
<p><i>Rendering opportunites</i>: If there are <span data-x="browsing context">browsing
contexts</span> <var>B</var> that do not have a <span>rendering opportunity</span>, then
remove from <var>docs</var> all <code>Document</code> objects whose <span
data-x="concept-document-bc">browsing context</span> is in <var>B</var>.</p>
data-x="concept-document-bc">browsing context</span> is <var>B</var>.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this context, B is a list of browsing contexts, so I think this was correct previously?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, great catch. I'll rename to "browsingContexts" for clarity. (There is also a "B" up in step 7.2, which technically doesn't collide.)

@tdresser
Copy link
Contributor Author

Yeah, we'll need to clear up paint timing once this has landed.
Other than that one nit, this looks fine to merge.

Thanks for fixing the indenting, I'll be more intentional about making sure it's sane next time.

@domenic domenic merged commit 92b90bd into whatwg:master Aug 16, 2018
npm1 added a commit to w3c/paint-timing that referenced this pull request Oct 3, 2018
This PR removes the "Modifications to other specifications" section because whatwg/html#3923 has already landed, so the HTML spec change does not need to be described here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants