Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834573005:
Scrollbar no longer inherits from FrameViewBase. (Closed)
|
Created:
3 years, 8 months ago by joelhockey Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-frames_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionScrollbar no longer inherits from FrameViewBase.
FrameView now holds Scrollbars separate to other FrameView children.
BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2834573005
Cr-Commit-Position: refs/heads/master@{#466609}
Committed: https://chromium.googlesource.com/chromium/src/+/9a51ef8f83c3ebd1e7c04ec66e35cc191e9870fe
Patch Set 1 #
Total comments: 17
Patch Set 2 : Update from upstream patch #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (14 generated)
Description was changed from ========== Scrollbar no longer inherits from FrameViewBase. BUG=637460 ========== to ========== Scrollbar no longer inherits from FrameViewBase. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Scrollbar no longer inherits from FrameViewBase. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Scrollbar no longer inherits from FrameViewBase. FrameView now holds Scrollbars separate to other FrameView children. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
joelhockey@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org, szager@chromium.org
This CL follows on from the cleanup of the Convert methods. After making those changes, there is not a lot required to remove FrameViewBase as base class of Scrollbar.
The CQ bit was checked by joelhockey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.h:486: const ScrollbarsSet* Scrollbars() const { return &scrollbars_; } Can this return a ScrollbarSet& instead? https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:2016: ScrollableArea()->Box().GetDocument().View()->RemoveScrollbar(scrollbar); Can't this still be accessed via scrollbar->Parent() ? That would be simpler and easier to understand. https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/Scrollbar.h (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:78: virtual void SetParent(FrameViewBase* parent) { parent_ = parent; } Why virtual? https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:81: void SetFrameRect(const IntRect&); Shouldn't this still be an override?
https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:3908: for (const auto& child : children_) Don't we need to call FrameRectChanged for scrollbars_? https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4762: for (const auto& child : children_) Ditto. Don't we need to call SetParentVisible for scrollbars_? https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4786: child->SetParentVisible(true); Ditto. https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4797: for (const auto& child : children_) Ditto. https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/Scrollbar.h (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:255: Member<FrameViewBase> parent_; I think this can be Member<FrameView>. I think the parent of Scrollbar is guaranteed to be FrameView.
https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.h:486: const ScrollbarsSet* Scrollbars() const { return &scrollbars_; } Yes. I will do a followup CL and I can make children, plugins and scrollbars all be refs rather than pointers? https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:2016: ScrollableArea()->Box().GetDocument().View()->RemoveScrollbar(scrollbar); On 2017/04/21 at 08:37:52, szager1 wrote: > Can't this still be accessed via scrollbar->Parent() ? That would be simpler and easier to understand. No. The visibility between core and platform is at the heart of what makes this change quite tricky. scrollbar->Parent is of type FrameViewBase. The RemoveScrollbar method belongs to FrameView. I want to get away from the downcasting. https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/Scrollbar.h (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:78: virtual void SetParent(FrameViewBase* parent) { parent_ = parent; } On 2017/04/21 at 08:37:52, szager1 wrote: > Why virtual? LayoutScrollbar overrides to call a private UpdateScrollbarParts. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:81: void SetFrameRect(const IntRect&); On 2017/04/21 at 08:37:52, szager1 wrote: > Shouldn't this still be an override? Yes, thanks. It is still overriding ScrollbarThemeClient::SetFrameRect. Shouldn't I get a compile error or warning? https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/Scrollbar.h:255: Member<FrameViewBase> parent_; On 2017/04/21 at 08:42:03, haraken wrote: > I think this can be Member<FrameView>. I think the parent of Scrollbar is guaranteed to be FrameView. I wish! That would have made my work easier ;-) No, FrameView is not visible to platform.
Sorry, these comments didn't show up in the UI. I hope my responses are understandable. > https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:3908: for (const auto& child : children_) > > Don't we need to call FrameRectChanged for scrollbars_? No, scrollbars never implemented that. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Frame... > https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:4762: for (const auto& child : children_) > > Ditto. Don't we need to call SetParentVisible for scrollbars_? No, same again. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Frame... > https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/scroll/Scrollbar.h:255: Member<FrameViewBase> parent_; > > I think this can be Member<FrameView>. I think the parent of Scrollbar is guaranteed to be FrameView. FrameView is not visible to platform, must be FrameViewBase.
https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:2016: ScrollableArea()->Box().GetDocument().View()->RemoveScrollbar(scrollbar); On 2017/04/21 09:56:10, joelhockey wrote: > On 2017/04/21 at 08:37:52, szager1 wrote: > > Can't this still be accessed via scrollbar->Parent() ? That would be simpler > and easier to understand. > > No. The visibility between core and platform is at the heart of what makes this > change quite tricky. > > scrollbar->Parent is of type FrameViewBase. The RemoveScrollbar method belongs > to FrameView. I want to get away from the downcasting. I think the proper fix is to define Scrollbar ownership semantics in FrameViewBase rather than FrameView. Ideally, this would look something like: - Move ScrollbarManager from paint/ to platform/ - Make sure ScrollbarManger has all methods that Scrollbar needs - Make Scrollbar::parent_ a ScrollbarManager With that in place, all you need to do is move all remaining methods from FrameViewBase to FrameView, and declare victory.
> I think the proper fix is to define Scrollbar ownership semantics in > FrameViewBase rather than FrameView. Ideally, this would look something like: Sorry, what I meant was: "define Scrollbar ownership semantics in platform/ rather than in FrameView."
> - Make Scrollbar::parent_ a ScrollbarManager ... and for good measure, rename it to manager_.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:2016: ScrollableArea()->Box().GetDocument().View()->RemoveScrollbar(scrollbar); On 2017/04/21 10:22:36, szager1 wrote: > On 2017/04/21 09:56:10, joelhockey wrote: > > On 2017/04/21 at 08:37:52, szager1 wrote: > > > Can't this still be accessed via scrollbar->Parent() ? That would be > simpler > > and easier to understand. > > > > No. The visibility between core and platform is at the heart of what makes > this > > change quite tricky. > > > > scrollbar->Parent is of type FrameViewBase. The RemoveScrollbar method > belongs > > to FrameView. I want to get away from the downcasting. > > I think the proper fix is to define Scrollbar ownership semantics in > FrameViewBase rather than FrameView. Ideally, this would look something like: > > - Move ScrollbarManager from paint/ to platform/ > - Make sure ScrollbarManger has all methods that Scrollbar needs > - Make Scrollbar::parent_ a ScrollbarManager > > With that in place, all you need to do is move all remaining methods from > FrameViewBase to FrameView, and declare victory. Just to be totally clear, this line of code would become: scrollbar->Manager()->RemoveScrollbar(scrollbar) ... or even: scrollbar->RemoveFromManager()
The CQ bit was checked by joelhockey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:2016: ScrollableArea()->Box().GetDocument().View()->RemoveScrollbar(scrollbar); > Just to be totally clear, this line of code would become: > > scrollbar->Manager()->RemoveScrollbar(scrollbar) > > ... or even: > > scrollbar->RemoveFromManager() Yes, nice idea. I'll have a go at that. I still need to get https://codereview.chromium.org/2832883003 landed first when you have some time to look at it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/21 at 12:22:05, joelhockey wrote: > https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2834573005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:2016: ScrollableArea()->Box().GetDocument().View()->RemoveScrollbar(scrollbar); > > Just to be totally clear, this line of code would become: > > > > scrollbar->Manager()->RemoveScrollbar(scrollbar) > > > > ... or even: > > > > scrollbar->RemoveFromManager() > > Yes, nice idea. I'll have a go at that. I still need to get https://codereview.chromium.org/2832883003 landed first when you have some time to look at it. I've just been trying a few things. I definitely like the idea to move ScrollbarManager to platform and have that as parent of Scrollbar. With that change, it doesn't make sense to keep FrameViewBase at all. But then that has knock-on effects for RemoteFrameView and other places. My conclusion is that it would be best to first land this fairly simple change, and then I can make a number of other CLs. haraken@ (or anyone else), would you give an lgtm for this change, and I will send another CL today with szager's suggestion, as well as a few other CLs.
LGTM https://codereview.chromium.org/2834573005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2834573005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:486: const ScrollbarsSet* Scrollbars() const { return &scrollbars_; } I'm assuming that you have already checked all call sites for children_ and Children() and make sure that you don't need to add the scrollbar version there. (This CL is moving scrollbars from children_ to scrollbars_, so it must be taken care of.)
On 2017/04/24 at 07:22:17, haraken wrote: > LGTM > > https://codereview.chromium.org/2834573005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/2834573005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.h:486: const ScrollbarsSet* Scrollbars() const { return &scrollbars_; } > > I'm assuming that you have already checked all call sites for children_ and Children() and make sure that you don't need to add the scrollbar version there. (This CL is moving scrollbars from children_ to scrollbars_, so it must be taken care of.) Yes, I have checked them. The only one use in FrameView::InvalidateAllCustomScrollbarsOnActiveChanged szager@ (and others). I will follow up with CLs to move methods out of FrameViewBase, and move ScrollbarManager into platform as discussed.
The CQ bit was checked by joelhockey@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493018938585920,
"parent_rev": "5983d0ae0dc74c59f63c1851637ae2a8a13421d6", "commit_rev":
"9a51ef8f83c3ebd1e7c04ec66e35cc191e9870fe"}
Message was sent while issue was closed.
Description was changed from ========== Scrollbar no longer inherits from FrameViewBase. FrameView now holds Scrollbars separate to other FrameView children. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Scrollbar no longer inherits from FrameViewBase. FrameView now holds Scrollbars separate to other FrameView children. BUG=637460 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2834573005 Cr-Commit-Position: refs/heads/master@{#466609} Committed: https://chromium.googlesource.com/chromium/src/+/9a51ef8f83c3ebd1e7c04ec66e35... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/9a51ef8f83c3ebd1e7c04ec66e35...
Message was sent while issue was closed.
lgtm |
