[webkit-reviews] review denied: [Bug 20277] implement window.scrollByPages and window.scrollByLines : [Attachment 22686] window.scrollByLines + window.scrollByPages

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 22686: window.scrollByLines + window.scrollByPages
https://bugs.webkit.org/attachment.cgi?id=22686&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks, this patch looks really good!

+2008-08-06  arno  <arenevier at fdn.fr>

We'd prefer to have your full name here if that's OK.

+		Tests for window.scrollByLines and window.scrollByPages

+		implements window.scrollByPages and window.scrollByLines DOM
level 0
+		methods

These lines have tabs in them. We can't commit patches with tab characters
(Subversion is set to not allow it), so it's best if you give use changes that
don't have them.

 #include "SecurityOrigin.h"
+#include "RenderView.h"
+#include "SimpleFontData.h"

The lists of included files should be kept sorted, so RenderView should be
moved up a few lines.

The DOMWindow class is supposed to simply be the DOM interface -- the actual
scrolling function should be in the FrameView class and DOMWindow should simply
forward the calls there.

+#if !PLATFORM(QT)
+    RenderView *renderer = m_frame->contentRenderer();

Why is Qt different here?

+	     int linespacing = style->font().primaryFont()->lineSpacing();
+	     scrollBy(0, linespacing * y);

You should capitalize your local variable like the function: lineSpacing.

Should we really base the definition of a line on the default font size? Is
there a better technique? Is this what other browsers do? How does this
definition of a page relate to what other browsers do?

Can we make this share code with the code that scrolls a line or a page when
you hit the arrow keys and page up and page down keys?

There are enough issues that I'm going to say review- for now, but this seems
about ready to go.