[webkit-reviews] review denied: [Bug 102323] Add Settings to disable custom scrollbars on main frame : [Attachment 174332] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tien-Ren Chen <trchen at chromium.org>'s
request for review:
Bug 102323: Add Settings to disable custom scrollbars on main frame
https://bugs.webkit.org/show_bug.cgi?id=102323

Attachment 174332: Patch
https://bugs.webkit.org/attachment.cgi?id=174332&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.