[webkit-reviews] review denied: [Bug 35257] Expose WebFrame::setCanHaveScrollbars() to Chromium : [Attachment 49331] Patch to add WebFrame::setAllowsScrolling()

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Sam Kerner
<skerner at chromium.org>'s request for review:
Bug 35257: Expose WebFrame::setCanHaveScrollbars() to Chromium
https://bugs.webkit.org/show_bug.cgi?id=35257

Attachment 49331: Patch to add WebFrame::setAllowsScrolling()
https://bugs.webkit.org/attachment.cgi?id=49331&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: ChangeLog

> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Expose WebFrame::setCanHaveScrollbars().  This allows a view
> +	   which is being resized to not need scroll bars to ensure that
> +	   they are not drawn.
> +
> +	   Existing function setAllowsScrolling() was renamed
> +	   setCanHaveScrollbars(), to be consistant with change 37159.
> +

^^^ please include a link to this bug.


> +	   * WebKit/chromium/public/WebFrame.h:
> +	     Expose setCanHaveScrollbars().
> +
> +	   * WebKit/chromium/src/ChromeClientImpl.cpp:
> +	     Rename setAllowsScrolling() to setCanHaveScrollbars()
> +
> +	   * WebKit/chromium/src/WebFrameImpl.cpp:
> +	     Rename setAllowsScrolling() to setCanHaveScrollbars()
> +
> +	   * WebKit/chromium/src/WebFrameImpl.h:
> +	     Rename setAllowsScrolling() to setCanHaveScrollbars()

^^^ this doesn't look like it is following the template produced
by prepare-ChangeLog


>	   Add code that enables SquirrelFish Extreme (a.k.a JSCX, JSC JIT)
> -	   in Android. It's disabled by default, but is enabled when the 
> +	   in Android. It's disabled by default, but is enabled when the
>	   enveronment variable ENABLE_JSC_JIT is set to true.
>	   https://bugs.webkit.org/show_bug.cgi?id=34855
>  
> @@ -279,11 +302,11 @@
>	   Reviewed by Simon Hausmann.
>  
>	   First steps of the QtScript API.
> -	   
> +
>	   Two new classes were created; QScriptEngine and QScriptValue.
>	   The first should encapsulate a javascript context and the second a
script
>	   value.
> -	   
> +
>	   This API is still in development, so it isn't compiled by default.
>	   To trigger compilation, pass --qmakearg="CONFIG+=build-qtscript" to
>	   build-webkit.
> @@ -655,7 +678,7 @@
>	   Move GOwnPtr* from wtf to wtf/gtk
>	   https://bugs.webkit.org/show_bug.cgi?id=31793
>  
> -	   * GNUmakefile.am: Add JavaScriptCore/wtf/gtk to 
> +	   * GNUmakefile.am: Add JavaScriptCore/wtf/gtk to
>	     the include path.

^^^ I think it is best to leave existing entries in the ChangeLog unmodified.


> Index: WebKit/chromium/public/WebFrame.h
> ===================================================================
> --- WebKit/chromium/public/WebFrame.h (revision 55153)
> +++ WebKit/chromium/public/WebFrame.h (working copy)
> @@ -492,6 +492,9 @@ public:
>					    float pageWidthInPixels,
>					    float pageHeightInPixels) const =
0;
>  
> +    // If set to false, do not draw scrollbars on this frame's view.
> +    virtual void setCanHaveScrollbars(bool canHaveScrollbars) = 0;

Please move this to the "Geometry" section of WebFrame.  I would put this
method
just above the scrollOffset getter.  Also, the parameter does not require a
name
since it is clear that this method is setting a boolean attribute.


> +++ WebKit/chromium/src/WebFrameImpl.h	(working copy)

> -    void setAllowsScrolling(bool);
> +    void setCanHaveScrollbars(bool canHaveScrollbars);

^^^ ditto, do not name the parameter.