Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 7 users
Status: Fixed
Owner:
User never visited
Closed: Feb 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
Remove style recalc timer
Project Member Reported by abarth@chromium.org, Jan 23 2014 Back to list
The style recalc timer means that we'll start recalcing style at arbitrary times.  In some of the Silk benchmark cases, the recalc timer causes us to recalc style twice in a frame instead of once.

Rather than use a timer to control style recalculation, we should instead use request animation frame.  I have a sketch of a CL in https://codereview.chromium.org/145133006/, but we're going to land that CL carefully in pieces because this change might have many unexpected implications.
 
Comment 1 by abarth@chromium.org, Jan 23 2014
The first Blink-side step is in https://codereview.chromium.org/140763007/
Project Member Comment 2 by bugdroid1@chromium.org, Jan 24 2014
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=165689

------------------------------------------------------------------------
r165689 | abarth@chromium.org | 2014-01-24T03:23:31.439055Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/layout/common-ancestor-relayout-boundary.html?r1=165689&r2=165688&pathrev=165689
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.cpp?r1=165689&r2=165688&pathrev=165689
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/device-adapt/opera/constrain-010.html?r1=165689&r2=165688&pathrev=165689
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/layout/common-ancestor-relayout-boundary-expected.txt?r1=165689&r2=165688&pathrev=165689
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/device-adapt/opera/constrain-009.html?r1=165689&r2=165688&pathrev=165689

Begin removing the style recalc timer

This CL starts us down the path of removing the style recalc timer. Instead of
actually removing the timer, this CL adjusts when we schedule the timer to
match when we'll schedule the animation frame after we remove the recalc timer.

BUG= 337617 

Review URL: https://codereview.chromium.org/140763007
------------------------------------------------------------------------
Project Member Comment 3 by bugdroid1@chromium.org, Jan 24 2014
------------------------------------------------------------------------
r246806 | abarth@chromium.org | 2014-01-24T07:34:36.902295Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/renderer/test_runner/WebTestProxy.cpp?r1=246806&r2=246805&pathrev=246806

Make --dump-render-tree's requestAnimationFrame more real

This CL adds a call to layout in --dump-render-tree's fake implementation of
requestAnimationFrame, which more closely mirrors what the real implementation
of requestAnimationFrame does. The real implementation is driven by the
compositor, which doesn't exist when running with --dump-render-tree.

R=esprehn@chromium.org
BUG= 337617 

Review URL: https://codereview.chromium.org/131973020
------------------------------------------------------------------------
Project Member Comment 4 by bugdroid1@chromium.org, Jan 24 2014
------------------------------------------------------------------------
r246841 | falken@chromium.org | 2014-01-24T12:02:21.559012Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/renderer/test_runner/WebTestProxy.cpp?r1=246841&r2=246840&pathrev=246841

Revert of Make --dump-render-tree's requestAnimationFrame more real (https://codereview.chromium.org/131973020/)

Reason for revert:
This seems to cause layout test inspector/layer-compositing-reasons.html to timeout on
all platforms. Reverting it locally fixed the timeout.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20Blink&tests=inspector%2Flayer-compositing-reasons.html

Original issue's description:
> Make --dump-render-tree's requestAnimationFrame more real
> 
> This CL adds a call to layout in --dump-render-tree's fake implementation of
> requestAnimationFrame, which more closely mirrors what the real implementation
> of requestAnimationFrame does. The real implementation is driven by the
> compositor, which doesn't exist when running with --dump-render-tree.
> 
> R=esprehn@chromium.org
> BUG= 337617 
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246806

TBR=esprehn@chromium.org,abarth@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG= 337617 

Review URL: https://codereview.chromium.org/145173013
------------------------------------------------------------------------
Project Member Comment 5 by bugdroid1@chromium.org, Jan 25 2014
------------------------------------------------------------------------
r247081 | abarth@chromium.org | 2014-01-25T07:12:49.539020Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/renderer/test_runner/WebTestProxy.cpp?r1=247081&r2=247080&pathrev=247081

Make --dump-render-tree's requestAnimationFrame more real

This CL adds a call to layout in --dump-render-tree's fake implementation of
requestAnimationFrame, which more closely mirrors what the real implementation
of requestAnimationFrame does. The real implementation is driven by the
compositor, which doesn't exist when running with --dump-render-tree.

R=esprehn@chromium.org
BUG= 337617 

Review URL: https://codereview.chromium.org/131973020
------------------------------------------------------------------------
Project Member Comment 6 by bugdroid1@chromium.org, Jan 29 2014
------------------------------------------------------------------------
r247563 | abarth@chromium.org | 2014-01-29T01:22:21.704179Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/browser_plugin/browser_plugin_browsertest.cc?r1=247563&r2=247562&pathrev=247563

Fix mistaken assumptions in tests

This test assumes that spinning the event loop is sufficient to trigger
asynchronous style recalculation. After we move style recalculation to be
triggered off requestAnimationFrame, this assumption will no longer be valid.
This CL removes this assumption by triggering layout explicitly

R=fsamuel@chromium.org
BUG= 337617 

Review URL: https://codereview.chromium.org/148153013
------------------------------------------------------------------------
Project Member Comment 7 by bugdroid1@chromium.org, Jan 29 2014
------------------------------------------------------------------------
r247590 | abarth@chromium.org | 2014-01-29T02:15:19.576522Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/autofill/password_autofill_agent_browsertest.cc?r1=247590&r2=247589&pathrev=247590

PasswordAutofillAgentTest.InlineAutocomplete depends on details of style recalc timing

This CL fixes an incorrect assumption in
PasswordAutofillAgentTest.InlineAutocomplete. Previously, this code assumed
that one trip through the event loop was sufficient to ensure that style was
recalcuated. After we move recalc style to be triggered by
requestAnimationFrame, this assumption will no longer be valid. This CL changes
the test to explicitly recalc style by calling WebView::layout.

Unfortunately, we cannot simply call WebView::layout in straight-line code
because this test does some of its work off the event loop. Instead, we need to
schedule the call to layout after we schedule the other async work.

BUG= 337617 
R=gcasto@chromium.org

Review URL: https://codereview.chromium.org/136003013
------------------------------------------------------------------------
Project Member Comment 8 by bugdroid1@chromium.org, Jan 29 2014
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=165988

------------------------------------------------------------------------
r165988 | abarth@chromium.org | 2014-01-29T02:26:31.576990Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/tests/WebFrameTest.cpp?r1=165988&r2=165987&pathrev=165988
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.h?r1=165988&r2=165987&pathrev=165988
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/animations/display-none-terminates-animation.html?r1=165988&r2=165987&pathrev=165988
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/inspector/layer-tree-model.html?r1=165988&r2=165987&pathrev=165988
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/tests/ScrollingCoordinatorChromiumTest.cpp?r1=165988&r2=165987&pathrev=165988
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.cpp?r1=165988&r2=165987&pathrev=165988

Remove the recalc style timer

This CL removes the recalc style time and instead drives style recalcuation off
of requestAnimationFrame. Previously, the recalc timer might fire in the middle
of a frame, which could cause us to recalc style twice: once in the middle of
the frame and again just before putting up the frame. Driving recalc using
requestAnimationFrame avoids this problem by deferring scheduled recalcs until
it's time to put up the frame.

BUG= 337617 

Review URL: https://codereview.chromium.org/145133006
------------------------------------------------------------------------
Status: Fixed
Looks like this stuck.
FYI there have been reports of Blink not loading fonts when in a background tab ( bug 336170 ).  That can't be related to this bug based on when it started, but I suspect we may yet have bugs like that (styles were assumed to run at some point and trigger something  unrelated to style) before we're fully done with this.
Please file new bugs for any issues like that and mark them as blocking this bug.
Sign in to add a comment