Summary: | Expose WebFrame::setCanHaveScrollbars() to Chromium | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Kerner <skerner> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, fishd, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Sam Kerner
2010-02-22 12:40:16 PST
Created attachment 49234 [details]
Patch to add WebFrame::setAllowsScrolling()
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.
Created attachment 49331 [details]
Patch to add WebFrame::setAllowsScrolling()
This patch removes some tabs in the previous patch.
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. 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 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. Created attachment 49415 [details]
Added link to this issue.
Please set the review flag to "?" when you are seeking review. Only the reviewer should set the review flag to "+". Thanks! Created attachment 49464 [details]
Last patch failed to apply. Regenerated the patch.
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.
Created attachment 49465 [details]
Fix style issue
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 on attachment 49465 [details] Fix style issue Clearing flags on attachment: 49465 Committed r55226: <http://trac.webkit.org/changeset/55226> All reviewed patches have been landed. Closing bug. |