[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 #22686|review?                     |review-
               Flag|                            |




------- Comment #2 from darin at apple.com  2008-08-21 18:01 PDT -------
(From update of attachment 22686)
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.


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