Record box shadow as main thread scrolling reasons

Box shadow should be recorded as main thread scrolling reasons for non-root layers.

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.BoxShadowTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2638013002
Cr-Commit-Position: refs/heads/master@{#445728}
Committed: https://chromium.googlesource.com/chromium/src/+/c83ddee4e5dcb0fdce873021c2701c9c3a2c547a

[yigu, 2017-01-17 14:28:09]

Description was changed from

==========
Record box shadow and translucent border as main thread scrolling reasons

BUG=660907
==========

to

==========
Record box shadow and translucent border as main thread scrolling reasons

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

[yigu, 2017-01-17 14:28:09]

Description was changed from

==========
Record box shadow and translucent border as main thread scrolling reasons

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

to

==========
Record box shadow and translucent border as main thread scrolling reasons

BUG=660907
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[yigu, 2017-01-17 19:12:02]

yigu@chromium.org changed reviewers:
+ ajuma@chromium.org, bokan@chromium.org, pdr@chromium.org

[yigu, 2017-01-17 19:12:03]

Adding two more main thread scrolling reasons. The translucent border one needs
to be recorded only when we enable the RunTimeFeature flag
"compositingOpaqueScrollers". PTAL.
p.s. histograms.xml will be added later in this patch.

[pdr., 2017-01-17 21:22:02]

This looks good to me. I'll defer to Ajuma / Bokan for the final ok.

[bokan, 2017-01-17 21:36:55]

https://codereview.chromium.org/2638013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2638013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1781:
!layer->layoutObject()->style()->boxShadow();
This looks like it's doing more than just recording, this change looks like
we're disabling compositing if the box has a shadow. If that's already the case,
where does that happen? Similarly with hasTranslucentBorder - how come we have
to add the condition above? Where does it actually disable composited scrolling?

[yigu, 2017-01-17 21:50:59]

Description was changed from

==========
Record box shadow and translucent border as main thread scrolling reasons

BUG=660907
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Record box shadow and translucent border as main thread scrolling reasons

BUG=660907
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[yigu, 2017-01-17 21:50:59]

yigu@chromium.org changed reviewers:
+ flackr@chromium.org

[flackr, 2017-01-17 21:59:42]

https://codereview.chromium.org/2638013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2638013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1781:
!layer->layoutObject()->style()->boxShadow();
On 2017/01/17 21:36:55, bokan wrote:
> This looks like it's doing more than just recording, this change looks like
> we're disabling compositing if the box has a shadow. If that's already the
case,
> where does that happen? Similarly with hasTranslucentBorder - how come we have
> to add the condition above? Where does it actually disable composited
scrolling?
> 

We have this condition in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
which means the background cannot paint in the scrolling contents (line 1777
above). Seems that we should have a way of recording the reasons for the
background paint location so they can be added at the same place in the code
where they're detected. I think we can drop translucent borders now that the
general case is handled (we paint the BG in both the scrolling contents layer
and graphics layer).

[yigu, 2017-01-23 18:51:06]

Description was changed from

==========
Record box shadow and translucent border as main thread scrolling reasons

BUG=660907
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Record box shadow as main thread scrolling reasons

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.BoxShadowTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[yigu, 2017-01-23 18:53:51]

The reason due to box shadow is now recorded from where it gets detected. The
translucent border one has been removed as we can handle it from compositor now.
PTAL. Thanks!

https://codereview.chromium.org/2638013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2638013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1781:
!layer->layoutObject()->style()->boxShadow();
On 2017/01/17 21:59:42, flackr wrote:
> On 2017/01/17 21:36:55, bokan wrote:
> > This looks like it's doing more than just recording, this change looks like
> > we're disabling compositing if the box has a shadow. If that's already the
> case,
> > where does that happen? Similarly with hasTranslucentBorder - how come we
have
> > to add the condition above? Where does it actually disable composited
> scrolling?
> > 
> 
> We have this condition in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> which means the background cannot paint in the scrolling contents (line 1777
> above). Seems that we should have a way of recording the reasons for the
> background paint location so they can be added at the same place in the code
> where they're detected. I think we can drop translucent borders now that the
> general case is handled (we paint the BG in both the scrolling contents layer
> and graphics layer).

Done.

[bokan, 2017-01-23 19:55:51]

Thanks, lgtm % nits

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:200:
BackgroundPaintLocation backgroundPaintLocation(uint32_t* reasons = 0) const;
nit: s/0/nullptr

Also, please document what the method puts in `reasons` in the comment.

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1760:
uint32_t* mainThreadScrollingReasonsDueToBackgroundPaintLocation =
Why keep this separate variable? Just pass mainThreadScrollingReasons to
backgroundPaintLocation.

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1813: for
(uint32_t reason = 1;
Just fold this loop into addStyleRelatedMainThreadScrollingReasons

[yigu, 2017-01-23 20:21:24]

Thanks:)

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:200:
BackgroundPaintLocation backgroundPaintLocation(uint32_t* reasons = 0) const;
On 2017/01/23 19:55:51, bokan wrote:
> nit: s/0/nullptr
> 
> Also, please document what the method puts in `reasons` in the comment.

Done.

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1760:
uint32_t* mainThreadScrollingReasonsDueToBackgroundPaintLocation =
On 2017/01/23 19:55:51, bokan wrote:
> Why keep this separate variable? Just pass mainThreadScrollingReasons to
> backgroundPaintLocation.

Done.

https://codereview.chromium.org/2638013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1813: for
(uint32_t reason = 1;
On 2017/01/23 19:55:51, bokan wrote:
> Just fold this loop into addStyleRelatedMainThreadScrollingReasons

Done.

[yigu, 2017-01-23 20:21:54]

yigu@chromium.org changed reviewers:
+ jwd@chromium.org

[yigu, 2017-01-23 20:21:54]

yigu@chromium.org changed required reviewers:
+ ajuma@chromium.org, jwd@chromium.org

[ajuma, 2017-01-23 20:37:49]

cc lgtm

[flackr, 2017-01-23 20:52:12]

LGTM with nit

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

https://codereview.chromium.org/2638013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1788:
layer->layoutObject()->backgroundPaintLocation(
Can we pass mainThreadScrollingReasons into the call to backgroundPaintLocation
above so as not to call it again?

[yigu, 2017-01-23 22:06:00]

Description was changed from

==========
Record box shadow as main thread scrolling reasons

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.BoxShadowTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Box shadow should be recorded as main thread scrolling reasons for non-root
layers.

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.BoxShadowTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[yigu, 2017-01-24 01:00:10]

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

[commit-bot: I haz the power, 2017-01-24 01:01:26]

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-24 02:01:18]

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

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

Dry run: This issue passed the CQ dry run.

[jwd, 2017-01-24 15:06:26]

histograms lgtm

[yigu, 2017-01-24 15:07:11]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-01-24 15:07:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
flackr@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2638013002/#ps80001
(title: "nit")

[commit-bot: I haz the power, 2017-01-24 15:07:29]

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-24 15:11:49]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485270431837050,
"parent_rev": "bedcbce4a048c2e6c6eaf7a3cef82eecf85543e9", "commit_rev":
"c83ddee4e5dcb0fdce873021c2701c9c3a2c547a"}

[commit-bot: I haz the power, 2017-01-24 15:12:21]

Description was changed from

==========
Box shadow should be recorded as main thread scrolling reasons for non-root
layers.

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.BoxShadowTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Box shadow should be recorded as main thread scrolling reasons for non-root
layers.

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.BoxShadowTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2638013002
Cr-Commit-Position: refs/heads/master@{#445728}
Committed:
https://chromium.googlesource.com/chromium/src/+/c83ddee4e5dcb0fdce873021c270...
==========

[commit-bot: I haz the power, 2017-01-24 15:12:23]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c83ddee4e5dcb0fdce873021c270...