RESOLVED WONTFIX 108541
[chromium-android] disable WebKit-side scrollbar rendering
https://bugs.webkit.org/show_bug.cgi?id=108541
Summary [chromium-android] disable WebKit-side scrollbar rendering
Tien-Ren Chen
Reported 2013-01-31 15:24:58 PST
[chromium-android] disable all scrollbars except for the main frame
Attachments
Patch (2.67 KB, patch)
2013-01-31 15:33 PST, Tien-Ren Chen
no flags
Patch (3.25 KB, patch)
2013-02-01 12:55 PST, Tien-Ren Chen
no flags
Patch (5.54 KB, patch)
2013-02-01 15:40 PST, Tien-Ren Chen
no flags
Patch (2.36 KB, patch)
2013-02-01 17:42 PST, Tien-Ren Chen
no flags
Patch (5.44 KB, patch)
2013-02-05 02:14 PST, Tien-Ren Chen
no flags
Tien-Ren Chen
Comment 1 2013-01-31 15:33:33 PST
Tien-Ren Chen
Comment 2 2013-01-31 15:35:17 PST
This is a WIP. I will add a Settings flag later. Please confirm the concept is correct. Thanks!
James Robinson
Comment 3 2013-01-31 15:46:15 PST
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.
Tien-Ren Chen
Comment 4 2013-01-31 16:15:17 PST
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.
James Robinson
Comment 5 2013-01-31 16:19:22 PST
(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).
Tien-Ren Chen
Comment 6 2013-02-01 12:55:20 PST
Tien-Ren Chen
Comment 7 2013-02-01 12:58:07 PST
How about this hack? There is no other user for ScrollbarControlSize::SmallScrollbar except safari-win to do some weird thing.
James Robinson
Comment 8 2013-02-01 13:13:03 PST
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?
Tien-Ren Chen
Comment 9 2013-02-01 13:22:23 PST
(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.
James Robinson
Comment 10 2013-02-01 13:30:13 PST
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.
Tien-Ren Chen
Comment 11 2013-02-01 13:34:15 PST
(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?
James Robinson
Comment 12 2013-02-01 13:36:52 PST
Can you hide it in ScrollbarThemeChromiumAndroid?
Tien-Ren Chen
Comment 13 2013-02-01 13:39:04 PST
(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.
James Robinson
Comment 14 2013-02-01 14:22:02 PST
(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.
Tien-Ren Chen
Comment 15 2013-02-01 15:40:10 PST
Tien-Ren Chen
Comment 16 2013-02-01 15:41:01 PST
Does this look less hacky now? PTAL, thanks!
James Robinson
Comment 17 2013-02-01 15:46:01 PST
Comment on attachment 186170 [details] Patch Why do you have to modify the cross-platform interfaces?
James Robinson
Comment 18 2013-02-01 16:06:21 PST
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.
Tien-Ren Chen
Comment 19 2013-02-01 17:42:23 PST
James Robinson
Comment 20 2013-02-01 17:45:34 PST
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.
Tien-Ren Chen
Comment 21 2013-02-05 02:14:23 PST
Tien-Ren Chen
Comment 22 2013-02-05 12:59:55 PST
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.
Peter Beverloo
Comment 23 2013-04-08 11:10:24 PDT
Resolving as WontFix given that Chromium moved to Blink.
Note You need to log in before you can comment on or make changes to this bug.