Bug 35257 - Expose WebFrame::setCanHaveScrollbars() to Chromium
Summary: Expose WebFrame::setCanHaveScrollbars() to Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-22 12:40 PST by Sam Kerner
Modified: 2010-02-24 23:30 PST (History)
3 users (show)

See Also:


Attachments
Patch to add WebFrame::setAllowsScrolling() (6.49 KB, patch)
2010-02-22 12:44 PST, Sam Kerner
no flags Details | Formatted Diff | Diff
Patch to add WebFrame::setAllowsScrolling() (7.14 KB, patch)
2010-02-23 15:33 PST, Sam Kerner
fishd: review-
Details | Formatted Diff | Diff
Address review comments. (4.00 KB, patch)
2010-02-24 10:59 PST, Sam Kerner
fishd: review-
Details | Formatted Diff | Diff
Added link to this issue. (4.06 KB, patch)
2010-02-24 11:14 PST, Sam Kerner
no flags Details | Formatted Diff | Diff
Last patch failed to apply. Regenerated the patch. (4.06 KB, patch)
2010-02-24 20:39 PST, Sam Kerner
no flags Details | Formatted Diff | Diff
Fix style issue (4.06 KB, patch)
2010-02-24 20:45 PST, Sam Kerner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Kerner 2010-02-22 12:40:16 PST
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
Comment 1 Sam Kerner 2010-02-22 12:44:44 PST
Created attachment 49234 [details]
Patch to add WebFrame::setAllowsScrolling()
Comment 2 WebKit Review Bot 2010-02-23 15:19:49 PST
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.
Comment 3 Sam Kerner 2010-02-23 15:33:29 PST
Created attachment 49331 [details]
Patch to add WebFrame::setAllowsScrolling()

This patch removes some tabs in the previous patch.
Comment 4 Darin Fisher (:fishd, Google) 2010-02-23 20:47:16 PST
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.
Comment 5 Sam Kerner 2010-02-24 10:59:53 PST
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
Comment 6 Darin Fisher (:fishd, Google) 2010-02-24 11:07:24 PST
Comment on attachment 49414 [details]
Address review comments.

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 55194)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> +2010-02-24  Sam Kerner  <skerner@chromium.org>
> +
> +        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.
Comment 7 Sam Kerner 2010-02-24 11:14:58 PST
Created attachment 49415 [details]
Added link to this issue.
Comment 8 Darin Fisher (:fishd, Google) 2010-02-24 11:24:19 PST
Please set the review flag to "?" when you are seeking review.  Only the reviewer should set the review flag to "+".  Thanks!
Comment 9 Sam Kerner 2010-02-24 20:39:37 PST
Created attachment 49464 [details]
Last patch failed to apply.  Regenerated the patch.
Comment 10 WebKit Review Bot 2010-02-24 20:42:30 PST
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.
Comment 11 Sam Kerner 2010-02-24 20:45:43 PST
Created attachment 49465 [details]
Fix style issue
Comment 12 Sam Kerner 2010-02-24 21:24:02 PST
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
Comment 13 WebKit Commit Bot 2010-02-24 23:30:21 PST
Comment on attachment 49465 [details]
Fix style issue

Clearing flags on attachment: 49465

Committed r55226: <http://trac.webkit.org/changeset/55226>
Comment 14 WebKit Commit Bot 2010-02-24 23:30:26 PST
All reviewed patches have been landed.  Closing bug.