Fix paint property under-invalidation checking about reference filters

Fix paint property under-invalidation checking about reference filters

Previously we checked equality of reference filters by comparing the
pointers to SkImageFilters. This cause false-positive when we forced
property update for under-invalidation checking.

Now ignore reference filters when comparing effect nodes, and check
for changes of filter operations defined in style to remedy the
ignored check.

An alternative way is to add value equality operators in SkImageFilter
subclasses, but that seems to need more code and time than this CL.
We might make that a long-term objective.

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

Committed: https://crrev.com/b3e11fe44bc15048029e2093a428234f0da084d3
Cr-Commit-Position: refs/heads/master@{#437470}

[Xianzhu, 2016-12-07 04:28:58]

Description was changed from

==========
Fix paint property under-invalidation checking about reference filters

Previously we checked equality of reference filters by comparing the
pointers to SkImageFilters. This cause false-positive when we forced
property update for under-invalidation checking.

Now ignore reference filters when comparing effect nodes, and check
for changes of filter operations defined in style to remedy the
ignored check.

An alternative way is to add value equality operators in SkImageFilter
subclasses, but that seems to need more code and time than this CL.
We might make that a long-term objective.

BUG=671097
==========

to

==========
Fix paint property under-invalidation checking about reference filters

Previously we checked equality of reference filters by comparing the
pointers to SkImageFilters. This cause false-positive when we forced
property update for under-invalidation checking.

Now ignore reference filters when comparing effect nodes, and check
for changes of filter operations defined in style to remedy the
ignored check.

An alternative way is to add value equality operators in SkImageFilter
subclasses, but that seems to need more code and time than this CL.
We might make that a long-term objective.

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

[Xianzhu, 2016-12-07 04:29:37]

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

[commit-bot: I haz the power, 2016-12-07 04:29:54]

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, 2016-12-07 04:57:19]

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

[commit-bot: I haz the power, 2016-12-07 04:57:20]

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

[Xianzhu, 2016-12-07 05:22:41]

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

[commit-bot: I haz the power, 2016-12-07 05:22:57]

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

[Xianzhu, 2016-12-07 05:34:07]

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

[commit-bot: I haz the power, 2016-12-07 05:34:14]

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, 2016-12-07 07:47:12]

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

[commit-bot: I haz the power, 2016-12-07 07:47:13]

Dry run: 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_...)

[Xianzhu, 2016-12-07 17:02:23]

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

[commit-bot: I haz the power, 2016-12-07 17:02:47]

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

[Xianzhu, 2016-12-07 17:03:47]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org, trchen@chromium.org

[commit-bot: I haz the power, 2016-12-07 18:31:18]

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

[commit-bot: I haz the power, 2016-12-07 18:31:19]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-12-08 02:00:41]

Nice find.

https://codereview.chromium.org/2556013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2556013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:434:
properties.setStyleFilterOperations(styleFilterOperations);
How much additional value will these checks give us? Looking at
ReferenceFilterOperation::operator==, I think we just check the style
(style="filter: url(#cat)" != style="filter: url('#dog')") but not the actual
value, such as if the #dog element's children change.

equalsIgnoringReferenceFilters already catches many cases such as when the total
number of operations change. WDYT of just relying on
equalsIgnoringReferenceFilters and not doing the extra checks of
styleFilterOperations? This will mean we fail to catch some cases, but it lets
us avoid adding a new pattern to FindObjectPropertiesNeedingUpdateScope and new
members to ObjectPaintProperties.

[Xianzhu, 2016-12-08 17:34:35]

https://codereview.chromium.org/2556013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2556013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:434:
properties.setStyleFilterOperations(styleFilterOperations);
On 2016/12/08 02:00:41, pdr. wrote:
> How much additional value will these checks give us? Looking at
> ReferenceFilterOperation::operator==, I think we just check the style
> (style="filter: url(#cat)" != style="filter: url('#dog')") but not the actual
> value, such as if the #dog element's children change.
> 
> equalsIgnoringReferenceFilters already catches many cases such as when the
total
> number of operations change. WDYT of just relying on
> equalsIgnoringReferenceFilters and not doing the extra checks of
> styleFilterOperations? This will mean we fail to catch some cases, but it lets
> us avoid adding a new pattern to FindObjectPropertiesNeedingUpdateScope and
new
> members to ObjectPaintProperties.

You are right. The little additional value is not worth the complexity.
Done.

[Xianzhu, 2016-12-08 19:38:36]

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

[commit-bot: I haz the power, 2016-12-08 19:39:24]

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

[pdr., 2016-12-08 21:14:12]

On 2016/12/08 at 17:34:35, wangxianzhu wrote:
>
https://codereview.chromium.org/2556013002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2556013002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:434:
properties.setStyleFilterOperations(styleFilterOperations);
> On 2016/12/08 02:00:41, pdr. wrote:
> > How much additional value will these checks give us? Looking at
> > ReferenceFilterOperation::operator==, I think we just check the style
> > (style="filter: url(#cat)" != style="filter: url('#dog')") but not the
actual
> > value, such as if the #dog element's children change.
> > 
> > equalsIgnoringReferenceFilters already catches many cases such as when the
total
> > number of operations change. WDYT of just relying on
> > equalsIgnoringReferenceFilters and not doing the extra checks of
> > styleFilterOperations? This will mean we fail to catch some cases, but it
lets
> > us avoid adding a new pattern to FindObjectPropertiesNeedingUpdateScope and
new
> > members to ObjectPaintProperties.
> 
> You are right. The little additional value is not worth the complexity.
> Done.

LGTM!

[Xianzhu, 2016-12-08 21:32:15]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2016-12-08 21:32:19]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-12-08 21:33:38]

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, 2016-12-09 00:14:15]

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

[commit-bot: I haz the power, 2016-12-09 00:14:16]

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

[Xianzhu, 2016-12-09 01:23:13]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-12-09 01:23:45]

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, 2016-12-09 04:47:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481246593259190,
"parent_rev": "3bec912b543461f16453a54d2a517dc10d94d800", "commit_rev":
"f9f50b89568948bcef4ae91482f5205f769e9975"}

[commit-bot: I haz the power, 2016-12-09 04:47:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-09 04:52:02]

Description was changed from

==========
Fix paint property under-invalidation checking about reference filters

Previously we checked equality of reference filters by comparing the
pointers to SkImageFilters. This cause false-positive when we forced
property update for under-invalidation checking.

Now ignore reference filters when comparing effect nodes, and check
for changes of filter operations defined in style to remedy the
ignored check.

An alternative way is to add value equality operators in SkImageFilter
subclasses, but that seems to need more code and time than this CL.
We might make that a long-term objective.

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

to

==========
Fix paint property under-invalidation checking about reference filters

Previously we checked equality of reference filters by comparing the
pointers to SkImageFilters. This cause false-positive when we forced
property update for under-invalidation checking.

Now ignore reference filters when comparing effect nodes, and check
for changes of filter operations defined in style to remedy the
ignored check.

An alternative way is to add value equality operators in SkImageFilter
subclasses, but that seems to need more code and time than this CL.
We might make that a long-term objective.

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

Committed: https://crrev.com/b3e11fe44bc15048029e2093a428234f0da084d3
Cr-Commit-Position: refs/heads/master@{#437470}
==========

[commit-bot: I haz the power, 2016-12-09 04:52:04]

Patchset 5 (id:??) landed as
https://crrev.com/b3e11fe44bc15048029e2093a428234f0da084d3
Cr-Commit-Position: refs/heads/master@{#437470}