Fixed the paint supression to track only render-blocking stylesheets

Fixed the paint supression to track only render-blocking stylesheets

BUG=707747

[Pat Meenan, 2017-04-17 15:12:22]

The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 15:12:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Pat Meenan, 2017-04-17 15:12:55]

Description was changed from

==========
Fixed the paint supression to track only render-blocking stylesheets

BUG=707747
==========

to

==========
Fixed the paint supression to track only render-blocking stylesheets

BUG=707747
==========

[Pat Meenan, 2017-04-17 15:12:55]

pmeenan@chromium.org changed reviewers:
+ esprehn@chromium.org

[commit-bot: I haz the power, 2017-04-17 16:45:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 16:45:28]

Dry run: This issue passed the CQ dry run.

[Pat Meenan, 2017-04-17 17:07:01]

esprehn@ could you PTAL?  This should fix the cases where paints were still
being (incorrectly) suppressed.

[esprehn, 2017-04-17 20:24:23]

Can you write a SimTest?

[esprehn, 2017-04-28 21:42:52]

What's the status here? :)

[Pat Meenan, 2017-05-08 17:05:18]

The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 17:05:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Pat Meenan, 2017-05-08 17:07:34]

Sorry about the delay.  Added a sim test to the low-level document logic to make
sure it still flags layouts triggered with pending stylesheets from the head but
does not flag if the pending stylesheet is from the body.

Let me know if you also want a sim test that checks the higher-level paint
suppression logic.

[commit-bot: I haz the power, 2017-05-08 19:51:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 19:51:23]

Dry run: This issue passed the CQ dry run.

[Pat Meenan, 2017-05-11 15:12:47]

pmeenan@chromium.org changed reviewers:
+ rune@opera.com

[Pat Meenan, 2017-05-11 15:12:48]

rune@, could you PTAL?

This fixes a case where the paint suppression logic incorrectly blocks painting
when layout is triggered while waiting for in-body stylesheets (when the
experiment flag is enabled to not block paint in those cases)

[rune, 2017-05-11 19:40:53]

lgtm

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2293:
!GetStyleEngine().HasPendingRenderBlockingSheets()) {
Not sure if I fully wrapped my head around the combination of
HasPendingRenderBlockingSheets and IsRenderingReady() which also rely on the
in-body runtime flag, but this only makes a difference with the flag enabled
which is currently not the case, right?

[Pat Meenan, 2017-05-11 20:25:29]

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2293:
!GetStyleEngine().HasPendingRenderBlockingSheets()) {
On 2017/05/11 19:40:53, rune wrote:
> Not sure if I fully wrapped my head around the combination of
> HasPendingRenderBlockingSheets and IsRenderingReady() which also rely on the
> in-body runtime flag, but this only makes a difference with the flag enabled
> which is currently not the case, right?

Good point.  The main issue with IsRenderingReady (and the underlying call to
HaveRenderStylesheetsLoaded()) is that they both have escape hatches for
ignoring pending stylesheets which presumably this is explicitly trying to
bypass (and it also includes a check for imports).

I can create a separate function as part of document
(HasPendingRenderBlockingSheets) that switches based on the flag and checks the
underlying style engine flag without the ability to ignore (though the naming is
still a bit confusing).  Would it make more sense to add a flag to the existing
HaveRenderBlockingStylesheetsLoaded and the underlying style engine functions
that lets the caller explicitly disable the ignore_pending_stylesheets check?

[Pat Meenan, 2017-05-11 20:34:28]

nm, found a cleaner way.  

Adding a HasPendingRenderBlockingSheets() method to the document that calls the
appropriate underlying style engine function of the same name based on the state
of the flag.  Building and testing it out now and a patch will be up shortly.

[Pat Meenan, 2017-05-11 20:49:03]

OK, changed so it will work correctly for both states of the flag.  The flag is
enabled for tests which is why the sim test succeeded.  Let me know if you want
me to create a sim test that explicitly checks both modes.

[rune, 2017-05-11 22:06:10]

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2293:
!GetStyleEngine().HasPendingRenderBlockingSheets()) {
On 2017/05/11 20:25:29, Pat Meenan wrote:
> On 2017/05/11 19:40:53, rune wrote:
> > Not sure if I fully wrapped my head around the combination of
> > HasPendingRenderBlockingSheets and IsRenderingReady() which also rely on the
> > in-body runtime flag, but this only makes a difference with the flag enabled
> > which is currently not the case, right?
> 
> Good point.  The main issue with IsRenderingReady (and the underlying call to
> HaveRenderStylesheetsLoaded()) is that they both have escape hatches for
> ignoring pending stylesheets which presumably this is explicitly trying to
> bypass (and it also includes a check for imports).

Thanks for the clarification. Don't you also need to check imports?

If I read the patches correctly - if you have ignore_pending set, render
blocking sheets loaded, but imports not finished, you will MarkFirstLayout. But
loading imports will block rendering, right?

> I can create a separate function as part of document
> (HasPendingRenderBlockingSheets) that switches based on the flag and checks
the
> underlying style engine flag without the ability to ignore (though the naming
is
> still a bit confusing).  Would it make more sense to add a flag to the
existing
> HaveRenderBlockingStylesheetsLoaded and the underlying style engine functions
> that lets the caller explicitly disable the ignore_pending_stylesheets check?

Looking at the existing code, there seems to be some inconsistency, and possibly
a bug there. IsRenderingReady() checks the StyleEngine directly for
ignoring_pending before the calls into the StyleEngine, making the
ignore_pending checks in StyleEngine::Have*Loaded dud.

I'm wondering if getting rid of the ignore_pending check in
StyleEngine::Have*Loaded could be an option?
I suspect (hope) none of the other calls to
HaveScriptBlockingStylesheetsLoaded() than the one in IsRenderingReady() are
called in an ignore_pending context.

[rune, 2017-05-11 22:13:31]

On 2017/05/11 22:06:10, rune wrote:
> Looking at the existing code, there seems to be some inconsistency, and
possibly
> a bug there.

I didn't quite finish my reasoning there. What I thought about was:

  bool IsScriptExecutionReady() const {
    return HaveImportsLoaded() && HaveScriptBlockingStylesheetsLoaded();
  }

Where HaveImportsLoaded() does not take into account ignore_pending while
HaveScriptBlockingStylesheetsLoaded() does. But ignore_pending is used for style
update, and I really hope we don't execute scripts from style update.

[Pat Meenan, 2017-05-12 13:24:08]

I'll do some more digging to see if I can actually figure out how the whole
"ignore pending" bits work.  I'm not very familiar with the paint suppression
and forced layout code and I'm starting to understand why everyone wants to see
it gone.

Maybe now would be a good time to discuss how we could completely eliminate it
instead of just skirting around it like I am for the in-body stylesheets change.

If I understand the general concept well enough, could we just:

1 - Pause the parser for in-body imports like we currently do for stylesheets
(well, with the flag flipped anyway)

2 - When inserting the body into the document, pause the parser (and body insert
itself) until any pending script-blocking imports or stylesheets have finished
loading

That would let us keep the parallel loading of imports defined in the head (and
let the parser go through any stylesheets in the head in parallel) and eliminate
the need to do fake layouts because it would be impossible to have anything to
layout or render and still have pending sheets.

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2823873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2293:
!GetStyleEngine().HasPendingRenderBlockingSheets()) {
On 2017/05/11 22:06:10, rune wrote:
> On 2017/05/11 20:25:29, Pat Meenan wrote:
> > On 2017/05/11 19:40:53, rune wrote:
> > > Not sure if I fully wrapped my head around the combination of
> > > HasPendingRenderBlockingSheets and IsRenderingReady() which also rely on
the
> > > in-body runtime flag, but this only makes a difference with the flag
enabled
> > > which is currently not the case, right?
> > 
> > Good point.  The main issue with IsRenderingReady (and the underlying call
to
> > HaveRenderStylesheetsLoaded()) is that they both have escape hatches for
> > ignoring pending stylesheets which presumably this is explicitly trying to
> > bypass (and it also includes a check for imports).
> 
> Thanks for the clarification. Don't you also need to check imports?
> 
> If I read the patches correctly - if you have ignore_pending set, render
> blocking sheets loaded, but imports not finished, you will MarkFirstLayout.
But
> loading imports will block rendering, right?
> 
> > I can create a separate function as part of document
> > (HasPendingRenderBlockingSheets) that switches based on the flag and checks
> the
> > underlying style engine flag without the ability to ignore (though the
naming
> is
> > still a bit confusing).  Would it make more sense to add a flag to the
> existing
> > HaveRenderBlockingStylesheetsLoaded and the underlying style engine
functions
> > that lets the caller explicitly disable the ignore_pending_stylesheets
check?
> 
> Looking at the existing code, there seems to be some inconsistency, and
possibly
> a bug there. IsRenderingReady() checks the StyleEngine directly for
> ignoring_pending before the calls into the StyleEngine, making the
> ignore_pending checks in StyleEngine::Have*Loaded dud.
> 
> I'm wondering if getting rid of the ignore_pending check in
> StyleEngine::Have*Loaded could be an option?
> I suspect (hope) none of the other calls to
> HaveScriptBlockingStylesheetsLoaded() than the one in IsRenderingReady() are
> called in an ignore_pending context.

I was largely trying to carry-over the existing behavior and just make it handle
the in-body stylesheet case correctly, but yes, now that you mention it, it is
probably buggy with respect to imports.

[rune, 2017-05-12 13:41:40]

On 2017/05/12 13:24:08, Pat Meenan wrote:

> I was largely trying to carry-over the existing behavior and just make it
handle
> the in-body stylesheet case correctly, but yes, now that you mention it, it is
> probably buggy with respect to imports.

OK. Just land PS3 and we can iterate on this.