Bug 35257 – Expose WebFrame::setCanHaveScrollbars() to Chromium

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sam Kerner]

Some Chromium windows are sized to fit the content exactly.  Browser Action popups are one example.  If that content's size increases (for example, if it gets taller because some javascript added a new DOM element), the window will be resized to fit.  As the window is being resized, scrollbars should not be drawn in the window.  They flicker into existence as content grows (and the window size becomes too small), and disappear when the window's size becomes large enough.  To prevent this, Chromium needs a way to tell WebKit not to draw scroll bars on a WebFrame.  The attached change exposes the required function.

Sam

[Sam Kerner]

Created attachment 49234 [details]
Patch to add WebFrame::setAllowsScrolling()

[WebKit Review Bot]

Attachment 49234 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 8 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Kerner]

Created attachment 49331 [details]
Patch to add WebFrame::setAllowsScrolling()

This patch removes some tabs in the previous patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49331 [details]
Patch to add WebFrame::setAllowsScrolling()

> 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.

[Sam Kerner]

Created attachment 49414 [details]
Address review comments.

All comments addressed, ready for another look.

> > +        Existing function setAllowsScrolling() was renamed
> > +        setCanHaveScrollbars(), to be consistant with change 37159.
> > +
> 
> ^^^ please include a link to this bug.

I was unable to find a bug for this change, so I added a link to the change itself: http://trac.webkit.org/changeset/37159

[Darin Fisher (:fishd, Google)]

Comment on attachment 49414 [details]
Address review comments.

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 55194)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> [phone]  Sam Kerner  <[e-mail]>
> +
> +        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:
> +        http://trac.webkit.org/changeset/37159

I meant that you should include a link to this bug report:
https://bugs.webkit.org/show_bug.cgi?id=35257

That way when someone reads the ChangeLog they can jump back to this
bug report, where the code review and related discussions took place.

[Sam Kerner]

Created attachment 49415 [details]
Added link to this issue.

[Darin Fisher (:fishd, Google)]

Please set the review flag to "?" when you are seeking review.  Only the reviewer should set the review flag to "+".  Thanks!

[Sam Kerner]

Created attachment 49464 [details]
Last patch failed to apply.  Regenerated the patch.

[WebKit Review Bot]

Attachment 49464 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Kerner]

Created attachment 49465 [details]
Fix style issue

[Sam Kerner]

Darin,
    The current patch is the same as the last one you looked at ("Added link to this issue"), but it fixes a patch formatting issue that caused the build bots to choke.

Sam

[WebKit Commit Bot]

Comment on attachment 49465 [details]
Fix style issue

Clearing flags on attachment: 49465

Committed r55226: <http://trac.webkit.org/changeset/55226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.