Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942163002:
[Refactor] Moved scrollbar creation and deletion to ScrollbarManager (Closed)
Description[Refactor] Moved scrollbar creation and deletion to ScrollbarManager
We're currently in a limbo state where PLSA and FrameView each have
their own version of a ScrollbarManager that's slightly different. This
patch is a stepping on the way to moving scrollbar management out of
ScrollableArea and into a single ScrollbarManager. This will make it
cleaner to administer scrollbars directly from the RootFrameViewport
and fix the blocking bugs on the linked bug.
BUG=661236
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Patch Set 1 #Patch Set 2 : Move destroy scrollbar to scrollbar manager #Patch Set 3 : change ShouldUseCustomScrollbars #
Total comments: 9
Patch Set 4 : bokan and skobes comments addressed #Messages
Total messages: 83 (59 generated)
Description was changed from ========== [Not for Review] Moved scrollbar creation to ScrollbarManager BUG=661236 ========== to ========== [Not for Review] Moved scrollbar creation to ScrollbarManager BUG=661236 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@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...
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_...)
The CQ bit was checked by chaopeng@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...
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_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chaopeng@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...
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_...)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
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_...)
Description was changed from ========== [Not for Review] Moved scrollbar creation to ScrollbarManager BUG=661236 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [Refactor] Moved scrollbar creation and deletion to ScrollbarManager BUG=661236 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
Patchset #3 (id:80001) has been deleted
bokan@ PTAL. Thank you.
Please add more detail/context to the commit message. +skobes,szager as FYI https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrameView.cpp (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrameView.cpp:658: return nullptr; No need to have this case here, PLSA::LayoutObjectForScrollbars will return nullptr if we're in the main frame and custom scrollbars aren't allowed. https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:284: LayoutObject*& style_source) const { This method shouldn't have an output param. The caller should call LayoutObjectForScrollbars() if needed. https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1221: return layout_object; Lets return nullptr in this case. https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:40: LayoutObject* actual_layout_object = nullptr; layout_object_for_style https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:46: ToElement(actual_layout_object->GetNode())); Add a DCHECK that if we're using custom scrollbars, the layout object isn't a LayoutView. https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:79: scrollable_area_->WillRemoveScrollbar(*scrollbar, orientation); This used to only be called only for non-custom scrollbars but I think it should be fine to call for custom too (and better to do that). Could you please just confirm that this doesn't blow up in an obvious way for custom scrollbars? https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.h:134: virtual LayoutObject* LayoutObjectForScrollbars() const { return nullptr; } Lets call this LayoutObjectForScrollbarStyle. Also, add a TODO to move these out of ScrollableArea into a new interface class with the 661236 as the crbug #.
skobes@chromium.org changed reviewers: + skobes@chromium.org
What is the benefit of this change?
On 2017/06/27 18:07:33, skobes wrote: > What is the benefit of this change? We're currently in a limbo state where PLSA and FrameView each have their own version of a ScrollbarManager that's slightly different. This patch is a stepping stone on the way to moving scrollbar management out of ScrollableArea and into a single ScrollbarManager. This will make it cleaner to administer scrollbars directly from the RootFrameViewport and fix the blocking bugs on the linked bug.
On 2017/06/27 18:31:37, bokan wrote: > We're currently in a limbo state where PLSA and FrameView each have their own > version of a ScrollbarManager that's slightly different. This patch is a > stepping stone on the way to moving scrollbar management out of ScrollableArea > and into a single ScrollbarManager. This will make it cleaner to administer > scrollbars directly from the RootFrameViewport and fix the blocking bugs on the > linked bug. Thanks, I didn't notice the blocking bugs. With RLS I'm cautious about sharing stuff between FrameView and PLSA, and would prefer not to see more things added to PaintInvalidationCapableScrollableArea. But it would be good to reduce the RootFrameViewport weirdness so overall direction LGTM. https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:283: bool PaintInvalidationCapableScrollableArea::ShouldUseCustomScrollbars( I think this method would make more sense on LayoutObject, with the name HasCustomScrollbarStyle.
On 2017/06/27 18:48:15, skobes wrote: > On 2017/06/27 18:31:37, bokan wrote: > > We're currently in a limbo state where PLSA and FrameView each have their own > > version of a ScrollbarManager that's slightly different. This patch is a > > stepping stone on the way to moving scrollbar management out of ScrollableArea > > and into a single ScrollbarManager. This will make it cleaner to administer > > scrollbars directly from the RootFrameViewport and fix the blocking bugs on > the > > linked bug. > > Thanks, I didn't notice the blocking bugs. With RLS I'm cautious about sharing > stuff between FrameView and PLSA, and would prefer not to see more things added > to PaintInvalidationCapableScrollableArea. But it would be good to reduce the > RootFrameViewport weirdness so overall direction LGTM. It's shared but independent and reflects the current reality but in a code-healthier way. I think this should actually benefit RLS since we're removing potential differences between the two, with RLS we can just prevent creating a ScrollbarManager on FrameView. Agreed on PICScrollableArea, I think the reason Chao put it there was because ScrollableArea is in Source/platform and needs to touch Source/core but I like your idea of moving that into LayoutObject.
szager@chromium.org changed reviewers: + szager@chromium.org
Can you move ScrollbarManager to platform/scroll? That seems like a better place for it.
https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: Scrollbar* ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { All of this logic could be moved into PLSA and FrameView, to allow moving ScrollbarManager into platform.
On 2017/06/27 20:36:48, szager1 wrote: > Can you move ScrollbarManager to platform/scroll? That seems like a better > place for it. I think it needs to be in core since it's touching a bunch of other stuff in core. Being in "paint" is kinda weird but no weirder than PLSA being in paint. Long term I think PLSA should move into core/layout and be owned directly by LayoutBox.
On 2017/06/27 20:43:14, skobes wrote:
> On 2017/06/27 20:36:48, szager1 wrote:
> > Can you move ScrollbarManager to platform/scroll? That seems like a better
> > place for it.
>
> I think it needs to be in core since it's touching a bunch of other stuff in
> core. Being in "paint" is kinda weird but no weirder than PLSA being in
paint.
> Long term I think PLSA should move into core/layout and be owned directly by
> LayoutBox.
I'm not in love with the proposed end state here. I've thought a fair amount
about this, and I think a cleaner end state would be something like this:
platform/
class ScrollableArea {
virtual Scrollbar* CreateScrollbar(...)=0;
virtual void DestroyScrollbar(...)=0;
ScrollbarManager scrollbar_manager_;
};
class ScrollbarManager {
void SetHasHorizontalScrollbar(bool);
void SetHasVerticalScrollbar(bool);
Scrollbar* HorizontalScrollbar();
Scrollbar* VerticalScrollbar();
};
core/
class FrameView {
Scrollbar* CreateScrollbar(...) override;
void DestroyScrollbar(...) override;
}
class FrameView {
Scrollbar* CreateScrollbar(...) override;
void DestroyScrollbar(...) override;
}
The ScrollbarManager class should also incorporate the
DestroyDetachedScrollbars() logic that's currently in the PLSA::ScrollbarManager
class.
ScrollableArea could also inherit from ScrollbarManager, but "prefer composition
to inheritence", right?
On 2017/06/27 20:42:13, szager1 wrote: > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: Scrollbar* > ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { > All of this logic could be moved into PLSA and FrameView, to allow moving > ScrollbarManager into platform. The whole point here is to move the logic out of these classes so scrollbars are not so tightly coupled to their container and to break up these monolithic classes. This will allow scrollbars to be agnostic about what they're tied to, as long as it can provide them with style and geometry/scroll information. I agree core/layout is probably a better place for it though.
On 2017/06/27 20:57:16, szager1 wrote:
> platform/
> class ScrollableArea {
> virtual Scrollbar* CreateScrollbar(...)=0;
> virtual void DestroyScrollbar(...)=0;
> ScrollbarManager scrollbar_manager_;
> };
> class ScrollbarManager {
> void SetHasHorizontalScrollbar(bool);
> void SetHasVerticalScrollbar(bool);
> Scrollbar* HorizontalScrollbar();
> Scrollbar* VerticalScrollbar();
> };
To clarify slightly:
class ScrollbarManager {
...
ScrollbarManager(ScrollableArea& scrollable_area)
: scrollable_area_(scrollable_area) {}
ScrollableArea& scrollable_area_;
Scrollbar* horizontal_scrollbar_;
Scrollbar* vertical_scrollbar_;
};
void ScrollbarManager::SetHasHorizontalScrollbar(bool) {
...
horizontal_scrollbar_ = scrollable_area_.CreateScrollbar(...);
}
Description was changed from ========== [Refactor] Moved scrollbar creation and deletion to ScrollbarManager BUG=661236 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [Refactor] Moved scrollbar creation and deletion to ScrollbarManager We're currently in a limbo state where PLSA and FrameView each have their own version of a ScrollbarManager that's slightly different. This patch is a stepping on the way to moving scrollbar management out of ScrollableArea and into a single ScrollbarManager. This will make it cleaner to administer scrollbars directly from the RootFrameViewport and fix the blocking bugs on the linked bug. BUG=661236 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2017/06/27 20:59:05, bokan wrote: > On 2017/06/27 20:42:13, szager1 wrote: > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: Scrollbar* > > ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { > > All of this logic could be moved into PLSA and FrameView, to allow moving > > ScrollbarManager into platform. > > The whole point here is to move the logic out of these classes so scrollbars are > not so tightly coupled to their container and to break up these monolithic > classes. This will allow scrollbars to be agnostic about what they're tied to, > as long as it can provide them with style and geometry/scroll information. > > I agree core/layout is probably a better place for it though. I guess I'm late to the party here. There's nothing wrong with this CL, so I won't hold it up, but I do think we're missing an opportunity to do something better.
On 2017/06/27 21:08:52, szager1 wrote: > On 2017/06/27 20:59:05, bokan wrote: > > On 2017/06/27 20:42:13, szager1 wrote: > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: Scrollbar* > > > ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { > > > All of this logic could be moved into PLSA and FrameView, to allow moving > > > ScrollbarManager into platform. > > > > The whole point here is to move the logic out of these classes so scrollbars > are > > not so tightly coupled to their container and to break up these monolithic > > classes. This will allow scrollbars to be agnostic about what they're tied to, > > as long as it can provide them with style and geometry/scroll information. > > > > I agree core/layout is probably a better place for it though. > > I guess I'm late to the party here. There's nothing wrong with this CL, so I > won't hold it up, but I do think we're missing an opportunity to do something > better. Looks like our messages crossed. There's no rush and this work is just being restarted so we might as well come to an agreed direction before we set off. What benefit do we get from putting ScrollbarManager into platform? My understanding is that platform/ should be for wrapping things that the underlying OS is providing us, ScrollbarManager doesn't really fit in here (neither does ScrollableArea IMO but I think we need it to interact with Mac OS scrolling). To me this seems like an added layer of indirection without much benefit but maybe I'm missing something. From my POV, I'd like ScrollableArea to know as little as possible about scrollbars. Eventually, for overlay scrollbars like in Android/CrOS - Blink wouldn't even know about any scrollbars and they could be a detail implemented fully in CC (for non-overlay scrollbars Blink would just need to know how much gutter to add during layout). Keeping everything self-contained and communicating through specific interfaces gives us more flexibility and would be cleaner long term IMO.
The CQ bit was checked by chaopeng@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...
My two cents: I find the platform/core boundary unnatural and inconvenient. I'd recommend putting new scrolling stuff in core to minimize headaches. Even ScrollableArea would be better off in core. I don't have a strong opinion about the ownership model for ScrollableArea vs ScrollbarManager or whether ScrollableArea mediates scrollbar creation.
On 2017/06/27 21:16:22, bokan wrote: > On 2017/06/27 21:08:52, szager1 wrote: > > On 2017/06/27 20:59:05, bokan wrote: > > > On 2017/06/27 20:42:13, szager1 wrote: > > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: Scrollbar* > > > > ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { > > > > All of this logic could be moved into PLSA and FrameView, to allow moving > > > > ScrollbarManager into platform. > > > > > > The whole point here is to move the logic out of these classes so scrollbars > > are > > > not so tightly coupled to their container and to break up these monolithic > > > classes. This will allow scrollbars to be agnostic about what they're tied > to, > > > as long as it can provide them with style and geometry/scroll information. > > > > > > I agree core/layout is probably a better place for it though. > > > > I guess I'm late to the party here. There's nothing wrong with this CL, so I > > won't hold it up, but I do think we're missing an opportunity to do something > > better. > > Looks like our messages crossed. There's no rush and this work is just being > restarted so we might as well come to an agreed direction before we set off. > > What benefit do we get from putting ScrollbarManager into platform? My > understanding is that platform/ should be for wrapping things that the > underlying OS is providing us, ScrollbarManager doesn't really fit in here > (neither does ScrollableArea IMO but I think we need it to interact with Mac OS > scrolling). To me this seems like an added layer of indirection without much > benefit but maybe I'm missing something. > > From my POV, I'd like ScrollableArea to know as little as possible about > scrollbars. Eventually, for overlay scrollbars like in Android/CrOS - Blink > wouldn't even know about any scrollbars and they could be a detail implemented > fully in CC (for non-overlay scrollbars Blink would just need to know how much > gutter to add during layout). Keeping everything self-contained and > communicating through specific interfaces gives us more flexibility and would be > cleaner long term IMO. I don't think that's the meaning of "platform" in Source/platform. There's lots of non-OS-related stuff in there. I think of "platform" as stuff that could exist independent of the web/ layer. By that definition, it would be completely appropriate to put ScrollbarManager in platform/. I like the idea of putting it in platform/ because it isolates Scrollbar ownership and lifetime management. It's a simplifying assumption to say that only a ScrollbarManager can own a Scrollbar, and every ScrollbarManager is owned by a ScrollableArea. I initially added the ScrollbarManager class to PLSA in order to isolate the complexity of Scrollbar lifetime management during multi-pass and reentrant layout. I think that's still the core value that ScrollbarManager offers. As long as we support custom scrollbars, we'll never be able to create scrollbars from platform/ code. That's why I think it makes sense to add CreateScrollbar as a virtual method on ScrollableArea (assuming ScrollbarManager holds a reference to a ScrollableArea). Aside from that, I think the entire interface between ScrollableArea and Scrollbar can be defined via non-virtual methods in ScrollbarManager, and I think that would be good.
On 2017/06/27 21:44:07, skobes wrote: > My two cents: I find the platform/core boundary unnatural and inconvenient. I'd > recommend putting new scrolling stuff in core to minimize headaches. Even > ScrollableArea would be better off in core. > > I don't have a strong opinion about the ownership model for ScrollableArea vs > ScrollbarManager or whether ScrollableArea mediates scrollbar creation. There will always be a platform/core boundary *somewhere* in the scrolling code. My preference is that there is a single class that acts as the interface between platform and core, much the way that ChromeClient is the single interface for core/ to call into web/. For the time being, I think ScrollableArea is the de facto interface class, although I could imagine creating a ScrollClient class for this purpose, some time in the indeterminate future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Maybe ScrollbarManager can fulfill its original purpose in platform, but it sounds like we want to expand the scope of its responsibilities. Scrollbar management is closely connected to layout in general, so we are going to have some code for it in core no matter what. The question is whether to keep it in FrameView/PLSA, or move it into ScrollbarManager. If we're moving it into ScrollbarManager then ScrollbarManager needs to be in core.
On 2017/06/27 22:26:13, skobes wrote: > Maybe ScrollbarManager can fulfill its original purpose in platform, but it > sounds like we want to expand the scope of its responsibilities. Scrollbar > management is closely connected to layout in general, so we are going to have > some code for it in core no matter what. The question is whether to keep it in > FrameView/PLSA, or move it into ScrollbarManager. If we're moving it into > ScrollbarManager then ScrollbarManager needs to be in core. Aside from Scrollbar creation/destruction, what core/ functionality do we want ScrollbarManager to assume?
On 2017/06/27 21:55:16, szager1 wrote: > On 2017/06/27 21:16:22, bokan wrote: > > On 2017/06/27 21:08:52, szager1 wrote: > > > On 2017/06/27 20:59:05, bokan wrote: > > > > On 2017/06/27 20:42:13, szager1 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: Scrollbar* > > > > > ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { > > > > > All of this logic could be moved into PLSA and FrameView, to allow > moving > > > > > ScrollbarManager into platform. > > > > > > > > The whole point here is to move the logic out of these classes so > scrollbars > > > are > > > > not so tightly coupled to their container and to break up these monolithic > > > > classes. This will allow scrollbars to be agnostic about what they're tied > > to, > > > > as long as it can provide them with style and geometry/scroll information. > > > > > > > > I agree core/layout is probably a better place for it though. > > > > > > I guess I'm late to the party here. There's nothing wrong with this CL, so > I > > > won't hold it up, but I do think we're missing an opportunity to do > something > > > better. > > > > Looks like our messages crossed. There's no rush and this work is just being > > restarted so we might as well come to an agreed direction before we set off. > > > > What benefit do we get from putting ScrollbarManager into platform? My > > understanding is that platform/ should be for wrapping things that the > > underlying OS is providing us, ScrollbarManager doesn't really fit in here > > (neither does ScrollableArea IMO but I think we need it to interact with Mac > OS > > scrolling). To me this seems like an added layer of indirection without much > > benefit but maybe I'm missing something. > > > > From my POV, I'd like ScrollableArea to know as little as possible about > > scrollbars. Eventually, for overlay scrollbars like in Android/CrOS - Blink > > wouldn't even know about any scrollbars and they could be a detail implemented > > fully in CC (for non-overlay scrollbars Blink would just need to know how much > > gutter to add during layout). Keeping everything self-contained and > > communicating through specific interfaces gives us more flexibility and would > be > > cleaner long term IMO. > > I don't think that's the meaning of "platform" in Source/platform. There's lots > of > non-OS-related stuff in there. I think of "platform" as stuff that could exist > independent of the web/ layer. By that definition, it would be completely > appropriate to put ScrollbarManager in platform/. I think its meaning used to be somewhat convoluted but my read of Blink Onion Soup (https://docs.google.com/document/d/13XsbaBz7A2H0PZIdFcytHf5-fVOlAfkLlIUKhxKzs...) was that going forward we should treat platform as a system abstraction layer. Ideally we'd move most of ScrollableArea too into Blink and have a well-defined, tight interface solely for communicating with Mac's ScrollAnimator which I think is the main reason it's still in platform/ today. > There will always be a platform/core boundary *somewhere* in the scrolling code. Can you explain why that's the case? Scrolling is/should-be entirely internal to Blink and CC (and my plan for scroll unification is to move most of it to CC). We have some hooks from/to Mac code today that go through platform/ but I think they're a big source of pain for us. I'd love if we could think of a way of making Mac less intrusive but I don't have any answers there yet. That said, I don't think the Mac integration is really related to lifetime in any way so I don't see why platform/ needs to be involved in at all. I agree with Steve that I find the platform/ boundary to be artificial and a hindrance. While I'd prefer the creation/destruction logic to be separate from ScrollableArea, that's from an aesthetic and design perspective. My hard requirement is that geometry and scrolling information between the Scrollbars and ScrollableAreas be passed through an interface so that it's easy to delegate scrollbars from one ScrollableArea to another. i.e. It should be easy for RFV to implement an interface, tell VisualViewport and FrameView not to create scrollbars, and create a ScrollbarManager that handles everything else. Having virtual Create/Destroy methods for the Scrollbar* doesn't get in the way of that, but I also don't see why we need to have that boundary.
On 2017/06/27 22:27:56, szager1 wrote: > On 2017/06/27 22:26:13, skobes wrote: > > Maybe ScrollbarManager can fulfill its original purpose in platform, but it > > sounds like we want to expand the scope of its responsibilities. Scrollbar > > management is closely connected to layout in general, so we are going to have > > some code for it in core no matter what. The question is whether to keep it > in > > FrameView/PLSA, or move it into ScrollbarManager. If we're moving it into > > ScrollbarManager then ScrollbarManager needs to be in core. > > Aside from Scrollbar creation/destruction, what core/ functionality do we want > ScrollbarManager to assume? State management. i.e. when the scroll position or geometry in a ScrollableArea changes, it notifies the manager which then updates the scrollbars. All scrollbar event handling today is done in Blink so that also needs to stay in core/. There's a bunch of bugs around scrollbars in the RFV today because it's not easy to decouple the scrollbars from the main FrameView (i.e. we can't really apply the pinch-zoom transformation so input handling is busted when zoomed).
On 2017/06/27 22:33:34, bokan wrote: > On 2017/06/27 21:55:16, szager1 wrote: > > On 2017/06/27 21:16:22, bokan wrote: > > > On 2017/06/27 21:08:52, szager1 wrote: > > > > On 2017/06/27 20:59:05, bokan wrote: > > > > > On 2017/06/27 20:42:13, szager1 wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > > > > File third_party/WebKit/Source/core/paint/ScrollbarManager.cpp > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2942163002/diff/140001/third_party/WebKit/Sou... > > > > > > third_party/WebKit/Source/core/paint/ScrollbarManager.cpp:36: > Scrollbar* > > > > > > ScrollbarManager::CreateScrollbar(ScrollbarOrientation orientation) { > > > > > > All of this logic could be moved into PLSA and FrameView, to allow > > moving > > > > > > ScrollbarManager into platform. > > > > > > > > > > The whole point here is to move the logic out of these classes so > > scrollbars > > > > are > > > > > not so tightly coupled to their container and to break up these > monolithic > > > > > classes. This will allow scrollbars to be agnostic about what they're > tied > > > to, > > > > > as long as it can provide them with style and geometry/scroll > information. > > > > > > > > > > I agree core/layout is probably a better place for it though. > > > > > > > > I guess I'm late to the party here. There's nothing wrong with this CL, > so > > I > > > > won't hold it up, but I do think we're missing an opportunity to do > > something > > > > better. > > > > > > Looks like our messages crossed. There's no rush and this work is just being > > > restarted so we might as well come to an agreed direction before we set off. > > > > > > What benefit do we get from putting ScrollbarManager into platform? My > > > understanding is that platform/ should be for wrapping things that the > > > underlying OS is providing us, ScrollbarManager doesn't really fit in here > > > (neither does ScrollableArea IMO but I think we need it to interact with Mac > > OS > > > scrolling). To me this seems like an added layer of indirection without much > > > benefit but maybe I'm missing something. > > > > > > From my POV, I'd like ScrollableArea to know as little as possible about > > > scrollbars. Eventually, for overlay scrollbars like in Android/CrOS - Blink > > > wouldn't even know about any scrollbars and they could be a detail > implemented > > > fully in CC (for non-overlay scrollbars Blink would just need to know how > much > > > gutter to add during layout). Keeping everything self-contained and > > > communicating through specific interfaces gives us more flexibility and > would > > be > > > cleaner long term IMO. > > > > I don't think that's the meaning of "platform" in Source/platform. There's > lots > > of > > non-OS-related stuff in there. I think of "platform" as stuff that could > exist > > independent of the web/ layer. By that definition, it would be completely > > appropriate to put ScrollbarManager in platform/. > > I think its meaning used to be somewhat convoluted but my read of Blink Onion > Soup > (https://docs.google.com/document/d/13XsbaBz7A2H0PZIdFcytHf5-fVOlAfkLlIUKhxKzs...) > was that > going forward we should treat platform as a system abstraction layer. Ideally > we'd move most of ScrollableArea too into Blink and have a well-defined, tight > interface solely for communicating with Mac's ScrollAnimator which I think is > the main reason it's still in platform/ today. > > > There will always be a platform/core boundary *somewhere* in the scrolling > code. > > Can you explain why that's the case? Scrolling is/should-be entirely internal to > Blink and CC (and my plan for scroll unification is to move most of it to CC). > We have some hooks from/to Mac code today that go through platform/ but I think > they're a big source of pain for us. I'd love if we could think of a way of > making Mac less intrusive but I don't have any answers there yet. That said, I > don't think the Mac integration is really related to lifetime in any way so I > don't see why platform/ needs to be involved in at all. I think the Mac theme stuff *is* related to lifetime. When a new Scrollbar is created, the mac theme code creates a corresponding native scrollbar object and ties the two objects together. It was a bug in that machinery that first led me to create PLSA::ScrollbarManager. I can't see moving that code into core/. > I agree with Steve that I find the platform/ boundary to be artificial and a > hindrance. While I'd prefer the creation/destruction logic to be separate from > ScrollableArea, that's from an aesthetic and design perspective. My hard > requirement is that geometry and scrolling information between the Scrollbars > and ScrollableAreas be passed through an interface so that it's easy to delegate > scrollbars from one ScrollableArea to another. i.e. It should be easy for RFV to > implement an interface, tell VisualViewport and FrameView not to create > scrollbars, and create a ScrollbarManager that handles everything else. Having > virtual Create/Destroy methods for the Scrollbar* doesn't get in the way of > that, but I also don't see why we need to have that boundary. I'm not going to defend ScrollableArea in platform/, I think that's clunky and difficult. But I think that ScrollableArea, Scrollbar, and ScrollbarManager should all live in the same area, be it platform/scroll, core/layout/scroll/, modules/scroll, or what have you.
On 2017/06/27 22:38:27, bokan wrote: > On 2017/06/27 22:27:56, szager1 wrote: > > On 2017/06/27 22:26:13, skobes wrote: > > > Maybe ScrollbarManager can fulfill its original purpose in platform, but it > > > sounds like we want to expand the scope of its responsibilities. Scrollbar > > > management is closely connected to layout in general, so we are going to > have > > > some code for it in core no matter what. The question is whether to keep it > > in > > > FrameView/PLSA, or move it into ScrollbarManager. If we're moving it into > > > ScrollbarManager then ScrollbarManager needs to be in core. > > > > Aside from Scrollbar creation/destruction, what core/ functionality do we want > > ScrollbarManager to assume? > > State management. i.e. when the scroll position or geometry in a ScrollableArea > changes, it notifies the manager which then updates the scrollbars. All > scrollbar event handling today is done in Blink so that also needs to stay in > core/. There's a bunch of bugs around scrollbars in the RFV today because it's > not easy to decouple the scrollbars from the main FrameView (i.e. we can't > really apply the pinch-zoom transformation so input handling is busted when > zoomed). I don't know this stuff enough to comment on it. Would this really be worse with ScrollbarManager in platform/?
On 2017/06/28 00:26:35, szager1 wrote: > On 2017/06/27 22:38:27, bokan wrote: > > On 2017/06/27 22:27:56, szager1 wrote: > > > On 2017/06/27 22:26:13, skobes wrote: > > > > Maybe ScrollbarManager can fulfill its original purpose in platform, but > it > > > > sounds like we want to expand the scope of its responsibilities. > Scrollbar > > > > management is closely connected to layout in general, so we are going to > > have > > > > some code for it in core no matter what. The question is whether to keep > it > > > in > > > > FrameView/PLSA, or move it into ScrollbarManager. If we're moving it into > > > > ScrollbarManager then ScrollbarManager needs to be in core. > > > > > > Aside from Scrollbar creation/destruction, what core/ functionality do we > want > > > ScrollbarManager to assume? > > > > State management. i.e. when the scroll position or geometry in a > ScrollableArea > > changes, it notifies the manager which then updates the scrollbars. All > > scrollbar event handling today is done in Blink so that also needs to stay in > > core/. There's a bunch of bugs around scrollbars in the RFV today because it's > > not easy to decouple the scrollbars from the main FrameView (i.e. we can't > > really apply the pinch-zoom transformation so input handling is busted when > > zoomed). > > I don't know this stuff enough to comment on it. Would this really be worse > with ScrollbarManager in platform/? hmm...I'll need to mull it over for a day or so, I'll get back to you. |
