Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(750)

Issue 2198853002: Draw main frame custom scrollbars in root layer scrolling mode. (Closed)

Created:
4 years, 4 months ago by MuVen
Modified:
4 years, 3 months ago
Reviewers:
bokan, skobes
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Modified as per the discussion in the bug #

Patch Set 3 : modified as per review comments #

Total comments: 2

Patch Set 4 : Addressed Review comments #

Patch Set 5 : modified as per review comments #

Patch Set 6 : cleanup #

Total comments: 8

Patch Set 7 : addressed review comments #

Patch Set 8 : adding if check. as without which it will crash in the webkit_unit_tests #

Patch Set 9 : addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -18 lines) Patch
A + third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display.html View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -10 lines 0 comments Download
A + third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-expected.png View 1 2 3 4 5 6 7 8 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 7 8 1 chunk +13 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RootFrameViewport.h View 1 2 3 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RootFrameViewport.cpp View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 53 (34 generated)
bokan
You'll also want to add a test https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode2642 third_party/WebKit/Source/core/frame/FrameView.cpp:2642: if (!layoutViewItem().isNull() ...
4 years, 4 months ago (2016-08-11 14:34:29 UTC) #3
MuVen
https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2198853002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode2642 third_party/WebKit/Source/core/frame/FrameView.cpp:2642: if (!layoutViewItem().isNull() && layoutViewItem().layer()->hasCompositedLayerMapping()) { On 2016/08/11 14:34:29, bokan ...
4 years, 4 months ago (2016-08-11 14:45:57 UTC) #4
skobes
On 2016/08/11 14:34:29, bokan wrote: > IMO, instead of doing this, we should fix FrameView::layerForHozizontalScrollbar ...
4 years, 4 months ago (2016-08-11 19:03:59 UTC) #5
MuVen
Can we do like this ? If(RLS) pass horizontalScrollbar Layer from CLM to VisualViewport; else ...
4 years, 4 months ago (2016-08-12 08:26:43 UTC) #6
bokan
On 2016/08/11 19:03:59, skobes wrote: > On 2016/08/11 14:34:29, bokan wrote: > > IMO, instead ...
4 years, 4 months ago (2016-08-12 14:00:28 UTC) #7
MuVen
PTAL. Thanks,
4 years, 3 months ago (2016-09-01 11:18:50 UTC) #12
skobes
I think the RLS / non-RLS decision branch should be in RootFrameViewport as bokan suggested, ...
4 years, 3 months ago (2016-09-01 21:00:18 UTC) #20
MuVen
PTAL.
4 years, 3 months ago (2016-09-02 13:50:31 UTC) #29
bokan
https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html 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/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html#newcode26 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/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): ...
4 years, 3 months ago (2016-09-02 16:01:54 UTC) #30
MuVen
https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right): https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp#newcode330 third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:330: LocalFrame* frame = mainFrame(); On 2016/09/02 16:01:54, bokan wrote: ...
4 years, 3 months ago (2016-09-02 16:36:13 UTC) #31
bokan
On 2016/09/02 16:36:13, MuVen wrote: > https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp > File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right): > > https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp#newcode330 > ...
4 years, 3 months ago (2016-09-02 16:51:51 UTC) #32
MuVen
PTAL. https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html 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/LayoutTests/scrollbars/custom-scrollbar-display-on-root-layer-scrolls.html#newcode26 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: ...
4 years, 3 months ago (2016-09-06 10:07:29 UTC) #40
skobes
Code changes look good. We already have a number of layout tests that render custom ...
4 years, 3 months ago (2016-09-06 18:44:07 UTC) #41
MuVen
On 2016/09/06 18:44:07, skobes wrote: > Code changes look good. > > We already have ...
4 years, 3 months ago (2016-09-07 14:41:12 UTC) #42
bokan
lgtm but please file the bug mentioned below https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2198853002/diff/140001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode2682 third_party/WebKit/Source/core/frame/FrameView.cpp:2682: if ...
4 years, 3 months ago (2016-09-07 15:00:45 UTC) #43
skobes
lgtm w/ two nits: - commit message should state the effect of the change, e.g. ...
4 years, 3 months ago (2016-09-07 16:41:23 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2198853002/220001
4 years, 3 months ago (2016-09-08 04:16:34 UTC) #49
commit-bot: I haz the power
Committed patchset #9 (id:220001)
4 years, 3 months ago (2016-09-08 05:54:44 UTC) #51
commit-bot: I haz the power
4 years, 3 months ago (2016-09-08 05:56:00 UTC) #53
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73
Cr-Commit-Position: refs/heads/master@{#417203}

Powered by Google App Engine
This is Rietveld 408576698