[chromium-android] disable WebKit-side scrollbar rendering

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Tien-Ren Chen]

[chromium-android] disable all scrollbars except for the main frame

[Tien-Ren Chen]

Created attachment 185880 [details]
Patch

[Tien-Ren Chen]

This is a WIP. I will add a Settings flag later. Please confirm the concept is correct. Thanks!

[James Robinson]

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]

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]

(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]

Created attachment 186113 [details]
Patch

[Tien-Ren Chen]

How about this hack? There is no other user for ScrollbarControlSize::SmallScrollbar except safari-win to do some weird thing.

[James Robinson]

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]

(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]

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]

(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]

Can you hide it in ScrollbarThemeChromiumAndroid?

[Tien-Ren Chen]

(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]

(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]

Created attachment 186170 [details]
Patch

[Tien-Ren Chen]

Does this look less hacky now? PTAL, thanks!

[James Robinson]

Comment on attachment 186170 [details]
Patch

Why do you have to modify the cross-platform interfaces?

[James Robinson]

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]

Created attachment 186196 [details]
Patch

[James Robinson]

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]

Created attachment 186581 [details]
Patch

[Tien-Ren Chen]

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]

Resolving as WontFix given that Chromium moved to Blink.