Force compositing inputs to be clean for getBoundingClientRect

Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG=672457
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2647533002
Cr-Commit-Position: refs/heads/master@{#447158}
Committed: https://chromium.googlesource.com/chromium/src/+/2c04a7850056a21b6c1dcb6d63451c4a0f11e707

[smcgruer, 2017-01-18 20:30:50]

smcgruer@chromium.org changed reviewers:
+ chrishtr@chromium.org, flackr@chromium.org

[smcgruer, 2017-01-18 20:30:51]

I suspect this may be overkill for the purpose of getting the compositing inputs
updated for getBoundingClientRect, but at the very least it works and we can
iterate from here :)

[smcgruer, 2017-01-18 20:31:46]

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

[commit-bot: I haz the power, 2017-01-18 20:33:04]

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

[flackr, 2017-01-18 21:11:10]

Good CL for discussion. One thing we could do to limit the perf impact is have a
flag for whether we have an ancestor sticky position element.

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();
nit: We tend to use the following style to scope conditionally used optional
variables
if (FrameView* view = document().view())
  ... code which uses view

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get the
lifecycle past compositing inputs?

[smcgruer, 2017-01-18 21:17:14]

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();
On 2017/01/18 21:11:10, flackr wrote:
> nit: We tend to use the following style to scope conditionally used optional
> variables
> if (FrameView* view = document().view())
>   ... code which uses view

Done.

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/01/18 21:11:10, flackr wrote:
> Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get the
> lifecycle past compositing inputs?

I was aiming for minimal lifecycle update. updateAllLifecyclePhasesExceptPaint()
should work the same, except it will also do the following after
CompositingClean:

    InPaintInvalidation,
    PaintInvalidationClean,
    InPrePaint,
    PrePaintClean,

[flackr, 2017-01-18 21:38:05]

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/01/18 21:17:13, smcgruer wrote:
> On 2017/01/18 21:11:10, flackr wrote:
> > Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get
the
> > lifecycle past compositing inputs?
> 
> I was aiming for minimal lifecycle update.
updateAllLifecyclePhasesExceptPaint()
> should work the same, except it will also do the following after
> CompositingClean:
> 
>     InPaintInvalidation,
>     PaintInvalidationClean,
>     InPrePaint,
>     PrePaintClean,
> 

Oops, you are right, this function seems to advance the lifecycle just past the
compositing update which should get the inputs clean - maybe also DCHECK this?

[chrishtr, 2017-01-18 23:27:14]

On 2017/01/18 at 21:11:10, flackr wrote:
> Good CL for discussion. One thing we could do to limit the perf impact is have
a flag for whether we have an ancestor sticky position element.
> 
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();
> nit: We tend to use the following style to scope conditionally used optional
variables
> if (FrameView* view = document().view())
>   ... code which uses view
> 
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
> Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get the
lifecycle past compositing inputs?

updateAllLifecyclePhasesExceptPaint does in fact do that.

[chrishtr, 2017-01-18 23:28:00]

On 2017/01/18 at 21:11:10, flackr wrote:
> Good CL for discussion. One thing we could do to limit the perf impact is have
a flag for whether we have an ancestor sticky position element.

I think this bit will be necessary. This is performance-critical code.

> 
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();
> nit: We tend to use the following style to scope conditionally used optional
variables
> if (FrameView* view = document().view())
>   ... code which uses view
> 
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
> Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get the
lifecycle past compositing inputs?

[smcgruer, 2017-01-19 20:06:03]

One example of how we could detect being in a position:sticky subtree (code is
quite messy, apologies).

I'm open to other ideas. This approach aims to avoid adding anything to
LayoutObject, at the cost of doing a walk to the next LayoutBoxModelObject above
us. (Unsure if the cost of that walk is ever more than 1.)

[smcgruer, 2017-01-19 20:49:41]

Note: A similar approach is needed in at least FrameSelection::revealSelection
for issue 677035. There I have yet to work out how to get to the correct
LayoutBoxModelObject, but I actually think we might want to take a step back
first and think about how many and which places now need to clean compositing
input as well as do layout to make sure that the location of boxes is correct.

[chrishtr, 2017-01-19 21:29:20]

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();
Hmm. I think this will be incorrect if layout/style state is dirty and updating
layout/style will
result in a change to isInStickySubtree(). Also, on reflection, the logic here
to avoid
updating to compositing clean plus scrolling only has a significant effect if
the developer
dirtied style/layout, then called clientQuads before yielding, because otherwise
Blink would update
the entire lifecycle before coming back to script.

Now I'm not sure if this is important to optimize. And also detecting whether
there is any stick
positioned ancestor is not so easy without extra code to store more dirty bits
(your CL incurs a
tree walk up to the root every time clientQuads is called).

Now I think we should just land this patch without the conditional here.

[smcgruer, 2017-01-20 01:40:41]

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/01/19 21:29:19, chrishtr wrote:
> Hmm. I think this will be incorrect if layout/style state is dirty and
updating
> layout/style will
> result in a change to isInStickySubtree(). Also, on reflection, the logic here
> to avoid
> updating to compositing clean plus scrolling only has a significant effect if
> the developer
> dirtied style/layout, then called clientQuads before yielding, because
otherwise
> Blink would update
> the entire lifecycle before coming back to script.
> 
> Now I'm not sure if this is important to optimize. And also detecting whether
> there is any stick
> positioned ancestor is not so easy without extra code to store more dirty bits
> (your CL incurs a
> tree walk up to the root every time clientQuads is called).
> 
> Now I think we should just land this patch without the conditional here.

I think those are all fair points. I'm happy to return this to the unconditional
approach.

Any thoughts on measuring any possible performance impact?

[flackr, 2017-01-20 02:19:42]

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/01/20 01:40:41, smcgruer wrote:
> On 2017/01/19 21:29:19, chrishtr wrote:
> > Hmm. I think this will be incorrect if layout/style state is dirty and
> updating
> > layout/style will
> > result in a change to isInStickySubtree(). Also, on reflection, the logic
here
> > to avoid
> > updating to compositing clean plus scrolling only has a significant effect
if
> > the developer
> > dirtied style/layout, then called clientQuads before yielding, because
> otherwise
> > Blink would update
> > the entire lifecycle before coming back to script.
> > 
> > Now I'm not sure if this is important to optimize. And also detecting
whether
> > there is any stick
> > positioned ancestor is not so easy without extra code to store more dirty
bits
> > (your CL incurs a
> > tree walk up to the root every time clientQuads is called).
> > 
> > Now I think we should just land this patch without the conditional here.
> 
> I think those are all fair points. I'm happy to return this to the
unconditional
> approach.
> 
> Any thoughts on measuring any possible performance impact?

We could certainly measure how often we actually don't already have fresh
values, and/or how long it takes to update, but even with microbenchmark data
it's not clear to me if we would know the overall impact. I think we can rely on
telemetry pagesets catching if this regresses performance overall in the wild,
although I agree with Chris that it's likely going to be clean most of the time
and repeated calls will only incur the cost once unless they dirty something
which is a known bad pattern.

[smcgruer, 2017-01-20 02:32:29]

Current version does fail fast/dom/forced-layout-only-in-document.html, havent
checked why yet (a task for tomorrow!)

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/01/20 02:19:41, flackr wrote:
> On 2017/01/20 01:40:41, smcgruer wrote:
> > On 2017/01/19 21:29:19, chrishtr wrote:
> > > Hmm. I think this will be incorrect if layout/style state is dirty and
> > updating
> > > layout/style will
> > > result in a change to isInStickySubtree(). Also, on reflection, the logic
> here
> > > to avoid
> > > updating to compositing clean plus scrolling only has a significant effect
> if
> > > the developer
> > > dirtied style/layout, then called clientQuads before yielding, because
> > otherwise
> > > Blink would update
> > > the entire lifecycle before coming back to script.
> > > 
> > > Now I'm not sure if this is important to optimize. And also detecting
> whether
> > > there is any stick
> > > positioned ancestor is not so easy without extra code to store more dirty
> bits
> > > (your CL incurs a
> > > tree walk up to the root every time clientQuads is called).
> > > 
> > > Now I think we should just land this patch without the conditional here.
> > 
> > I think those are all fair points. I'm happy to return this to the
> unconditional
> > approach.
> > 
> > Any thoughts on measuring any possible performance impact?
> 
> We could certainly measure how often we actually don't already have fresh
> values, and/or how long it takes to update, but even with microbenchmark data
> it's not clear to me if we would know the overall impact. I think we can rely
on
> telemetry pagesets catching if this regresses performance overall in the wild,
> although I agree with Chris that it's likely going to be clean most of the
time
> and repeated calls will only incur the cost once unless they dirty something
> which is a known bad pattern.

Ack, uploaded new PS that takes the code back to PS2.

[smcgruer, 2017-01-20 02:36:25]

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

[commit-bot: I haz the power, 2017-01-20 02:36:42]

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

[flackr, 2017-01-20 02:47:02]

Please add some tests. Also, we should ensure the other common methods which
depend on the correct position have clean inputs as well (i.e. offsetTop,
offsetLeft, scrollIntoView).

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1130:
view->updateLifecycleToCompositingCleanPlusScrolling();
I suspect we'll need to do this for a lot of other methods, maybe we should add
a private helper that ensures clean compositing inputs. With this here we don't
need to updateStyleAndLayout above do we?

[commit-bot: I haz the power, 2017-01-20 03:56:02]

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

[commit-bot: I haz the power, 2017-01-20 03:56:03]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-01-20 16:07:09]

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1130:
view->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/01/20 02:47:02, flackr wrote:
> maybe we should add a private helper that ensures clean compositing inputs.

Done. I think it could also be just an anon function if that is preferential.

>  With this here we don't need to updateStyleAndLayout above do we?

I'm unsure. It appears to work, but I don't know the full impact of doing this.
Chris?

[smcgruer, 2017-01-20 16:08:46]

Note: I still need to add some tests that actually scroll and make sure that
everything remains good/etc. I'm currently trying to decide if these should be
LayoutTests or if there's a nicer (but still covering the same logic) unit-test
way to do it.

[chrishtr, 2017-01-20 16:28:21]

On 2017/01/20 at 16:07:09, smcgruer wrote:
>
https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Element.cpp:1130:
view->updateLifecycleToCompositingCleanPlusScrolling();
> On 2017/01/20 02:47:02, flackr wrote:
> > maybe we should add a private helper that ensures clean compositing inputs.
> 
> Done. I think it could also be just an anon function if that is preferential.
> 
> >  With this here we don't need to updateStyleAndLayout above do we?
> 
> I'm unsure. It appears to work, but I don't know the full impact of doing
this. Chris?

If you don't re-add the previous call to updateStyle...IgnorePendingStyleSheets,
you will regress the situation where there is a pending style sheet that
previously was not going to be updated, but will now. If you just add a call to
updateLifecycleToCompositingCleanPlusScrolling
after the existing call, it will do the right thing, because it starts with a
LayoutClean state.

[smcgruer, 2017-01-20 22:13:42]

On 2017/01/20 16:28:21, chrishtr wrote:
> On 2017/01/20 at 16:07:09, smcgruer wrote:
> >
>
https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Element.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Element.cpp:1130:
> view->updateLifecycleToCompositingCleanPlusScrolling();
> > On 2017/01/20 02:47:02, flackr wrote:
> > > maybe we should add a private helper that ensures clean compositing
inputs.
> > 
> > Done. I think it could also be just an anon function if that is
preferential.
> > 
> > >  With this here we don't need to updateStyleAndLayout above do we?
> > 
> > I'm unsure. It appears to work, but I don't know the full impact of doing
> this. Chris?
> 
> If you don't re-add the previous call to
updateStyle...IgnorePendingStyleSheets,
> you will regress the situation where there is a pending style sheet that
> previously was not going to be updated, but will now. If you just add a call
to
> updateLifecycleToCompositingCleanPlusScrolling
> after the existing call, it will do the right thing, because it starts with a
> LayoutClean state.

Ack. We had better leave it then :)

Sounds like the only thing left to do then is to add some LayoutTests (or some
other sort of JS test?) to make sure that position:sticky element positions work
now.

[smcgruer, 2017-01-23 16:15:53]

Description was changed from

==========
Force compositing inputs to be clean for getBoundingClientRect

This is necessary to have the StickyPositionScrollingConstraints setup
when getBoundingClientRect is called immediately after the page is
changed. See the bug for more details.

BUG=672457
==========

to

==========
Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG=672457
==========

[smcgruer, 2017-01-23 16:16:24]

LayoutTests added. PTAL.

[flackr, 2017-01-23 16:43:50]

Looking good, just a few test suggestions.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
(right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:54:
scroller.append(document.createElement('div'));
Can we have a test which inserts the sticky element and immediately queries it?

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:65:
}, "offsetLeft should be correct for sticky after script-caused layout");
offsetTop and offsetLeft could probably be a single test.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
It's probably worth adding a comment for why we need to do this despite updating
the lifecycle right after.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
It might be worth unit testing that the correct offset is returned here, maybe
after expecting CompositingClean, assert that the sticky LayoutBoxModelObject no
longer needs compositing inputs update and that the original offset matches the
new offset? i.e.

// Modify DOM, expect compositing inputs to be dirty.
EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
offset = Element->offsetLeft();
EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
EXPECT_EQ(Element->offsetLeft(), offset);

[smcgruer, 2017-01-24 15:26:31]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
(right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:54:
scroller.append(document.createElement('div'));
On 2017/01/23 16:43:50, flackr wrote:
> Can we have a test which inserts the sticky element and immediately queries
it?

Fantastic idea. This actually helped expose that my fix for offsetTop/offsetLeft
was wrong; the actual JS bindings end up calling
HTMLElement::offsetTopForBinding, which has almost exactly the same
implementation but isn't an override... I intend to pursue making that situation
go away, but in the meantime I added the same refresh code to
HTMLElement::offsetTopForBinding.

Oh, and added an insert test.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:65:
}, "offsetLeft should be correct for sticky after script-caused layout");
On 2017/01/23 16:43:50, flackr wrote:
> offsetTop and offsetLeft could probably be a single test.

Done.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
On 2017/01/23 16:43:50, flackr wrote:
> It's probably worth adding a comment for why we need to do this despite
updating
> the lifecycle right after.

Done.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
On 2017/01/23 16:43:50, flackr wrote:
> It might be worth unit testing that the correct offset is returned here, maybe
> after expecting CompositingClean, assert that the sticky LayoutBoxModelObject
no
> longer needs compositing inputs update and that the original offset matches
the
> new offset? i.e.
> 
> // Modify DOM, expect compositing inputs to be dirty.
> EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
> offset = Element->offsetLeft();
> EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
> EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
> EXPECT_EQ(Element->offsetLeft(), offset);

Tried this, however for reasons I don't yet understand, the following failed:

    padding->setInnerHTML("<div id='test'></div>");
    EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,
              document.lifecycle().state());
  
   
EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate());
<-- this is false, not true.
    int offsetTop = sticky->offsetTop();

Not sure why, but setInnerHTML (on anything, I tried it on the sticky element as
well as the padding) doesn't cause anything to flip the LMBO to
needsCompositingInputsUpdate().

[flackr, 2017-01-24 17:00:21]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
On 2017/01/24 15:26:30, smcgruer wrote:
> On 2017/01/23 16:43:50, flackr wrote:
> > It might be worth unit testing that the correct offset is returned here,
maybe
> > after expecting CompositingClean, assert that the sticky
LayoutBoxModelObject
> no
> > longer needs compositing inputs update and that the original offset matches
> the
> > new offset? i.e.
> > 
> > // Modify DOM, expect compositing inputs to be dirty.
> > EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
> > offset = Element->offsetLeft();
> > EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
> > EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
> > EXPECT_EQ(Element->offsetLeft(), offset);
> 
> Tried this, however for reasons I don't yet understand, the following failed:
> 
>     padding->setInnerHTML("<div id='test'></div>");
>     EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,
>               document.lifecycle().state());
>   
>    
>
EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate());
> <-- this is false, not true.
>     int offsetTop = sticky->offsetTop();
> 
> Not sure why, but setInnerHTML (on anything, I tried it on the sticky element
as
> well as the padding) doesn't cause anything to flip the LMBO to
> needsCompositingInputsUpdate().

I think I know what's happening. This normally gets set when the layout within
the scrollable area changes by
PaintLayerScrollableArea::invalidateAllStickyConstraints:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

But layout is probably still dirty at this point.

[flackr, 2017-01-24 18:20:55]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
On 2017/01/24 17:00:21, flackr wrote:
> On 2017/01/24 15:26:30, smcgruer wrote:
> > On 2017/01/23 16:43:50, flackr wrote:
> > > It might be worth unit testing that the correct offset is returned here,
> maybe
> > > after expecting CompositingClean, assert that the sticky
> LayoutBoxModelObject
> > no
> > > longer needs compositing inputs update and that the original offset
matches
> > the
> > > new offset? i.e.
> > > 
> > > // Modify DOM, expect compositing inputs to be dirty.
> > > EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
> > > offset = Element->offsetLeft();
> > > EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
> > > EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
> > > EXPECT_EQ(Element->offsetLeft(), offset);
> > 
> > Tried this, however for reasons I don't yet understand, the following
failed:
> > 
> >     padding->setInnerHTML("<div id='test'></div>");
> >     EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,
> >               document.lifecycle().state());
> >   
> >    
> >
>
EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate());
> > <-- this is false, not true.
> >     int offsetTop = sticky->offsetTop();
> > 
> > Not sure why, but setInnerHTML (on anything, I tried it on the sticky
element
> as
> > well as the padding) doesn't cause anything to flip the LMBO to
> > needsCompositingInputsUpdate().
> 
> I think I know what's happening. This normally gets set when the layout within
> the scrollable area changes by
> PaintLayerScrollableArea::invalidateAllStickyConstraints:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> 
> But layout is probably still dirty at this point.

I guess remove the assertion that compositing inputs are dirty before the change
and just assert afterwards that inputs are clean + a position dependent on the
layout change.

[smcgruer, 2017-01-24 20:09:42]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
On 2017/01/24 18:20:54, flackr wrote:
> On 2017/01/24 17:00:21, flackr wrote:
> > On 2017/01/24 15:26:30, smcgruer wrote:
> > > On 2017/01/23 16:43:50, flackr wrote:
> > > > It might be worth unit testing that the correct offset is returned here,
> > maybe
> > > > after expecting CompositingClean, assert that the sticky
> > LayoutBoxModelObject
> > > no
> > > > longer needs compositing inputs update and that the original offset
> matches
> > > the
> > > > new offset? i.e.
> > > > 
> > > > // Modify DOM, expect compositing inputs to be dirty.
> > > > EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
> > > > offset = Element->offsetLeft();
> > > > EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
> > > > EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
> > > > EXPECT_EQ(Element->offsetLeft(), offset);
> > > 
> > > Tried this, however for reasons I don't yet understand, the following
> failed:
> > > 
> > >     padding->setInnerHTML("<div id='test'></div>");
> > >     EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,
> > >               document.lifecycle().state());
> > >   
> > >    
> > >
> >
>
EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate());
> > > <-- this is false, not true.
> > >     int offsetTop = sticky->offsetTop();
> > > 
> > > Not sure why, but setInnerHTML (on anything, I tried it on the sticky
> element
> > as
> > > well as the padding) doesn't cause anything to flip the LMBO to
> > > needsCompositingInputsUpdate().
> > 
> > I think I know what's happening. This normally gets set when the layout
within
> > the scrollable area changes by
> > PaintLayerScrollableArea::invalidateAllStickyConstraints:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> > 
> > But layout is probably still dirty at this point.
> 
> I guess remove the assertion that compositing inputs are dirty before the
change
> and just assert afterwards that inputs are clean + a position dependent on the
> layout change.

Added a partially-complete test to make sure that I understand what you're
looking for. Please ack the test and let me know if its not what you were
thinking of.

[flackr, 2017-01-24 20:22:28]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
On 2017/01/24 20:09:42, smcgruer wrote:
> On 2017/01/24 18:20:54, flackr wrote:
> > On 2017/01/24 17:00:21, flackr wrote:
> > > On 2017/01/24 15:26:30, smcgruer wrote:
> > > > On 2017/01/23 16:43:50, flackr wrote:
> > > > > It might be worth unit testing that the correct offset is returned
here,
> > > maybe
> > > > > after expecting CompositingClean, assert that the sticky
> > > LayoutBoxModelObject
> > > > no
> > > > > longer needs compositing inputs update and that the original offset
> > matches
> > > > the
> > > > > new offset? i.e.
> > > > > 
> > > > > // Modify DOM, expect compositing inputs to be dirty.
> > > > > EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
> > > > > offset = Element->offsetLeft();
> > > > > EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
> > > > > EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
> > > > > EXPECT_EQ(Element->offsetLeft(), offset);
> > > > 
> > > > Tried this, however for reasons I don't yet understand, the following
> > failed:
> > > > 
> > > >     padding->setInnerHTML("<div id='test'></div>");
> > > >     EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,
> > > >               document.lifecycle().state());
> > > >   
> > > >    
> > > >
> > >
> >
>
EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate());
> > > > <-- this is false, not true.
> > > >     int offsetTop = sticky->offsetTop();
> > > > 
> > > > Not sure why, but setInnerHTML (on anything, I tried it on the sticky
> > element
> > > as
> > > > well as the padding) doesn't cause anything to flip the LMBO to
> > > > needsCompositingInputsUpdate().
> > > 
> > > I think I know what's happening. This normally gets set when the layout
> within
> > > the scrollable area changes by
> > > PaintLayerScrollableArea::invalidateAllStickyConstraints:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> > > 
> > > But layout is probably still dirty at this point.
> > 
> > I guess remove the assertion that compositing inputs are dirty before the
> change
> > and just assert afterwards that inputs are clean + a position dependent on
the
> > layout change.
> 
> Added a partially-complete test to make sure that I understand what you're
> looking for. Please ack the test and let me know if its not what you were
> thinking of.

Acknowledged. That's exactly what I was suggesting.

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:54:
sticky.scrollIntoView();
Did we need to change layout before scrolling into view to test that it gets the
correct new constraints?

[smcgruer, 2017-01-24 22:20:01]

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:54:
sticky.scrollIntoView();
On 2017/01/24 20:22:28, flackr wrote:
> Did we need to change layout before scrolling into view to test that it gets
the
> correct new constraints?

Probably a good idea, done.

[flackr, 2017-01-24 22:47:16]

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:58:
scroller.append(newDiv);
nit: insert it before the sticky element here and below so that the old
constraints would produce the wrong answer.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129: // NOTE(smcgruer):
|updateLifecycleToCompositingCleanPlusScrolling| below
I don't think NOTE(author) is common chromium style, just the comment is good.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:103: "#sticky { height: 25px;
position: sticky; top: 0; left: 25px }"
nit: semicolon after 25px

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:136: EXPECT_EQ(offsetTop,
sticky->offsetTop());
Hm, I'm not sure what case I was worried about with ensuring the repeated call
returns the same value.

[smcgruer, 2017-01-25 20:51:43]

Description was changed from

==========
Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG=672457
==========

to

==========
Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG=672457
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[smcgruer, 2017-01-25 20:51:57]

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:58:
scroller.append(newDiv);
On 2017/01/24 22:47:15, flackr wrote:
> nit: insert it before the sticky element here and below so that the old
> constraints would produce the wrong answer.

Done. As usual, your offhand comments cause me to discover big new bugs, so this
also prompted a fix in PaintLayerScrollableArea so that we properly invalidate
the constraints on a scroller itself as well as its ancestor overlay.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129: // NOTE(smcgruer):
|updateLifecycleToCompositingCleanPlusScrolling| below
On 2017/01/24 22:47:16, flackr wrote:
> I don't think NOTE(author) is common chromium style, just the comment is good.

Done.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:103: "#sticky { height: 25px;
position: sticky; top: 0; left: 25px }"
On 2017/01/24 22:47:16, flackr wrote:
> nit: semicolon after 25px

Done.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementTest.cpp:136: EXPECT_EQ(offsetTop,
sticky->offsetTop());
On 2017/01/24 22:47:16, flackr wrote:
> Hm, I'm not sure what case I was worried about with ensuring the repeated call
> returns the same value.

Ok, removed.

[chrishtr, 2017-01-25 21:36:28]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
Previously it was updateStyleAndLayoutIgnorePendingStylesheetsForNode(this)
instead.
Why the change?

[smcgruer, 2017-01-25 21:39:05]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
On 2017/01/25 21:36:28, chrishtr wrote:
> Previously it was updateStyleAndLayoutIgnorePendingStylesheetsForNode(this)
> instead.
> Why the change?

All updateStyleAndLayoutIgnorePendingStylesheetsForNode(node) does is early-exit
on !node->inActiveDocument(), and then call
updateStyleAndLayoutIgnorePendingStylesheets(). I already do the early-exit
check above, so it seemed superfluous, but I can change it back if you'd prefer.

[flackr, 2017-01-25 21:48:18]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:105:
assert_equals(scroller.scrollTop, 195);
Shouldn't this be 220 (150px paddingBefore + 70px writer)?

[smcgruer, 2017-01-25 21:50:19]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:105:
assert_equals(scroller.scrollTop, 195);
On 2017/01/25 21:48:18, flackr wrote:
> Shouldn't this be 220 (150px paddingBefore + 70px writer)?

No; scrollIntoViewIfNeeded attempts to center the target element. So it's (150 +
70 - 25).

[flackr, 2017-01-25 21:53:27]

LGTM

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:105:
assert_equals(scroller.scrollTop, 195);
On 2017/01/25 21:50:19, smcgruer wrote:
> On 2017/01/25 21:48:18, flackr wrote:
> > Shouldn't this be 220 (150px paddingBefore + 70px writer)?
> 
> No; scrollIntoViewIfNeeded attempts to center the target element. So it's (150
+
> 70 - 25).

Ah, thanks for explanation!

[chrishtr, 2017-01-25 22:18:56]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
On 2017/01/25 at 21:39:05, smcgruer wrote:
> On 2017/01/25 21:36:28, chrishtr wrote:
> > Previously it was updateStyleAndLayoutIgnorePendingStylesheetsForNode(this)
> > instead.
> > Why the change?
> 
> All updateStyleAndLayoutIgnorePendingStylesheetsForNode(node) does is
early-exit on !node->inActiveDocument(), and then call
updateStyleAndLayoutIgnorePendingStylesheets(). I already do the early-exit
check above, so it seemed superfluous, but I can change it back if you'd prefer.

Oh ok.

[chrishtr, 2017-01-25 22:19:04]

(defer to flackr on rest of review)

[smcgruer, 2017-01-25 23:30:02]

flackr has LGTM'd, but I still need an LGTM from someone with Blink OWNERS.

[chrishtr, 2017-01-26 00:42:33]

lgtm

[smcgruer, 2017-01-26 03:02:31]

The CQ bit was checked by smcgruer@chromium.org

[commit-bot: I haz the power, 2017-01-26 03:04:18]

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

[commit-bot: I haz the power, 2017-01-26 04:48:08]

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

[commit-bot: I haz the power, 2017-01-26 04:48:09]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[smcgruer, 2017-01-26 11:17:02]

The CQ bit was checked by smcgruer@chromium.org

[commit-bot: I haz the power, 2017-01-26 11:17:19]

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

[commit-bot: I haz the power, 2017-01-26 12:47:03]

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

[commit-bot: I haz the power, 2017-01-26 12:47:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[smcgruer, 2017-01-26 15:04:55]

Ok, seems this patch changes something relating to animations, in such a way
that only a single test caught it -
inspector-protocol/animation/animation-empty-transition-cancel.html

I'm still digging, but before this CL the animation would be created and then
cancelled immediately due to this series of calls:

    node.offsetTop;
    node.style.transition = "1s";
    node.offsetTop;
    node.style.width = "200px";
    node.offsetTop;
    node.style.transition = "";
    node.offsetTop;

The animation would *not* be started.

After this CL, the animation is created *and* started, and never cancelled.

[smcgruer, 2017-01-26 16:22:07]

smcgruer@chromium.org changed reviewers:
+ samli@chromium.org

[smcgruer, 2017-01-26 16:22:08]

Found the root cause. InspectorAnimationAgent::animationPlayStateChanged will
not trigger an 'animationCancelled' event if it has ever seen the animation
started. (Because it early exits on 'm_idToAnimation.get(animationId)' being
true. An animation started event causes
'InspectorAnimationAgent::buildObjectForAnimation' to be called, which adds the
elementId to that hashmap.)

I'm unclear if the behavior is deliberate or not. It looks to have been
introduced in https://codereview.chromium.org/1422713012 if I'm reading the code
correctly, and it certainly doesn't look deliberate to me in that CL.

+samli since he wrote much of the InspectorAnimationAgent code.

Sam, was this intended behavior? For context, this CL changes the lifecycle
behavior of offsetTop/etc to also run compositing. That then had the knock-on
effect of causing the animation in
inspector-protocol/animation/animation-empty-transition-cancel.html to start
running before it is cancelled (due to a call to
Animation::notifyCompositorStartTime) which then triggered this above described
behavior in InspectorAnimationAgent::animationPlayStateChanged.

[smcgruer, 2017-01-26 16:24:34]

FYI with the following diff the test passes, though of course I don't know the
intended behavior in InspectorAnimationAgent.cpp :)

diff --git
a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
b/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
index 081bdb56fff1..c71f3ba9f8a8 100644
--- a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
+++ b/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
@@ -500,11 +500,11 @@ void InspectorAnimationAgent::animationPlayStateChanged(
     blink::Animation::AnimationPlayState oldPlayState,
     blink::Animation::AnimationPlayState newPlayState) {
   const String& animationId = String::number(animation->sequenceNumber());
-  if (m_idToAnimation.get(animationId) ||
-      m_clearedAnimations.contains(animationId))
+  if (m_clearedAnimations.contains(animationId))
     return;
-  if (newPlayState == blink::Animation::Running ||
-      newPlayState == blink::Animation::Finished)
+  if ((newPlayState == blink::Animation::Running ||
+       newPlayState == blink::Animation::Finished) &&
+      !m_idToAnimation.contains(animationId))
     frontend()->animationStarted(buildObjectForAnimation(*animation));
   else if (newPlayState == blink::Animation::Idle ||
            newPlayState == blink::Animation::Paused)

[smcgruer, 2017-01-27 13:05:35]

smcgruer@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org, suzyh@chromium.org
- samli@chromium.org

[smcgruer, 2017-01-27 13:05:36]

It appears that samli@ is no longer active in Chromium. Adding a few other
people who have touched InspectorAnimationAgent semi-recently, at the very least
hoping that they can point me in the right direction of whoever owns it now :)

suzyh, kozyatinskiy, please see comments above regarding what I think is wrong
with InspectorAnimationAgent and how it is blocking this CL. Thanks.

[samli, 2017-01-28 00:16:13]

On 2017/01/26 at 16:24:34, smcgruer wrote:
> FYI with the following diff the test passes, though of course I don't know the
intended behavior in InspectorAnimationAgent.cpp :)
> 
> diff --git
a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
b/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
> index 081bdb56fff1..c71f3ba9f8a8 100644
> --- a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
> +++ b/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
> @@ -500,11 +500,11 @@ void InspectorAnimationAgent::animationPlayStateChanged(
>      blink::Animation::AnimationPlayState oldPlayState,
>      blink::Animation::AnimationPlayState newPlayState) {
>    const String& animationId = String::number(animation->sequenceNumber());
> -  if (m_idToAnimation.get(animationId) ||
> -      m_clearedAnimations.contains(animationId))
> +  if (m_clearedAnimations.contains(animationId))
>      return;
> -  if (newPlayState == blink::Animation::Running ||
> -      newPlayState == blink::Animation::Finished)
> +  if ((newPlayState == blink::Animation::Running ||
> +       newPlayState == blink::Animation::Finished) &&
> +      !m_idToAnimation.contains(animationId))
>      frontend()->animationStarted(buildObjectForAnimation(*animation));
>    else if (newPlayState == blink::Animation::Idle ||
>             newPlayState == blink::Animation::Paused)

Diff looks fine to me.

[suzyh_UTC10 (ex-contributor), 2017-01-30 20:05:59]

On 2017/01/27 at 13:05:36, smcgruer wrote:
> It appears that samli@ is no longer active in Chromium. Adding a few other
people who have touched InspectorAnimationAgent semi-recently, at the very least
hoping that they can point me in the right direction of whoever owns it now :)
> 
> suzyh, kozyatinskiy, please see comments above regarding what I think is wrong
with InspectorAnimationAgent and how it is blocking this CL. Thanks.

Apologies for the delay in responding! Thanks for sending that
InspectorAnimationAgent CL through. I'm not very familiar with
InspectorAnimationAgent but am happy to chase up if you encounter any further
issues.

[smcgruer, 2017-01-30 22:20:41]

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

[commit-bot: I haz the power, 2017-01-30 22:21:29]

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

[commit-bot: I haz the power, 2017-01-31 01:36:57]

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

[commit-bot: I haz the power, 2017-01-31 01:36:58]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-01-31 01:44:28]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-01-31 01:44:29]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2647533002/#ps250001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-31 01:44:50]

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

[commit-bot: I haz the power, 2017-01-31 01:49:50]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1485827068864920,
"parent_rev": "909de64ab362012e5480461304d8538eeaf71eef", "commit_rev":
"2c04a7850056a21b6c1dcb6d63451c4a0f11e707"}

[commit-bot: I haz the power, 2017-01-31 01:50:29]

Description was changed from

==========
Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG=672457
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG=672457
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2647533002
Cr-Commit-Position: refs/heads/master@{#447158}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c04a7850056a21b6c1dcb6d6345...
==========

[commit-bot: I haz the power, 2017-01-31 01:50:30]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/2c04a7850056a21b6c1dcb6d6345...

[johnme, 2017-02-01 13:00:09]

A revert of this CL (patchset #14 id:250001) has been created in
https://codereview.chromium.org/2667003003/ by johnme@chromium.org.

The reason for reverting is: transitions/unprefixed-transform-origin.html has
failed on almost every build of WebKit Mac10.11 (dbg) and WebKit Linux Trusty
(dbg) since this patch landed.

Each time it's because the transitionend event fails to fire for property
-webkit-transform-origin-z.

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=tr....