[webkit-reviews] review granted: [Bug 20277] implement window.scrollByPages and window.scrollByLines : [Attachment 23085] updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 16 09:46:21 PDT 2008

Darin Adler <darin at apple.com> has granted arno. <arenevier at fdn.fr>'s request
for review:
Bug 20277: implement window.scrollByPages and window.scrollByLines

Attachment 23085: updated patch

------- Additional Comments from Darin Adler <darin at apple.com>
+using std::min;

Normally we'd just do "using namespace std".

+    // Line increment is given line height as reported by platform. Fallbacks
+    // to 12pt

This should be "Falls back to 12 pt." rather than "Fallbacks to 12pt." I also
think that there's no good reason to quite this figure in "pt". We should say
"falls back to 16 pixels".

I'd like to say a FIXME that says "this should share the code with the keyboard
bindings that do the same thing".


This #if is in the wrong place. It should be around the entire function except
for the call to scrollBy. It's also horrible that we need this!

+	 doc->updateLayoutIgnorePendingStylesheets();

Why are we doing updateLayout in scrollByLines? Is it that we are trying to
force the renderer be created? If so then we need to get the renderer *after*
calling updateLayoutIgnorePendingStylesheets.

+    int lineSpacing = 0;
+    RenderView *renderer = m_frame->contentRenderer();
+    if (renderer) {
+	 RenderStyle* style = renderer->style();
+	 if (style) {
+	     lineSpacing = style->font().primaryFont()->lineSpacing();
+	 }
+    }
+    if (lineSpacing == 0)
+	 lineSpacing = 16;

Why not just initialize lineSpacing to 16? I don't see the point in setting it
to 0 and then 16.

+    void scrollByLines(int y);
+    void scrollByPages(int y);

I think these arguments would be better off either without names or with
clearer names.

I'm going to say r=me despite these problems. It seems OK to land this.

More information about the webkit-reviews mailing list