Eliminate Paint Timing monkey patching #1

tdresser · 2018-07-05T17:42:54Z

No description provided.

tdresser · 2018-07-05T17:49:06Z

source

+      <p>A <span>browsing context</span> has a rendering opportunity if the user agent is currently
+      able to present the contents of the <span>browsing context</span> to the user, accounting for
+      hardware refresh rate constraints and user agent throttling for performance reasons, but
+      considering content presented even if it's outside the viewport.</p>


This is messy, but I haven't come up with anything any better. Any thoughts?

I'm generally quite impressed by how this is all phrased. I don't find this messy.

However, I'm unsure of the practical implications of "considering content presented even if it's outside the viewport". I guess it's saying that iframes are supposed to "have rendering opportunities" even if they're below the fold?

It seems like by this phrasing it'd be OK to throttle offscreen iframes down to 0 FPS "for performance reasons", but would not be OK to consider them to "not have a rendering opportunity" just because they're offscreen. Which is a bit weird, but maybe that's just because limiting behaviors are often weird.

Maybe it'd help to explicitly connect this to the below step, which allows removing for a wider variety of reasons. E.g. explicitly stating that this step just establishes "rendering opportunities", and the below step is where the browser decides whether to take advantage of the opportunity or not. Or maybe explicitly mentioning the term "frame rate".

I've been explicitly avoiding "frame rate", as I'm worried we'd get pushback on the multi-display, or variable framerate cases (i.e. gsync)

I've tried to clarify this. I also realized I was incorrectly handling the case where we coalesce tasks. We shouldn't mark paint timing in that case, but are doing so in this spec.

tdresser · 2018-07-05T17:49:51Z

source

     </li>

     <li>
-      <p>If there are a <span data-x="nested browsing context">nested browsing contexts</span>


I'm not sure why we previously handled nested and top level browsing contexts independently. This seems clearer to me.

It may have been incrementally added, and nobody thought to merge them.

tdresser · 2018-07-05T19:03:50Z

@domenic, can you take look at this? There are certainly rough bits, but I think this is approximately correct, based on what we discussed for event timing.

tdresser · 2018-07-06T17:23:27Z

source

-       callbacks</span> interleaved). For example, a user agent might wish to coalesce timer
-       callbacks together, with no intermediate rendering updates.</p>
-      </div>
+      <p>If there are a <span data-x="browsing context">browsing contexts</span> <var>B</var> that


Nit: "a browsing contexts" -> "browsing contexts"

domenic

Overall this looks quite solid. I think the more we can do to make clear the differences between the two "remove the document" steps, the better, but it's pretty reasonable already.

domenic · 2018-07-10T22:13:44Z

source

+      <p>A <span>browsing context</span> has a rendering opportunity if the user agent is currently
+      able to present the contents of the <span>browsing context</span> to the user, accounting for
+      hardware refresh rate constraints and user agent throttling for performance reasons, but
+      considering content presented even if it's outside the viewport.</p>


I'm generally quite impressed by how this is all phrased. I don't find this messy.

However, I'm unsure of the practical implications of "considering content presented even if it's outside the viewport". I guess it's saying that iframes are supposed to "have rendering opportunities" even if they're below the fold?

It seems like by this phrasing it'd be OK to throttle offscreen iframes down to 0 FPS "for performance reasons", but would not be OK to consider them to "not have a rendering opportunity" just because they're offscreen. Which is a bit weird, but maybe that's just because limiting behaviors are often weird.

Maybe it'd help to explicitly connect this to the below step, which allows removing for a wider variety of reasons. E.g. explicitly stating that this step just establishes "rendering opportunities", and the below step is where the browser decides whether to take advantage of the opportunity or not. Or maybe explicitly mentioning the term "frame rate".

domenic · 2018-07-10T22:14:14Z

source

     </li>

     <li>
-      <p>If there are a <span data-x="nested browsing context">nested browsing contexts</span>
+      <p>Invoke the <span>Mark Paint Timing</span> algorithm for each <code>Document</code> object


Nit: lowercase algorithm names, usually.

domenic · 2018-07-10T22:14:43Z

source

     </li>

     <li>
-      <p>If there are a <span data-x="nested browsing context">nested browsing contexts</span>


It may have been incrementally added, and nobody thought to merge them.

domenic · 2018-07-10T22:19:25Z

source

+      <code>Document</code> objects whose <span data-x="concept-document-bc">browsing context</span>
+      is in <var>B</var>.</p>
+
+      <p>A <span>browsing context</span> has a rendering opportunity if the user agent is currently


I think you'll want to <dfn> "rendering opportunity", and then cross-reference it where it's mentioned. It seems like a pretty useful concept.

https://github.com/whatwg/wattsi/blob/master/Syntax.md if you haven't seen it yet.

I was planning on doing this, but then couldn't figure out where to stick the definition. Any thoughts on where it should go?

This sentence seems like a good place!

tdresser · 2018-07-12T19:28:42Z

source

     </li>

     <li>
-      <p>If there are a <span data-x="nested browsing context">nested browsing contexts</span>
+      <p>Invoke the <span>Mark Paint Timing</span> algorithm for each <code>Document</code> object


tdresser · 2018-07-12T19:29:43Z

source

+      <p>A <span>browsing context</span> has a rendering opportunity if the user agent is currently
+      able to present the contents of the <span>browsing context</span> to the user, accounting for
+      hardware refresh rate constraints and user agent throttling for performance reasons, but
+      considering content presented even if it's outside the viewport.</p>


I've been explicitly avoiding "frame rate", as I'm worried we'd get pushback on the multi-display, or variable framerate cases (i.e. gsync)

tdresser · 2018-07-12T19:29:57Z

source

-       callbacks</span> interleaved). For example, a user agent might wish to coalesce timer
-       callbacks together, with no intermediate rendering updates.</p>
-      </div>
+      <p>If there are a <span data-x="browsing context">browsing contexts</span> <var>B</var> that


tdresser · 2018-07-13T14:47:47Z

source

+      <code>Document</code> objects whose <span data-x="concept-document-bc">browsing context</span>
+      is in <var>B</var>.</p>
+
+      <p>A <span>browsing context</span> has a rendering opportunity if the user agent is currently


I was planning on doing this, but then couldn't figure out where to stick the definition. Any thoughts on where it should go?

tdresser · 2018-07-13T18:37:45Z

source

+      <p>A <span>browsing context</span> has a rendering opportunity if the user agent is currently
+      able to present the contents of the <span>browsing context</span> to the user, accounting for
+      hardware refresh rate constraints and user agent throttling for performance reasons, but
+      considering content presented even if it's outside the viewport.</p>


I've tried to clarify this. I also realized I was incorrectly handling the case where we coalesce tasks. We shouldn't mark paint timing in that case, but are doing so in this spec.

tdresser · 2018-07-13T18:41:06Z

source

+     <code>Document</code> object removed.</p></li>
+
+     <li><p>If there are <span data-x="browsing context">browsing contexts</span> <var>B</var> for
+     which the user agent believes it's preferrable to skip updating the rendering for other


I'm not sure how specific to be here.

I suspect that for all cases where there is a rendering opportunity and new things to render, but we choose not to rendering anyways, we don't want to mark paint timing, but the only concrete example I've got of when this could happen is for task coalescing.

The key thing we want to avoid is the presence or lack of content to draw impacting whether or not paint timing is marked, which I believe this accomplishes.

domenic

LGTM with small suggestions. Some source formatting stuff too but I can clean that up when merging instead of doing back-and-forth.

domenic · 2018-07-13T18:44:50Z

source

-      constrained. In such cases, the browser could decide to update the rendering for such content
-      infrequently or never.</p>
-     </li>
+     <li id="step-7-3-rendering-opportunities">


Don't bake step numbers into IDs or references; they'll change over time. You could instead use labels; search for "step labeled" in the spec to see some examples.

domenic · 2018-07-13T18:46:17Z

source

+     visible, the user agent might decide to drop that page to a much slower 4 rendering
+     opportunities per second, or even less.</p>
+
+     <p>If there are <span data-x="browsing context">browsing contexts</span> <var>B</var> that do


I think actually just putting this paragraph first would be a bit nicer. Say what you're doing, then explain it.

domenic · 2018-07-13T18:48:25Z

source

+     (there's no <span>rendering opportunity</span>).</p>
+
+     <p><a href="#step-7-4-unnecessary-rendering">Step 7.4</a> prevents the user agent from updating
+     the renderingwhen there's no new content to draw.</p>


Missing space

tdresser

Sorry for the massive delay.

I've addressed your feedback, and will open a PR to merge these changes to the official HTML spec.

tdresser · 2018-08-15T14:43:43Z

source

+     visible, the user agent might decide to drop that page to a much slower 4 rendering
+     opportunities per second, or even less.</p>
+
+     <p>If there are <span data-x="browsing context">browsing contexts</span> <var>B</var> that do


tdresser · 2018-08-15T14:44:03Z

source

+     (there's no <span>rendering opportunity</span>).</p>
+
+     <p><a href="#step-7-4-unnecessary-rendering">Step 7.4</a> prevents the user agent from updating
+     the renderingwhen there's no new content to draw.</p>


tdresser · 2018-08-15T14:46:12Z

source

-      constrained. In such cases, the browser could decide to update the rendering for such content
-      infrequently or never.</p>
-     </li>
+     <li id="step-7-3-rendering-opportunities">


Eliminate Paint Timing monkey patching

ef50cec

tdresser commented Jul 5, 2018

View reviewed changes

tdresser commented Jul 6, 2018

View reviewed changes

domenic reviewed Jul 10, 2018

View reviewed changes

tdresser added 3 commits July 13, 2018 11:33

WIP: improve notes on reasons to bail

580e2fc

Time to sort out coalesced tasks

80313b1

Handle task coalescing

6e5475c

tdresser commented Jul 13, 2018

View reviewed changes

domenic approved these changes Jul 13, 2018

View reviewed changes

Address feedback

cec851e

tdresser commented Aug 15, 2018

View reviewed changes

tdresser merged commit f0df7a0 into master Aug 15, 2018

tdresser mentioned this pull request Aug 15, 2018

Eliminate paint timing monkey patching whatwg/html#3923

Merged

Eliminate Paint Timing monkey patching #1

Eliminate Paint Timing monkey patching #1

Uh oh!

Conversation

tdresser commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdresser commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdresser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!