Add main thread scrolling reasons to GraphicsLayer::layerTreeAsJSON.

The mainThreadScrollingReasons are added to GraphicsLayerDebugInfo.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No tests written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7827029dfc7f728e6f328f897c102acf1fccfb6b
Cr-Commit-Position: refs/heads/master@{#405490}

[sahel, 2016-05-25 20:38:34]

Description was changed from

==========
Add main thread scrolling reasons to GraphicsLayer::layerTreeAsJSON.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No test written for this cl.

BUG=569086
==========

to

==========
Add main thread scrolling reasons to GraphicsLayer::layerTreeAsJSON.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No test written for this cl.

BUG=569086
==========

[sahel, 2016-05-25 20:38:34]

sahel@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2016-05-26 13:32:04]

tdresser@chromium.org changed reviewers:
+ vollick@chromium.org

[tdresser, 2016-05-26 13:32:04]

Ian, is this what you were thinking, or should we map the scrolling reasons to a
string?

Have you tested this locally?

[Ian Vollick, 2016-05-26 13:47:14]

On 2016/05/26 13:32:04, tdresser wrote:
> Ian, is this what you were thinking, or should we map the scrolling reasons to
a
> string?

Yep, I'd prefer a string.

[jbroman, 2016-05-26 14:08:33]

On 2016/05/26 at 13:47:14, vollick wrote:
> On 2016/05/26 13:32:04, tdresser wrote:
> > Ian, is this what you were thinking, or should we map the scrolling reasons
to a
> > string?
> 
> Yep, I'd prefer a string.

Drive-by: consider reusing
ScrollingCoordinator::mainThreadScrollingReasonsAsText.

[sahel, 2016-06-06 20:44:17]

On 2016/05/26 14:08:33, jbroman wrote:
> On 2016/05/26 at 13:47:14, vollick wrote:
> > On 2016/05/26 13:32:04, tdresser wrote:
> > > Ian, is this what you were thinking, or should we map the scrolling
reasons
> to a
> > > string?
> > 
> > Yep, I'd prefer a string.
> 
> Drive-by: consider reusing
> ScrollingCoordinator::mainThreadScrollingReasonsAsText.

@jbroman

When I try to use blink::ScrollingCoordinator::mainThreadScrollingReasonsAsText
in GraphicsLayer.cpp, I get linking error. 
Because including core/page/scrolling/ScrollingCoordinator.h in
GraphicsLayer.cpp is not allowed due to -core in
third_party/WebKit/Source/platform's dependency rules.
Do you have any idea how to resolve the issue?

-Thanks

[sahel, 2016-06-30 15:24:40]

Description was changed from

==========
Add main thread scrolling reasons to GraphicsLayer::layerTreeAsJSON.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No test written for this cl.

BUG=569086
==========

to

==========
Add main thread scrolling reasons to GraphicsLayer::layerTreeAsJSON.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No test written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sahel, 2016-06-30 15:30:45]

I couldn't include from core/... due to dependency rules.
So, I moved the helper function next to the enum.

The scroll layer was skipped in layerTreeAstext function in internals.cpp, I had
to write the scrollLayerTreeAsText function.

[sahel, 2016-06-30 15:31:15]

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

[commit-bot: I haz the power, 2016-06-30 15:31:36]

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-06-30 16:15:46]

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

[commit-bot: I haz the power, 2016-06-30 16:15:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdresser, 2016-07-01 14:36:01]

Deferring to vollick@, who has a better idea of what we want here. Did we want
to dump this data as part of layerTreeAsText?

I think we'll want some more thorough test coverage here.

[Ian Vollick, 2016-07-05 20:11:28]

On 2016/07/01 14:36:01, tdresser wrote:
> Deferring to vollick@, who has a better idea of what we want here. Did we want
> to dump this data as part of layerTreeAsText?
> 
> I think we'll want some more thorough test coverage here.

I'd prefer surfacing this data via layerTreeAsJSON
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

This makes it very easy to write tests for since you can dig through the JSON
and look for the values you expect rather than doing brittle text diffs.

[Ian Vollick, 2016-07-05 21:02:03]

On 2016/07/05 20:11:28, vollick wrote:
> On 2016/07/01 14:36:01, tdresser wrote:
> > Deferring to vollick@, who has a better idea of what we want here. Did we
want
> > to dump this data as part of layerTreeAsText?
> > 
> > I think we'll want some more thorough test coverage here.
> 
> I'd prefer surfacing this data via layerTreeAsJSON
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
> 
> This makes it very easy to write tests for since you can dig through the JSON
> and look for the values you expect rather than doing brittle text diffs.

Sorry, this was a super-braindead comment! :) FTR, I spoke with Sahel offline
and I agree that this would be good to add to layerTreeAsText, and suggested
adding a new constant LAYER_TREE_INCLUDES_XYZ to Internals to ensure we don't
skip important layers.

[sahel, 2016-07-06 19:22:05]

Patchset #3 (id:40001) has been deleted

[sahel, 2016-07-06 19:25:15]

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

[commit-bot: I haz the power, 2016-07-06 19:25:44]

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

[sahel, 2016-07-06 19:27:39]

Added LayerTreeIncludesScrollLayer flag, as we discussed with
vollick@.

By using this flag, I got rid of scrollLayerTreeAsText function in
Internals.cpp.

[commit-bot: I haz the power, 2016-07-06 20:33:16]

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

[commit-bot: I haz the power, 2016-07-06 20:33:17]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[sahel, 2016-07-07 15:15:13]

I added the flag to Internals.idl and changed the layout test to read the flag
from internals.

I also fixed a bug in the helper function in main_tead_scrolling_reasons.h ,
value 1 << 15 was missing in the helper function. (I made sure that 1:1 mapping
between the enum and helper function is correct now.)

I still expect some layouttests to fail for the following reasons:

1- if a layouttest prints the root layer, scrollayer is included as well, and
with this new change, the scrolling reasons will be printed. Once I get the
reviews, I'll correct the expected files of the failing layouttests.

2- I use scrollLayer() in PaintLayerCompositor.cpp to get the scroll layer and
it behaves differently on different machines, so my expected file is not always
the same as the actual. I don't know how to solve it, one way is just avoiding
to print the tree and use assertions to make sure that the string has the
mainThreadScrollingReasons, any other ideas?

[sahel, 2016-07-07 15:24:27]

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

[commit-bot: I haz the power, 2016-07-07 15:25:09]

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-07-07 16:34:00]

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

[commit-bot: I haz the power, 2016-07-07 16:34:01]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[sahel, 2016-07-07 21:16:55]

Patchset #5 (id:100001) has been deleted

[Ian Vollick, 2016-07-08 13:35:12]

On 2016/07/06 at 20:33:17, commit-bot wrote:
> Dry run: Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

Very sorry for waffling, but I think may be a better, though similar, approach
for surfacing the main thread scrolling reasons.

If we add the reasons to the GraphicsLayerDebugInfo rather than layerTreeAsJSON,
you should be able to visit about:tracing and do a frame viewer trace to see the
reasons on any page.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

And to test this code, you should be able to write a unit test rather than a
layout test. One example,
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Scro...

[sahel, 2016-07-12 14:24:32]

Description was changed from

==========
Add main thread scrolling reasons to GraphicsLayer::layerTreeAsJSON.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No test written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
The mainThreadScrollingReasons are added to GraphicsLayerDebugInfo.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No tests written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[tdresser, 2016-07-12 14:29:17]

-me, as vollick is a better reviewer here.

[Ian Vollick, 2016-07-12 15:12:13]

https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
cc/input/main_thread_scrolling_reason.h:81: uint32_t reasons) {
Perhaps this could convert to a TracedValue directly, and the
mainThreadScrollingReasonsAsText could just call ToString on it?

[sahel, 2016-07-12 17:20:59]

https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
cc/input/main_thread_scrolling_reason.h:81: uint32_t reasons) {
On 2016/07/12 15:12:13, vollick wrote:
> Perhaps this could convert to a TracedValue directly, and the
> mainThreadScrollingReasonsAsText could just call ToString on it?

The format of the text that stringCoordinator needs is neither a key-value pairs
nor array. It is a single string with comma separated names. Should I change the
ScrollingCoordinator::mainThreadScrollingReasonsAsText() function and where it's
called as well?

[Ian Vollick, 2016-07-12 17:27:34]

On 2016/07/12 at 17:20:59, sahel wrote:
>
https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
> File cc/input/main_thread_scrolling_reason.h (right):
> 
>
https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
> cc/input/main_thread_scrolling_reason.h:81: uint32_t reasons) {
> On 2016/07/12 15:12:13, vollick wrote:
> > Perhaps this could convert to a TracedValue directly, and the
> > mainThreadScrollingReasonsAsText could just call ToString on it?
> 
> The format of the text that stringCoordinator needs is neither a key-value
pairs nor array. It is a single string with comma separated names. Should I
change the ScrollingCoordinator::mainThreadScrollingReasonsAsText() function and
where it's called as well?

I may well be missing something, but I was under the impression that this data
was only surfaced, via Page, to Internals for layout tests.

If that's true, in the short term I imagine we could update the test
expectations for those tests. Longer term, though, it would be nice to convert
those to unit tests and remove the function from Internals.

[tdresser, 2016-07-12 17:40:30]

Actually -me.

[tdresser, 2016-07-12 17:40:59]

tdresser@chromium.org changed reviewers:
- tdresser@chromium.org

[sahel, 2016-07-12 20:35:24]

I changed the MainThreadScrollingReason helper function as recommended to avoid
using vectors.

https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
cc/input/main_thread_scrolling_reason.h:81: uint32_t reasons) {
On 2016/07/12 15:12:13, vollick wrote:
> Perhaps this could convert to a TracedValue directly, and the
> mainThreadScrollingReasonsAsText could just call ToString on it?

Done.

[Ian Vollick, 2016-07-12 21:05:25]

On 2016/07/12 at 20:35:24, sahel wrote:
> I changed the MainThreadScrollingReason helper function as recommended to
avoid using vectors.
> 
>
https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
> File cc/input/main_thread_scrolling_reason.h (right):
> 
>
https://codereview.chromium.org/2015433004/diff/140001/cc/input/main_thread_s...
> cc/input/main_thread_scrolling_reason.h:81: uint32_t reasons) {
> On 2016/07/12 15:12:13, vollick wrote:
> > Perhaps this could convert to a TracedValue directly, and the
> > mainThreadScrollingReasonsAsText could just call ToString on it?
> 
> Done.

Thanks. lgtm.

[sahel, 2016-07-12 21:31:02]

sahel@chromium.org changed reviewers:
+ bokan@chromium.org

[sahel, 2016-07-12 21:31:03]

@bokan

please review the following file.
third_party/WebKit/Source/web/PageOverlay.cpp

Thanks

[bokan, 2016-07-12 22:35:53]

Is the intent that we want to add this debugging info any time we add a main
thread scrolling reason? If so, this seems fragile (i.e. next time someone adds
a main thread scrolling reason there's a good chance they'll forget to add the
debugging info call).

I can see it's a bit tricky as WebLayer(Impl) and GraphicsLayer are on opposite
sides of the divide but it looks like cc::Layer already has a LayerClient*,
which GraphicsLayer implements. Could we add something like
didUpdateMainThreadScrollingReasons to that interface?

[sahel, 2016-07-13 14:11:26]

On 2016/07/12 22:35:53, bokan wrote:
> Is the intent that we want to add this debugging info any time we add a main
> thread scrolling reason? If so, this seems fragile (i.e. next time someone
adds
> a main thread scrolling reason there's a good chance they'll forget to add the
> debugging info call).
> 
> I can see it's a bit tricky as WebLayer(Impl) and GraphicsLayer are on
opposite
> sides of the divide but it looks like cc::Layer already has a LayerClient*,
> which GraphicsLayer implements. Could we add something like
> didUpdateMainThreadScrollingReasons to that interface?

Yes, we want to have mainThreadScrollingReasons in debugInfo of graphicsLayer, 
that's why any time that we add/clear reasons from weblayer, we need to updare
scrolling 
reasons of the debugInfo object of the GraphicsLayer.

I can see how it is fragile, but I don't know how using the interface would
solve the
problem. I think we would still need to call the proposed function that is
implemented in
graphicslayer any time that we add/update reasons in a weblayer, and it would be
basically
the same problem with some other function.
Do you think it would be easier to remember to call
didUpdateMainThreadScrollingReasons?

[bokan, 2016-07-13 14:36:04]

On 2016/07/13 14:11:26, sahel wrote:
> On 2016/07/12 22:35:53, bokan wrote:
> > Is the intent that we want to add this debugging info any time we add a main
> > thread scrolling reason? If so, this seems fragile (i.e. next time someone
> adds
> > a main thread scrolling reason there's a good chance they'll forget to add
the
> > debugging info call).
> > 
> > I can see it's a bit tricky as WebLayer(Impl) and GraphicsLayer are on
> opposite
> > sides of the divide but it looks like cc::Layer already has a LayerClient*,
> > which GraphicsLayer implements. Could we add something like
> > didUpdateMainThreadScrollingReasons to that interface?
> 
> Yes, we want to have mainThreadScrollingReasons in debugInfo of graphicsLayer,

> that's why any time that we add/clear reasons from weblayer, we need to updare
> scrolling 
> reasons of the debugInfo object of the GraphicsLayer.
> 
> I can see how it is fragile, but I don't know how using the interface would
> solve the
> problem. I think we would still need to call the proposed function that is
> implemented in
> graphicslayer any time that we add/update reasons in a weblayer, and it would
be
> basically
> the same problem with some other function.
> Do you think it would be easier to remember to call
> didUpdateMainThreadScrollingReasons?

The caller doesn't call didUpdateMainThreadScrollingReasons themselves, it would
be called from cc::Layer::AddMainThreadScrollingReasons (which is called from
WebLayerImpl's version) so that anytime that function is called we automatically
add the debugging info too. That way, you don't even have to modify the
callsites in Blink as the current patchset does.

[sahel, 2016-07-13 16:54:54]

I implemented the didUpdateMainThreadScrollingReasons as recommended.

[sahel, 2016-07-13 16:55:44]

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

[commit-bot: I haz the power, 2016-07-13 16:56:25]

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

[bokan, 2016-07-13 17:00:37]

Thanks! Just a nit/question but otherwise lgtm.

https://codereview.chromium.org/2015433004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2015433004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:350:
scrollbarGraphicsLayer->debugInfo().setMainThreadScrollingReasons(scrollbarGraphicsLayer->platformLayer()->mainThreadScrollingReasons());
You shouldn't need this or the below one anymore, right?

[sahel, 2016-07-13 17:14:30]

https://codereview.chromium.org/2015433004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2015433004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:350:
scrollbarGraphicsLayer->debugInfo().setMainThreadScrollingReasons(scrollbarGraphicsLayer->platformLayer()->mainThreadScrollingReasons());
On 2016/07/13 17:00:37, bokan wrote:
> You shouldn't need this or the below one anymore, right?

Right, I missed them, my bad.

[sahel, 2016-07-13 20:03:28]

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

[commit-bot: I haz the power, 2016-07-13 20:04:18]

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-07-13 20:04:22]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names without "master." prefix is deprecated:
tryserver.blink. For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-13 21:01:34]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-13 21:48:58]

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

[commit-bot: I haz the power, 2016-07-13 21:48:59]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-07-14 14:30:05]

On 2016/07/13 21:01:34, commit-bot: I haz the power wrote:
> Your CL relies on deprecated CQ feature(s):
>  * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
> "master." prefix is deprecated:
>      tryserver.blink
>    For more details, see http://crbug.com/617627.

patch 10: I had to make the didUpdateMainThreadScrollingReasons pure virtual
because the win_chromium_comple_debug_ng failed due to a liking error.

[sahel, 2016-07-14 14:30:19]

The CQ bit was checked by sahel@chromium.org

[sahel, 2016-07-14 14:30:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2015433004/#ps220001
(title: " ")

[commit-bot: I haz the power, 2016-07-14 14:30:31]

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-07-14 14:30:34]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 14:35:24]

Description was changed from

==========
The mainThreadScrollingReasons are added to GraphicsLayerDebugInfo.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No tests written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
The mainThreadScrollingReasons are added to GraphicsLayerDebugInfo.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No tests written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-14 14:35:26]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-07-14 14:35:35]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 14:37:18]

Description was changed from

==========
The mainThreadScrollingReasons are added to GraphicsLayerDebugInfo.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No tests written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
The mainThreadScrollingReasons are added to GraphicsLayerDebugInfo.

The scrolling reason is added when it isn't kNotScrollingOnMain.
No tests written for this cl.

BUG=569086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7827029dfc7f728e6f328f897c102acf1fccfb6b
Cr-Commit-Position: refs/heads/master@{#405490}
==========

[commit-bot: I haz the power, 2016-07-14 14:37:20]

Patchset 10 (id:??) landed as
https://crrev.com/7827029dfc7f728e6f328f897c102acf1fccfb6b
Cr-Commit-Position: refs/heads/master@{#405490}