Update comments about not setting kHandlingScrollFromMainThread on subframes

When add main thread scrolling reason kHandlingScrolFromMainThread, there is no need to update the subframes as they are not scrollable when the reason is set to main frame.
BUG=675677
Review-Url: https://codereview.chromium.org/2620453003
Cr-Commit-Position: refs/heads/master@{#443228}
Committed: https://chromium.googlesource.com/chromium/src/+/21fc2f81007c4b950e49f817061aa7825577c812

[yigu, 2017-01-06 16:57:02]

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

[yigu, 2017-01-06 16:57:02]

Hi Philip,
We were talking about this TODO in a previous patch. Instead of setting
kHandlingScrollFromMainThread for subframes in ScrollAnimator, I'm thinking of
do it in FrameView where we update all reasons for subframes. WDYT?

[pdr., 2017-01-06 20:41:29]

https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
scrollLayer->mainThreadScrollingReasons();
Will updateSubFrameScrollOnMainReason always get called after
ScrollAnimator::addMainThreadScrollingReason and
ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test for
both of these cases?

kHandlingScrollFromMainThread is different from the other main thread scrolling
reasons. Could we unify them so all updates go through the same codepath?

[yigu, 2017-01-06 21:43:54]

On 2017/01/06 20:41:29, pdr. wrote:
>
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
> scrollLayer->mainThreadScrollingReasons();
> Will updateSubFrameScrollOnMainReason always get called after
> ScrollAnimator::addMainThreadScrollingReason and
> ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test for
> both of these cases?
> 
> kHandlingScrollFromMainThread is different from the other main thread
scrolling
> reasons. Could we unify them so all updates go through the same codepath?

As long as we could access FrameView in ScrollAnimator we could use
FrameView::updateSubFrameScrollingOnMainReason to do so. But I'm not sure how to
access Frame..

[pdr., 2017-01-06 22:04:21]

On 2017/01/06 at 21:43:54, yigu wrote:
> On 2017/01/06 20:41:29, pdr. wrote:
> >
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
> > scrollLayer->mainThreadScrollingReasons();
> > Will updateSubFrameScrollOnMainReason always get called after
> > ScrollAnimator::addMainThreadScrollingReason and
> > ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test for
> > both of these cases?
> > 
> > kHandlingScrollFromMainThread is different from the other main thread
scrolling
> > reasons. Could we unify them so all updates go through the same codepath?
> 
> As long as we could access FrameView in ScrollAnimator we could use
FrameView::updateSubFrameScrollingOnMainReason to do so. But I'm not sure how to
access Frame..

Could we pull it all up into
RootFrameViewport::updateCompositorScrollAnimations?

I don't know this area very well. Do you know if anyone else would be able to
offer us some high-level design advice?

[yigu, 2017-01-07 02:31:20]

On 2017/01/06 22:04:21, pdr. wrote:
> On 2017/01/06 at 21:43:54, yigu wrote:
> > On 2017/01/06 20:41:29, pdr. wrote:
> > >
>
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
> > > scrollLayer->mainThreadScrollingReasons();
> > > Will updateSubFrameScrollOnMainReason always get called after
> > > ScrollAnimator::addMainThreadScrollingReason and
> > > ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test
for
> > > both of these cases?
> > > 
> > > kHandlingScrollFromMainThread is different from the other main thread
> scrolling
> > > reasons. Could we unify them so all updates go through the same codepath?
> > 
> > As long as we could access FrameView in ScrollAnimator we could use
> FrameView::updateSubFrameScrollingOnMainReason to do so. But I'm not sure how
to
> access Frame..
> 
> Could we pull it all up into
> RootFrameViewport::updateCompositorScrollAnimations?
> 
> I don't know this area very well. Do you know if anyone else would be able to
> offer us some high-level design advice?

The current logic is written by ymalik@ who is no longer in Chrome Team. Do
chrishtr or wangxianzhu know this logic better? Guess I'm not that familiar with
the fellows yet.

[pdr., 2017-01-09 17:36:56]

On 2017/01/07 at 02:31:20, yigu wrote:
> On 2017/01/06 22:04:21, pdr. wrote:
> > On 2017/01/06 at 21:43:54, yigu wrote:
> > > On 2017/01/06 20:41:29, pdr. wrote:
> > > >
> >
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
> > > > scrollLayer->mainThreadScrollingReasons();
> > > > Will updateSubFrameScrollOnMainReason always get called after
> > > > ScrollAnimator::addMainThreadScrollingReason and
> > > > ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test
for
> > > > both of these cases?
> > > > 
> > > > kHandlingScrollFromMainThread is different from the other main thread
> > scrolling
> > > > reasons. Could we unify them so all updates go through the same
codepath?
> > > 
> > > As long as we could access FrameView in ScrollAnimator we could use
> > FrameView::updateSubFrameScrollingOnMainReason to do so. But I'm not sure
how to
> > access Frame..
> > 
> > Could we pull it all up into
> > RootFrameViewport::updateCompositorScrollAnimations?
> > 
> > I don't know this area very well. Do you know if anyone else would be able
to
> > offer us some high-level design advice?
> 
> The current logic is written by ymalik@ who is no longer in Chrome Team. Do
chrishtr or wangxianzhu know this logic better? Guess I'm not that familiar with
the fellows yet.

Ah, in that case we will have to learn about it ourselves :D

I think my suggestion to pull it all up into
RootFrameViewport::updateCompositorScrollAnimations would be a good design, but
not required. I would really like to see kHandlingScrollFromMainThread work like
all of the other main thread scrolling reasons though.

[yigu, 2017-01-11 22:44:45]

Hi Philip, PTAL. Thanks.

https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
scrollLayer->mainThreadScrollingReasons();
On 2017/01/06 20:41:29, pdr. wrote:
> Will updateSubFrameScrollOnMainReason always get called after
> ScrollAnimator::addMainThreadScrollingReason and
> ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test for
> both of these cases?
> 
> kHandlingScrollFromMainThread is different from the other main thread
scrolling
> reasons. Could we unify them so all updates go through the same codepath?

As discussed, we will not update this reason on subframes.

[pdr., 2017-01-11 23:14:50]

On 2017/01/11 at 22:44:45, yigu wrote:
> Hi Philip, PTAL. Thanks.
> 
>
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2620453003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/FrameView.cpp:4801: reasons |=
scrollLayer->mainThreadScrollingReasons();
> On 2017/01/06 20:41:29, pdr. wrote:
> > Will updateSubFrameScrollOnMainReason always get called after
> > ScrollAnimator::addMainThreadScrollingReason and
> > ScrollAnimator::removeMainThreadScrollingReason? Can you add a unit test for
> > both of these cases?
> > 
> > kHandlingScrollFromMainThread is different from the other main thread
scrolling
> > reasons. Could we unify them so all updates go through the same codepath?
> 
> As discussed, we will not update this reason on subframes.

LGTM please update the change description

[yigu, 2017-01-12 02:51:51]

Description was changed from

==========
Update kHandlingScrollFrommainThread and other potential reasons on subframe

BUG=675677
==========

to

==========
When add main thread scrolling reason kHandlingScrolFromMainThread, there is no
need to update the subframes as they are not scrollable when the reason is set
to main frame.
BUG=675677
==========

[yigu, 2017-01-12 02:52:18]

The CQ bit was checked by yigu@chromium.org

[commit-bot: I haz the power, 2017-01-12 02:52:49]

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-12 03:03:39]

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

[commit-bot: I haz the power, 2017-01-12 03:03:40]

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-...)

[yigu, 2017-01-12 14:38:58]

The CQ bit was checked by yigu@chromium.org

[commit-bot: I haz the power, 2017-01-12 14:39:18]

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-12 14:43:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484231938326470,
"parent_rev": "20b0d38e48ac6294d88377f1b09fbd54eb7704f4", "commit_rev":
"21fc2f81007c4b950e49f817061aa7825577c812"}

[commit-bot: I haz the power, 2017-01-12 14:43:41]

Description was changed from

==========
When add main thread scrolling reason kHandlingScrolFromMainThread, there is no
need to update the subframes as they are not scrollable when the reason is set
to main frame.
BUG=675677
==========

to

==========
When add main thread scrolling reason kHandlingScrolFromMainThread, there is no
need to update the subframes as they are not scrollable when the reason is set
to main frame.
BUG=675677

Review-Url: https://codereview.chromium.org/2620453003
Cr-Commit-Position: refs/heads/master@{#443228}
Committed:
https://chromium.googlesource.com/chromium/src/+/21fc2f81007c4b950e49f817061a...
==========

[commit-bot: I haz the power, 2017-01-12 14:43:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/21fc2f81007c4b950e49f817061a...