Clip related property and border radius are now recorded for UMA

Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[yigu, 2017-01-04 19:12:53]

Description was changed from

==========
Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
==========

to

==========
Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-01-04 19:12:54]

Description was changed from

==========
Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[yigu, 2017-01-04 19:19:03]

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

[yigu, 2017-01-04 19:19:04]

Two more scrolling on main thread reasons have been added. That should be it for
now. Other reasons need more investigation to be added in the future. PTAL.
Thanks!

[ajuma, 2017-01-04 22:17:10]

cc/ lgtm

[bokan, 2017-01-05 14:05:49]

https://codereview.chromium.org/2613743002/diff/1/cc/input/main_thread_scroll...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/2613743002/diff/1/cc/input/main_thread_scroll...
cc/input/main_thread_scrolling_reason.h:51: // The number of flags in this
struct (excluding itself).
Please update this comment to describe the foot cannon that is this count value.
It's not the count of values, but the maximum shift + 2 because the loop in
InputHandlerProxy::RecordMainThreadScrollingReasons is wrong. If you could,
fixing the uses of this count value and renaming it to
kMainThreadScrollingReasonMaxValue (so that in this case it would be 1<<20 in a
followup patch would be great.

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

https://codereview.chromium.org/2613743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1774: if
(layer->size().isEmpty())
It's inconsistent that if the size is empty, we'll add some but not all the main
thread scrolling reasons. I'd move this up to before you declare
needsCompositedScrolling.

https://codereview.chromium.org/2613743002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right):

https://codereview.chromium.org/2613743002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1083:
MainThreadScrollingReason::kBackgroundNotOpaqueInRect)) {
You should also check that there aren't any of the non-LCD-text reasons
otherwise the assert below will fail. You currently don't test any layers that
have more than one reason so that wont happen, but I think you should add a test
that uses two reasons, e.g. "border-radius and opacity" and ensure that the
appropriate reasons are set.

[yigu, 2017-01-05 14:46:51]

Thanks David. PTAL.

https://codereview.chromium.org/2613743002/diff/1/cc/input/main_thread_scroll...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/2613743002/diff/1/cc/input/main_thread_scroll...
cc/input/main_thread_scrolling_reason.h:51: // The number of flags in this
struct (excluding itself).
On 2017/01/05 14:05:48, bokan wrote:
> Please update this comment to describe the foot cannon that is this count
value.
> It's not the count of values, but the maximum shift + 2 because the loop in
> InputHandlerProxy::RecordMainThreadScrollingReasons is wrong. If you could,
> fixing the uses of this count value and renaming it to
> kMainThreadScrollingReasonMaxValue (so that in this case it would be 1<<20 in
a
> followup patch would be great.

Done.

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

https://codereview.chromium.org/2613743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1774: if
(layer->size().isEmpty())
On 2017/01/05 14:05:48, bokan wrote:
> It's inconsistent that if the size is empty, we'll add some but not all the
main
> thread scrolling reasons. I'd move this up to before you declare
> needsCompositedScrolling.

Done.

https://codereview.chromium.org/2613743002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right):

https://codereview.chromium.org/2613743002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1083:
MainThreadScrollingReason::kBackgroundNotOpaqueInRect)) {
On 2017/01/05 14:05:48, bokan wrote:
> You should also check that there aren't any of the non-LCD-text reasons
> otherwise the assert below will fail. You currently don't test any layers that
> have more than one reason so that wont happen, but I think you should add a
test
> that uses two reasons, e.g. "border-radius and opacity" and ensure that the
> appropriate reasons are set.

Done.

[bokan, 2017-01-05 14:52:41]

lgtm % nit and comment

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

https://codereview.chromium.org/2613743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1731: if
(layer->size().isEmpty())
It might change behavior to have this above the `if` block below. Unless you
know it's better here, I'd leave it between the below `if` and `bool
needsCompositedScrolling`

https://codereview.chromium.org/2613743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right):

https://codereview.chromium.org/2613743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1094: uint32_t
m_LCDTextRelatedReasons;
nit: this can be a static constexpr I think.

[yigu, 2017-01-05 20:20:27]

Hi David. On second thought, I have modified the ClipPathTest. It tests both
layer->AncestorWithClipPath() and layer->DescendantWithClipPath() but not
layer->layoutObject()->hasClipPath(). There are cases that we composite with a
layer having clipPath. See
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/compositi....
PTAL. Thanks!

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

https://codereview.chromium.org/2613743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1731: if
(layer->size().isEmpty())
On 2017/01/05 14:52:41, bokan wrote:
> It might change behavior to have this above the `if` block below. Unless you
> know it's better here, I'd leave it between the below `if` and `bool
> needsCompositedScrolling`

Done.

https://codereview.chromium.org/2613743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right):

https://codereview.chromium.org/2613743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1094: uint32_t
m_LCDTextRelatedReasons;
On 2017/01/05 14:52:41, bokan wrote:
> nit: this can be a static constexpr I think.

Done.

[jwd, 2017-01-06 15:10:04]

lgtm

[bokan, 2017-01-06 15:46:10]

still lgtm

[yigu, 2017-01-06 15:46:38]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-01-06 15:46:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2613743002/#ps80001
(title: "Add clip path test for ancestor and descendant")

[commit-bot: I haz the power, 2017-01-06 15:46:59]

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-06 17:09:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483717598909590,
"parent_rev": "7b6d0ee2e211c47f3fd5234d9cf005dcb748112f", "commit_rev":
"c302a98ceb085bf25d0c004e666d64fd4c3ee9f6"}

[commit-bot: I haz the power, 2017-01-06 17:09:34]

Description was changed from

==========
Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Clip related property and border radius are now recorded for UMA

BUG=660907
TEST=StyleRelatedMainThreadScrollingReasonTest.ClipPathTest;
StyleRelatedMainThreadScrollingReasonTest.ClipTest;
StyleRelatedMainThreadScrollingReasonTest.BorderRadiusTest
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-01-06 17:09:36]

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