Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom
scrollbars are picked from CompositedLayerMapping, Non-RLS  custom scrollbars
are picked from PaintLayerCompositor.

BUG=623853
Committed: https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73
Cr-Commit-Position: refs/heads/master@{#417203}

[MuVen, 2016-08-01 13:27:24]

Description was changed from

==========
done

BUG=
==========

to

==========
MainFrame CustomScrollbars are not drawn on RLS.

BUG=623853
==========

[bokan, 2016-08-11 14:34:11]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org, skobes@chromium.org

[bokan, 2016-08-11 14:34:29]

You'll also want to add a test

https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2642: if
(!layoutViewItem().isNull() &&
layoutViewItem().layer()->hasCompositedLayerMapping()) {
IMO, instead of doing this, we should fix FrameView::layerForHozizontalScrollbar
and friends to return the appropriate layer and use that above.

+skobes@: Is it true that if LayoutView has a CompositedLayerMapping then we in
root-layer-scrolls? If so, then layerForHorizontalScrollbar can check that and
return the CLM scrollbar layer, otherwise the PLC scrollbar layer. Otherwise, I
guess you could just key off settings()->rootLayerScrolls(). Does that sound ok?

[MuVen, 2016-08-11 14:45:57]

https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2642: if
(!layoutViewItem().isNull() &&
layoutViewItem().layer()->hasCompositedLayerMapping()) {
On 2016/08/11 14:34:29, bokan wrote:
> IMO, instead of doing this, we should fix
FrameView::layerForHozizontalScrollbar
> and friends to return the appropriate layer and use that above.
> 
> +skobes@: Is it true that if LayoutView has a CompositedLayerMapping then we
in
> root-layer-scrolls? If so, then layerForHorizontalScrollbar can check that and
> return the CLM scrollbar layer, otherwise the PLC scrollbar layer. Otherwise,
I
> guess you could just key off settings()->rootLayerScrolls(). Does that sound
ok?

That's a great idea to fix in layerForHorizontalScrollbar. I shall add tests,
and modified patch, if skobes is ok with this.

[skobes, 2016-08-11 19:03:59]

On 2016/08/11 14:34:29, bokan wrote:
> IMO, instead of doing this, we should fix
FrameView::layerForHozizontalScrollbar
> and friends to return the appropriate layer and use that above.

I don't think we should change FrameView::layerForHozizontalScrollbar; I am
trying to be careful about maintaining the separation between FrameView and root
layer scrollable area paths.

How about adding a RLS-aware layerForHozizontalScrollbar() on VisualViewport?

> +skobes@: Is it true that if LayoutView has a CompositedLayerMapping then we
in
> root-layer-scrolls?

I don't think this is true, but you may be able to query
CLM::hasScrollingLayer().

[MuVen, 2016-08-12 08:26:43]

Can we do like this ?

If(RLS)
  pass horizontalScrollbar Layer from CLM to VisualViewport;
else
  pass horizontalScrollbar Layer from PLC to VisualViewport;

During Frameview synchronizedPaint; get the scrollbar layers from
VisualViewport.

[bokan, 2016-08-12 14:00:28]

On 2016/08/11 19:03:59, skobes wrote:
> On 2016/08/11 14:34:29, bokan wrote:
> > IMO, instead of doing this, we should fix
> FrameView::layerForHozizontalScrollbar
> > and friends to return the appropriate layer and use that above.
> 
> I don't think we should change FrameView::layerForHozizontalScrollbar; I am
> trying to be careful about maintaining the separation between FrameView and
root
> layer scrollable area paths.
> 
> How about adding a RLS-aware layerForHozizontalScrollbar() on VisualViewport?

That sounds fine, though probably on RootFrameViewport in that case.

[MuVen, 2016-09-01 10:39:44]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 10:39:57]

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

[MuVen, 2016-09-01 10:40:13]

The CQ bit was unchecked by sataya.m@samsung.com

[MuVen, 2016-09-01 11:18:39]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[MuVen, 2016-09-01 11:18:50]

PTAL. 

Thanks,

[commit-bot: I haz the power, 2016-09-01 11:18:55]

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

[MuVen, 2016-09-01 12:14:14]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 12:14:24]

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

[MuVen, 2016-09-01 12:22:13]

Patchset #5 (id:80001) has been deleted

[MuVen, 2016-09-01 12:33:33]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-09-01 14:45:52]

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

[commit-bot: I haz the power, 2016-09-01 14:45:53]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-01 21:00:18]

I think the RLS / non-RLS decision branch should be in RootFrameViewport as
bokan suggested, not in VisualViewport as I initially wrote.  VisualViewport
owns scrollbar layers, so it's weird for it to have getters that return
something else.

[MuVen, 2016-09-02 07:59:56]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 08:00:07]

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-09-02 09:23:05]

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

[commit-bot: I haz the power, 2016-09-02 09:23:06]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-09-02 09:28:40]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 09:28:55]

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-09-02 11:07:37]

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

[commit-bot: I haz the power, 2016-09-02 11:07:37]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-09-02 13:50:31]

PTAL.

[bokan, 2016-09-02 16:01:54]

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html
(right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html:26:
<body>
Nit: No body tags

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2682: if
(m_viewportScrollableArea) {
This can be a DCHECK since we're the main frame.

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:330: LocalFrame*
frame = mainFrame();
I don't think you need any of the changes in this file. If root layer scrolls is
enabled, layoutViewport() is the PLSA belonging to the root frame in which case
PaintLayerScrollableArea::layerForHorizontalScrollbar() does what you're doing
here.

If root layer scrolls is disabled, layoutViewport() is the FrameView so
FrameView::layerForHorizontalScrollbar is what you return here as well.

I think you only really need to add the scroll corner method but that can also
delegate to layoutViewport(). Or amy I missing something?

[MuVen, 2016-09-02 16:36:13]

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:330: LocalFrame*
frame = mainFrame();
On 2016/09/02 16:01:54, bokan wrote:
> I don't think you need any of the changes in this file. If root layer scrolls
is
> enabled, layoutViewport() is the PLSA belonging to the root frame in which
case
> PaintLayerScrollableArea::layerForHorizontalScrollbar() does what you're doing
> here.
> 
> If root layer scrolls is disabled, layoutViewport() is the FrameView so
> FrameView::layerForHorizontalScrollbar is what you return here as well.
> 
> I think you only really need to add the scroll corner method but that can also
> delegate to layoutViewport(). Or amy I missing something?

so as i understand its better to retain the old definition 
layoutViewport().layerForHorizontalScrollbar()(correct me if im wrong); here
layoutViewport will vary based on the RLS enabled or disabled. 

if(RLS -> enabled)
   PLSA->layerForHorizontalScrollbar() ; will be invoked
else
   FrameView->layerForHorizontalScrollbar(); will be invoked.

we need to add scrollcorner method delegating to layoutViewport(); as
scrollcorner method is not overridden.

[bokan, 2016-09-02 16:51:51]

On 2016/09/02 16:36:13, MuVen wrote:
>
https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):
> 
>
https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:330: LocalFrame*
> frame = mainFrame();
> On 2016/09/02 16:01:54, bokan wrote:
> > I don't think you need any of the changes in this file. If root layer
scrolls
> is
> > enabled, layoutViewport() is the PLSA belonging to the root frame in which
> case
> > PaintLayerScrollableArea::layerForHorizontalScrollbar() does what you're
doing
> > here.
> > 
> > If root layer scrolls is disabled, layoutViewport() is the FrameView so
> > FrameView::layerForHorizontalScrollbar is what you return here as well.
> > 
> > I think you only really need to add the scroll corner method but that can
also
> > delegate to layoutViewport(). Or amy I missing something?
> 
> so as i understand its better to retain the old definition 
> layoutViewport().layerForHorizontalScrollbar()(correct me if im wrong); here
> layoutViewport will vary based on the RLS enabled or disabled. 
> 
> if(RLS -> enabled)
>    PLSA->layerForHorizontalScrollbar() ; will be invoked
> else
>    FrameView->layerForHorizontalScrollbar(); will be invoked.
> 
> we need to add scrollcorner method delegating to layoutViewport(); as
> scrollcorner method is not overridden.

that's correct.

[MuVen, 2016-09-06 04:18:39]

Patchset #7 (id:160001) has been deleted

[MuVen, 2016-09-06 04:19:14]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 04:19:27]

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

[MuVen, 2016-09-06 04:39:55]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 04:40:07]

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

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

[commit-bot: I haz the power, 2016-09-06 06:09:03]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-09-06 10:07:29]

PTAL.

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html
(right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html:26:
<body>
On 2016/09/02 16:01:54, bokan wrote:
> Nit: No body tags

Done.

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2682: if
(m_viewportScrollableArea) {
On 2016/09/02 16:01:54, bokan wrote:
> This can be a DCHECK since we're the main frame.

Need if(m_viewportScrollableArea) check as without which webkit_unit_tests will
crash ex: FrameThrottlingTest.cpp.

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:330: LocalFrame*
frame = mainFrame();
On 2016/09/02 16:01:54, bokan wrote:
> I don't think you need any of the changes in this file. If root layer scrolls
is
> enabled, layoutViewport() is the PLSA belonging to the root frame in which
case
> PaintLayerScrollableArea::layerForHorizontalScrollbar() does what you're doing
> here.
> 
> If root layer scrolls is disabled, layoutViewport() is the FrameView so
> FrameView::layerForHorizontalScrollbar is what you return here as well.
> 
> I think you only really need to add the scroll corner method but that can also
> delegate to layoutViewport(). Or amy I missing something?

Done.

[skobes, 2016-09-06 18:44:07]

Code changes look good.

We already have a number of layout tests that render custom scrollbars.  Aren't
any of them fixed by this patch?  If so we should just update the expectations
instead of adding a new one.

[MuVen, 2016-09-07 14:41:12]

On 2016/09/06 18:44:07, skobes wrote:
> Code changes look good.
> 
> We already have a number of layout tests that render custom scrollbars. 
Aren't
> any of them fixed by this patch?  If so we should just update the expectations
> instead of adding a new one.

I have checked the test expectations, i think all the tests passed because they
are
ref tests, as the test cases behave same in the actual and the expected test
case.
If its a image based test then we would have encountered a failure.

[bokan, 2016-09-07 15:00:45]

lgtm but please file the bug mentioned below

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2682: if
(m_viewportScrollableArea) {
On 2016/09/06 10:07:29, MuVen wrote:
> On 2016/09/02 16:01:54, bokan wrote:
> > This can be a DCHECK since we're the main frame.
> 
> Need if(m_viewportScrollableArea) check as without which webkit_unit_tests
will
> crash ex: FrameThrottlingTest.cpp.

Interesting, those tests need to be fixed. Please file a bug (cc me) and list
all the tests that break. The problem is that "Main frame doesn't create
RootFrameViewport in some webkit_unit_tests". Please add a TODO here linked to
that bug.

[skobes, 2016-09-07 16:41:23]

lgtm w/ two nits:

- commit message should state the effect of the change, e.g. "Draw main frame
custom scrollbars in root layer scrolling mode."

- don't put "root-layer-scrolls" in the test filename (the test is not really
specific to the feature)

[MuVen, 2016-09-08 04:12:10]

Description was changed from

==========
MainFrame CustomScrollbars are not drawn on RLS.

BUG=623853
==========

to

==========
Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom
scrollbars are picked from CompositedLayerMapping, Non-RLS  custom scrollbars
are picked from PaintLayerCompositor.

BUG=623853
==========

[MuVen, 2016-09-08 04:16:07]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[MuVen, 2016-09-08 04:16:10]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2016-09-08 04:16:10]

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

[commit-bot: I haz the power, 2016-09-08 04:16:34]

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-09-08 05:54:43]

Description was changed from

==========
Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom
scrollbars are picked from CompositedLayerMapping, Non-RLS  custom scrollbars
are picked from PaintLayerCompositor.

BUG=623853
==========

to

==========
Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom
scrollbars are picked from CompositedLayerMapping, Non-RLS  custom scrollbars
are picked from PaintLayerCompositor.

BUG=623853
==========

[commit-bot: I haz the power, 2016-09-08 05:54:44]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-09-08 05:55:59]

Description was changed from

==========
Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom
scrollbars are picked from CompositedLayerMapping, Non-RLS  custom scrollbars
are picked from PaintLayerCompositor.

BUG=623853
==========

to

==========
Draw main frame custom scrollbars in root layer scrolling mode.

Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom
scrollbars are picked from CompositedLayerMapping, Non-RLS  custom scrollbars
are picked from PaintLayerCompositor.

BUG=623853

Committed: https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73
Cr-Commit-Position: refs/heads/master@{#417203}
==========

[commit-bot: I haz the power, 2016-09-08 05:56:00]

Patchset 9 (id:??) landed as
https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73
Cr-Commit-Position: refs/heads/master@{#417203}