Add Settings to disable custom scrollbars on main frame
Created attachment 174332 [details] Patch
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.
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
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.
Created attachment 174537 [details] Patch
(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
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. :)
Created attachment 174578 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 174578 [details] Patch Clearing flags on attachment: 174578 Committed r134878: <http://trac.webkit.org/changeset/134878>
All reviewed patches have been landed. Closing bug.