[Webkit-unassigned] [Bug 20277] implement window.scrollByPages and window.scrollByLines

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20277


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23085|review?(darin at apple.com)    |review+
               Flag|                            |




------- Comment #11 from darin at apple.com  2008-09-16 09:46 PDT -------
(From update of attachment 23085)
+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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.