RESOLVED FIXED 35257
Expose WebFrame::setCanHaveScrollbars() to Chromium
https://bugs.webkit.org/show_bug.cgi?id=35257
Summary Expose WebFrame::setCanHaveScrollbars() to Chromium
Sam Kerner
Reported 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
Attachments
Patch to add WebFrame::setAllowsScrolling() (6.49 KB, patch)
2010-02-22 12:44 PST, Sam Kerner
no flags
Patch to add WebFrame::setAllowsScrolling() (7.14 KB, patch)
2010-02-23 15:33 PST, Sam Kerner
fishd: review-
Address review comments. (4.00 KB, patch)
2010-02-24 10:59 PST, Sam Kerner
fishd: review-
Added link to this issue. (4.06 KB, patch)
2010-02-24 11:14 PST, Sam Kerner
no flags
Last patch failed to apply. Regenerated the patch. (4.06 KB, patch)
2010-02-24 20:39 PST, Sam Kerner
no flags
Fix style issue (4.06 KB, patch)
2010-02-24 20:45 PST, Sam Kerner
no flags
Sam Kerner
Comment 1 2010-02-22 12:44:44 PST
Created attachment 49234 [details] Patch to add WebFrame::setAllowsScrolling()
WebKit Review Bot
Comment 2 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.
Sam Kerner
Comment 3 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.
Darin Fisher (:fishd, Google)
Comment 4 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.
Sam Kerner
Comment 5 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
Darin Fisher (:fishd, Google)
Comment 6 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.
Sam Kerner
Comment 7 2010-02-24 11:14:58 PST
Created attachment 49415 [details] Added link to this issue.
Darin Fisher (:fishd, Google)
Comment 8 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!
Sam Kerner
Comment 9 2010-02-24 20:39:37 PST
Created attachment 49464 [details] Last patch failed to apply. Regenerated the patch.
WebKit Review Bot
Comment 10 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.
Sam Kerner
Comment 11 2010-02-24 20:45:43 PST
Created attachment 49465 [details] Fix style issue
Sam Kerner
Comment 12 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
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2010-02-24 23:30:26 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.