Remove hack in HTMLParserScheduler for deferring to layout while parsing.

Remove hack in HTMLParserScheduler for deferring to layout while parsing.

This code was intending to make sure that layout() timers get
a chance to fire before parser timers do.  However we no longer use
SharedTimer for schedulingLayout.  Instead we use the compositor's
scheduleAnimation timer which is only called when the compositor
actually is going to put up a frame.

I could imagine scenerios whereby a layout was scheduled and either
was called and didn't clear the m_pendingLayout bit due to
the Document being inactive or some other early-return, or a scenerio
in which a layout was scheduled but the compistor never needed to
put up a frame, which would result in the Parser spinning like this
waiting for the layout timer to fire and clear the m_pendingLayout
bool, but it never happened.

As the FIXME said, the real fix here is to reduce the max duration
of the parser and possibly coordinate with the compositor to allow
the parser to do work directly after the frame has been put up, in
the time when the main-thread is otherwise idle waiting for the
compositor and gpu threads to actually put up the frame.

This may cause a regression on SpeedIndex benchmarks and we may
need to find a different way to implement what this code
was trying to do.

I was not able to produce a test which triggered this bug, but I'm
confident it's possible to get Blink into such a confused state.

BUG=288265
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170472

[abarth-chromium, 2014-03-29 20:20:34]

lgtm

https://codereview.chromium.org/218373002/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLParserScheduler.cpp (left):

https://codereview.chromium.org/218373002/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLParserScheduler.cpp:95: if
(m_parser->document()->shouldParserYieldAgressivelyBeforeScriptExecution()) {
Can we delete shouldParserYieldAgressivelyBeforeScriptExecution?

[eseidel, 2014-03-29 23:52:45]

On 2014/03/29 20:20:34, abarth wrote:
> lgtm
> 
>
https://codereview.chromium.org/218373002/diff/1/Source/core/html/parser/HTML...
> File Source/core/html/parser/HTMLParserScheduler.cpp (left):
> 
>
https://codereview.chromium.org/218373002/diff/1/Source/core/html/parser/HTML...
> Source/core/html/parser/HTMLParserScheduler.cpp:95: if
> (m_parser->document()->shouldParserYieldAgressivelyBeforeScriptExecution()) {
> Can we delete shouldParserYieldAgressivelyBeforeScriptExecution?

I looked into that.  It's used in one other place (where it actually is about to
execute scripts).  I think that would change more behavior and we might want to
keep that for now.

I think the full-on solution to this problem is to better integrate scheduling
of work like this with the compositor and higher layers of chromium.  We should
be able to chose what work we want to do.  We have a zillion timers in Blink but
most of them are never used.  The few important ones should be driven more
smartly.

[tonyg, 2014-03-31 14:49:10]

lgtm2

I think the parser has some unnecessary defense like this against slow tree
building. That is probably safe to remove now as the actual tokenization and
tree building shouldn't take a perceptible amount of time.

OTOH, I think we need to maintain code that makes sure we update layout before
running app's JS. That can prevent new layouts for an arbitrarily long time.

We should be a bit careful in this area as this stuff is subtle and can have
100's of ms or seconds of impact on speed index (for which we currently have no
automated coverage on the bots or UMAs).

[eseidel, 2014-03-31 17:15:10]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-31 17:15:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/218373002/1

[commit-bot: I haz the power, 2014-03-31 18:39:01]

Change committed as 170472