[webkit-reviews] review requested: [Bug 20277] implement window.scrollByPages and window.scrollByLines : [Attachment 23055] patch v2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 28 09:00:18 PDT 2008

arno. <arenevier at fdn.fr> has asked  for review:
Bug 20277: implement window.scrollByPages and window.scrollByLines

Attachment 23055: patch v2

------- Additional Comments from arno. <arenevier at fdn.fr>
Thanks for you useful comments. Now, main code is in FrameView, and DOMWindow
just forwards it.

> Why is Qt different here?

because in WebCore/platform/graphics/Font.h, primaryFont method does not seem
to be defined for Qt, so I fear it won't compile on qt.
May be I misunderstood something, and primaryFont is defined otherwise, but I
did not found where.

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

Mozilla seems to be the only implementor of scrollByLines/scrollByPages; opera
and ie do not define those methods. I tried to follow mozilla's implementation:
line height is given by platform, defaults is 12pt.

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

I checked gtk backend, and it calls scrollBy with a fixed (40) number of pixels
when hitting arrow key; it forwards keypress event to underlying gtk widget
when hitting page down key. Windows code looks quite similar (number of pixels
is 15). So, I don't see how to share code.

Problem with current patch is that scrollByLines is not coherent with hitting
arrow key (ie: it does not scroll same number of pixels); also, scrollByPages
is not coherent with hitting page down. If that's important, may be, DOMWindow
could forward directly to platform ScrollView, and each platform would handle
it their own way.

More information about the webkit-reviews mailing list