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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=20277

Attachment 23085: updated patch
https://bugs.webkit.org/attachment.cgi?id=23085&action=edit

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

+#if !PLATFORM(QT)

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;
+#if !PLATFORM(QT)
+    RenderView *renderer = m_frame->contentRenderer();
+    if (renderer) {
+	 RenderStyle* style = renderer->style();
+	 if (style) {
+	     lineSpacing = style->font().primaryFont()->lineSpacing();
+	 }
+    }
+    if (lineSpacing == 0)
+#endif
+	 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.