[chromium-android] disable all scrollbars except for the main frame
Created attachment 185880 [details] Patch
This is a WIP. I will add a Settings flag later. Please confirm the concept is correct. Thanks!
I don't think we want to tweak hasScrollbar since that has side effects on things that determine scrollability, we just want to control whether it draws or not. It's better to do that by tweaking the scrollbar width to 0.
Tweaking scrollbar width means we'll have to change ScrollbarThemeChromium::scrollbarThickness() and potentially introduce a new value in ScrollbarControlSize. Are you sure destroying the scrollbar will have side effects on scrollability? I was worrying about the same thing but not sure if that's true. An alternative is we create a custom scrollbar (RenderScrollbar) instead.
(In reply to comment #4) > Tweaking scrollbar width means we'll have to change ScrollbarThemeChromium::scrollbarThickness() and potentially introduce a new value in ScrollbarControlSize. > > Are you sure destroying the scrollbar will have side effects on scrollability? I was worrying about the same thing but not sure if that's true. I believe the GTK port tries to avoid scrollbars completely instead of making them 0-width. They've hit and fixed a lot of bugs with this path. It's possible they've fixed all of them, but I wouldn't bet on it. > > An alternative is we create a custom scrollbar (RenderScrollbar) instead. Yuck - let's not go there. Another option would be to have it be there and have normal size but not render anything. We could have the scrollbar theme set up to by default not render anything and then override that in ScrollingCoordinatorChromium for scrollbars the compositor knows about (i.e. today main frame scrollbars, in the future hopefully all compositor-coordinated scrollbars).
Created attachment 186113 [details] Patch
How about this hack? There is no other user for ScrollbarControlSize::SmallScrollbar except safari-win to do some weird thing.
Comment on attachment 186113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186113&action=review I don't understand why you feel the need to do this in such a hacky way. > Source/WebCore/page/FrameView.cpp:526 > +#if PLATFORM(CHROMIUM) && OS(ANDROID) > + if (m_frame = m_frame->page()->mainFrame()) > + return Scrollbar::createNativeScrollbar(this, orientation, RegularScrollbar); > +#endif Doesn't this also break custom scrollbars? Why do you need to do that?
(In reply to comment #8) > (From update of attachment 186113 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186113&action=review > > I don't understand why you feel the need to do this in such a hacky way. Needing the compositor the recognize the main frame scrollbar and pass a magic flag to the theme sounds hacky too... I think these are 2 separate matter: 1. Should we introduce a new magic flag? Or reuse ScrollbarControlSize::SmallScrollbar is fine? 2. Who should be responsible for setting the flag? At creation time? Right after creation? Or the compositor? I feel this is the least intrusive way to do the hack, hopefully we don't need to keep the hack for too long? > > Source/WebCore/page/FrameView.cpp:526 > > +#if PLATFORM(CHROMIUM) && OS(ANDROID) > > + if (m_frame = m_frame->page()->mainFrame()) > > + return Scrollbar::createNativeScrollbar(this, orientation, RegularScrollbar); > > +#endif > > Doesn't this also break custom scrollbars? Why do you need to do that? We disabled custom scrollbar anyway. I can switch the order to make custom scrollbar to work in case we want to enable it.
I think a solution where scrollbars on android by default do not paint but ones that go through ScrollingCoordinatorChromium are recognized would be best, since that's the behavior we want and it'll naturally allow more and more scrollbars as they are hooked up to the ScrollingCoordinator.
(In reply to comment #10) > I think a solution where scrollbars on android by default do not paint but ones that go through ScrollingCoordinatorChromium are recognized would be best, since that's the behavior we want and it'll naturally allow more and more scrollbars as they are hooked up to the ScrollingCoordinator. I see, so perhaps 1. We add a flag ScrollbarThemeClient::isCoordinated() 2. ScrollingCoordinator sets the flag when a Scrollbar get registered 3. ScrollbarThemeAndroid looks at the flag and only paint when it sees a coordinate scrollbar 4. Profit! Sounds reasonable to you?
Can you hide it in ScrollbarThemeChromiumAndroid?
(In reply to comment #12) > Can you hide it in ScrollbarThemeChromiumAndroid? Do you mean setting the width to 0 instead of skip painting? I think the timing is too late though, unless we want to re-layout the scrollbars when registering them.
(In reply to comment #13) > (In reply to comment #12) > > Can you hide it in ScrollbarThemeChromiumAndroid? > > Do you mean setting the width to 0 instead of skip painting? > No just to control the painting > I think the timing is too late though, unless we want to re-layout the scrollbars when registering them.
Created attachment 186170 [details] Patch
Does this look less hacky now? PTAL, thanks!
Comment on attachment 186170 [details] Patch Why do you have to modify the cross-platform interfaces?
I think what we can do is make ScrollbarThemeChromiumAndroid paint nothing and then override our compositor's thumb painting logic on OS(ANDROID) to paint android-looking thumbs. That way we can still use all of the positioning/sizing logic on all scrollbars and still see the thumbs for compositor-handled scrollbars only.
Created attachment 186196 [details] Patch
Comment on attachment 186196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186196&action=review > Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:106 > +#if 0 just delete it, don't #if 0. The code will live on in the revision tracking system when/if we want to revive it > Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:148 > + // FIXME: We're hardcoding scrollbars in the compositor until smoothiness issue getting solved. smoothiness isn't a word, and this doesn't explain very well why we're doing nothing > Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:151 > + (void)context; > + (void)scrollbar; > + (void)rect; The way in WebKit to deal with unused parameters is to not give them a name.
Created attachment 186581 [details] Patch
Comment on attachment 186581 [details] Patch Per discussion in http://code.google.com/p/chromium/issues/detail?id=173302 https://codereview.chromium.org/12147005/ We might not need a quick hack scrollbar after all. I'll be working on a proper sublayer scrollbar coordinator in the compositor.
Resolving as WontFix given that Chromium moved to Blink.