Incorporate ComputedStyle::hasTransform when diffing transform styles.

Incorporate ComputedStyle::hasTransform when diffing transform styles.

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

Review-Url: https://codereview.chromium.org/2767783003
Cr-Commit-Position: refs/heads/master@{#458907}
Committed: https://chromium.googlesource.com/chromium/src/+/463b2dfe64df899fda4359f2d360ee1cba46e26b

[wkorman, 2017-03-21 23:31:37]

Description was changed from

==========
Incorporate ComputedStyle::hasTransform when diffing transform styles.

BUG=695125
==========

to

==========
Incorporate ComputedStyle::hasTransform when diffing transform styles.

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

[wkorman, 2017-03-21 23:32:45]

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

[commit-bot: I haz the power, 2017-03-21 23:33:04]

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-03-21 23:51:19]

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

[commit-bot: I haz the power, 2017-03-21 23:51:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2017-03-21 23:56:39]

wkorman@chromium.org changed reviewers:
+ alancutter@chromium.org, chrishtr@chromium.org, trchen@chromium.org

[wkorman, 2017-03-21 23:56:40]

Initial proposed change.

For chrishtr@ -- I think we should leave ComputedStyle::transformDataEquivalent
alone, since it is one of a batch of xxxDataEquivalent methods, all of which are
named and clearly intended to check *only* the rareNonInheritedData equivalence.

So instead I've updated the two callsites and added documentation.

You'd mentioned potentially only checking the hasCurrentTransformAnimation(),
but at both callsites I feel like we're actually looking to check any of the
conditions in hasTransform(), but maybe you know something I don't.

I'm trying to put together a test case but it may be quite difficult. trchen@
and I spent a day or so just winnowing the one from the bug, and it involves
setTimeout in conjunction with a JS busy loop, and requires clicking an element.
Have not yet been able to produce onLoad or without those items.

[chrishtr, 2017-03-22 00:04:01]

On 2017/03/21 at 23:56:40, wkorman wrote:
> Initial proposed change.
> 
> For chrishtr@ -- I think we should leave
ComputedStyle::transformDataEquivalent alone, since it is one of a batch of
xxxDataEquivalent methods, all of which are named and clearly intended to check
*only* the rareNonInheritedData equivalence.
> 
> So instead I've updated the two callsites and added documentation.
> 
> You'd mentioned potentially only checking the hasCurrentTransformAnimation(),
but at both callsites I feel like we're actually looking to check any of the
conditions in hasTransform(), but maybe you know something I don't.
> 
> I'm trying to put together a test case but it may be quite difficult. trchen@
and I spent a day or so just winnowing the one from the bug, and it involves
setTimeout in conjunction with a JS busy loop, and requires clicking an element.
Have not yet been able to produce onLoad or without those items.

How about a unittest that mutates the animation bit and then runs the style
callbacks?

[chrishtr, 2017-03-22 00:05:42]

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1030: if (hasTransform()
!= other.hasTransform() ||
Add a comment here too.

[alancutter (OOO until 2018), 2017-03-22 00:31:08]

On 2017/03/22 at 00:04:01, chrishtr wrote:
> How about a unittest that mutates the animation bit and then runs the style
callbacks?

A SimTest may be appropriate here as it will handle much of the boiler plate for
you.

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:393: if (oldStyle &&
oldStyle->hasTransform() == newStyle.hasTransform() &&
Perhaps hasTransform() would be better named hasTransformEffect().

[wkorman, 2017-03-22 01:28:02]

PTAL, added unit tests.

Note the PaintLayer test just calls updateTransform directly which IMO is
sufficient. Using PaintLayer::styleDidChange leads to complexity around
m_needsAncestorDependentCompositingInputsUpdateancestor which may be
surmountable but didn't seem to me like it would produce a better test.

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:393: if (oldStyle &&
oldStyle->hasTransform() == newStyle.hasTransform() &&
On 2017/03/22 00:31:08, alancutter wrote:
> Perhaps hasTransform() would be better named hasTransformEffect().

Some kind of rename would definitely be helpful. Effect is sort of an overloaded
phrase.

The actual logic doesn't even seem to be transform specific, for example, it
checks hasOffset() which I think can be mutated through non-default position. I
would say the method is more like "needsPaintLayer", but I think there are many
other cases that can lead to need for a paint layer.

In any case a rename best done in a different change. Open to naming thoughts
and I can pursue as follow-on if we come up with something.

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2767783003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1030: if (hasTransform()
!= other.hasTransform() ||
On 2017/03/22 00:05:42, chrishtr wrote:
> Add a comment here too.

Done.

[wkorman, 2017-03-22 01:29:00]

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

[commit-bot: I haz the power, 2017-03-22 01:30:19]

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

[chrishtr, 2017-03-22 02:06:33]

lgtm

[alancutter (OOO until 2018), 2017-03-22 02:36:29]

I'm reluctant to give this a pass without a more end-to-end test but from my
experience with the bug involving alert() I suspect it may be extremely
difficult. If from your understanding of the code you know of a simpler, more
testable repro case that would be preferable to include here, otherwise lgtm as
is.

[commit-bot: I haz the power, 2017-03-22 03:33:29]

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

[commit-bot: I haz the power, 2017-03-22 03:33:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[wkorman, 2017-03-22 20:31:17]

On 2017/03/22 02:36:29, alancutter wrote:
> I'm reluctant to give this a pass without a more end-to-end test but from my
> experience with the bug involving alert() I suspect it may be extremely
> difficult. If from your understanding of the code you know of a simpler, more
> testable repro case that would be preferable to include here, otherwise lgtm
as
> is.

Yeah, I hear you. I've spent a chunk of time exploring layout test options and
haven't
found anything feasible that's automated and not flaky.

I'll attach the last best effort to the bug so we have it for future reference
in
any case.

[wkorman, 2017-03-22 20:39:40]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-03-22 20:39:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2767783003/#ps40001
(title: "Sync to head.")

[commit-bot: I haz the power, 2017-03-22 20:40:05]

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-03-22 22:45:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490215180564040,
"parent_rev": "a0ca178033ce1a45919921fa67a050b808d706de", "commit_rev":
"463b2dfe64df899fda4359f2d360ee1cba46e26b"}

[commit-bot: I haz the power, 2017-03-22 22:46:23]

Description was changed from

==========
Incorporate ComputedStyle::hasTransform when diffing transform styles.

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

to

==========
Incorporate ComputedStyle::hasTransform when diffing transform styles.

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

Review-Url: https://codereview.chromium.org/2767783003
Cr-Commit-Position: refs/heads/master@{#458907}
Committed:
https://chromium.googlesource.com/chromium/src/+/463b2dfe64df899fda4359f2d360...
==========

[commit-bot: I haz the power, 2017-03-22 22:46:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/463b2dfe64df899fda4359f2d360...