Bug 108541

Summary: [chromium-android] disable WebKit-side scrollbar rendering
Product: WebKit Reporter: Tien-Ren Chen <trchen>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: enne, jamesr, klobag, laszlo.gombos, nduca, peter, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tien-Ren Chen 2013-01-31 15:24:58 PST
[chromium-android] disable all scrollbars except for the main frame
Comment 1 Tien-Ren Chen 2013-01-31 15:33:33 PST
Created attachment 185880 [details]
Patch
Comment 2 Tien-Ren Chen 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!
Comment 3 James Robinson 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.
Comment 4 Tien-Ren Chen 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.
Comment 5 James Robinson 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).
Comment 6 Tien-Ren Chen 2013-02-01 12:55:20 PST
Created attachment 186113 [details]
Patch
Comment 7 Tien-Ren Chen 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.
Comment 8 James Robinson 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?
Comment 9 Tien-Ren Chen 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.
Comment 10 James Robinson 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.
Comment 11 Tien-Ren Chen 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?
Comment 12 James Robinson 2013-02-01 13:36:52 PST
Can you hide it in ScrollbarThemeChromiumAndroid?
Comment 13 Tien-Ren Chen 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.
Comment 14 James Robinson 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.
Comment 15 Tien-Ren Chen 2013-02-01 15:40:10 PST
Created attachment 186170 [details]
Patch
Comment 16 Tien-Ren Chen 2013-02-01 15:41:01 PST
Does this look less hacky now? PTAL, thanks!
Comment 17 James Robinson 2013-02-01 15:46:01 PST
Comment on attachment 186170 [details]
Patch

Why do you have to modify the cross-platform interfaces?
Comment 18 James Robinson 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.
Comment 19 Tien-Ren Chen 2013-02-01 17:42:23 PST
Created attachment 186196 [details]
Patch
Comment 20 James Robinson 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.
Comment 21 Tien-Ren Chen 2013-02-05 02:14:23 PST
Created attachment 186581 [details]
Patch
Comment 22 Tien-Ren Chen 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.
Comment 23 Peter Beverloo 2013-04-08 11:10:24 PDT
Resolving as WontFix given that Chromium moved to Blink.