Bug 102323 – Add Settings to disable custom scrollbars on main frame

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

Add Settings to disable custom scrollbars on main frame

[Tien-Ren Chen]

Created attachment 174332 [details]
Patch

[Alexandre Elias]

Comment on attachment 174332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174332&action=review

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:65
> +#if OS(ANDROID)

The normal approach is to have a separate patch in the Chromium tree doing this, since we try to avoid having #if OS() in WebKit.

[WebKit Review Bot]

Comment on attachment 174332 [details]
Patch

Attachment 174332 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14843421

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Adam Barth]

Comment on attachment 174332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174332&action=review

How do other mobile ports of WebKit solve this problem?

> Source/WebCore/page/FrameView.cpp:512
> +    if (m_frame && m_frame->page() && m_frame->settings()) {

The line below this one implies that m_frame can never be 0 here.

Rather than testing m_frame-page() for 0, we'd usually write this test as:

if (Settings* settings = m_frame->settings()) {
   ...
}

> Source/WebCore/page/Settings.h:657
> +        void setMainFrameCustomScrollbarDisabled(bool disabled) { m_mainFrameCustomScrollbarDisabled = disabled; }
> +        bool mainFrameCustomScrollbarDisabled() const { return m_mainFrameCustomScrollbarDisabled; }

We generally write settings in terms of "enabled" rather than "disabled".

>> Source/WebKit/chromium/src/WebSettingsImpl.cpp:65
>> +#if OS(ANDROID)
> 
> The normal approach is to have a separate patch in the Chromium tree doing this, since we try to avoid having #if OS() in WebKit.

Yeah, we shouldn't have OS(ANDROID) ifdefs in this code.  The reason to have Settings is so they can be configured by the embedder.

[Tien-Ren Chen]

Created attachment 174537 [details]
Patch

[Tien-Ren Chen]

(In reply to comment #4)
> (From update of attachment 174332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174332&action=review
> 
> How do other mobile ports of WebKit solve this problem?

I'm not really sure. As far as I know, iOS Safari doesn't create custom scrollbars on any frames including iframes, but I don't know where does the hack kicks in.

It is debatable whether we should disable custom scrollbar completely or just on frames or just on the main frame. My opinion is that we should divert from standard webkit behavior as little as possible.

> > Source/WebCore/page/FrameView.cpp:512
> > +    if (m_frame && m_frame->page() && m_frame->settings()) {
> 
> The line below this one implies that m_frame can never be 0 here.
> 
> Rather than testing m_frame-page() for 0, we'd usually write this test as:
> 
> if (Settings* settings = m_frame->settings()) {
>    ...
> }

Done

> > Source/WebCore/page/Settings.h:657
> > +        void setMainFrameCustomScrollbarDisabled(bool disabled) { m_mainFrameCustomScrollbarDisabled = disabled; }
> > +        bool mainFrameCustomScrollbarDisabled() const { return m_mainFrameCustomScrollbarDisabled; }
> 
> We generally write settings in terms of "enabled" rather than "disabled".

Let's make it allowCustomScrollbarInMainFrame

> >> Source/WebKit/chromium/src/WebSettingsImpl.cpp:65
> >> +#if OS(ANDROID)
> > 
> > The normal approach is to have a separate patch in the Chromium tree doing this, since we try to avoid having #if OS() in WebKit.
> 
> Yeah, we shouldn't have OS(ANDROID) ifdefs in this code.  The reason to have Settings is so they can be configured by the embedder.

Done

[Adam Barth]

Comment on attachment 174537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174537&action=review

> Source/WebCore/page/FrameView.cpp:513
> +        if (!settings->allowCustomScrollbarInMainFrame() && m_frame->page() && m_frame->page()->mainFrame() == m_frame)

The test for m_frame->page() isn't actually needed because the only way |settings| can be non-0 is if m_frame->page() is non-0, but it's fine to leave the test here as a security blanket.

> Source/WebCore/page/Settings.h:850
> +        bool m_allowCustomScrollbarInMainFrame : 1;

You should add this to Settings.in now that we have fancy-pants auto generation of this file.  :)

[Tien-Ren Chen]

Created attachment 174578 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 174578 [details]
Patch

Clearing flags on attachment: 174578

Committed r134878: <http://trac.webkit.org/changeset/134878>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.