[webkit-reviews] review denied: [Bug 45228] window.scrollBy() scrolls incorrectly when zoomed in/out : [Attachment 119045] Added the Test for scrolling when Scale factor is applied

Wed Dec 28 02:03:37 PST 2011

Julien Chaffraix <jchaffraix at webkit.org> has denied Sneha Bhat
<mail.snehabhat at gmail.com>'s request for review:
Bug 45228: window.scrollBy() scrolls incorrectly when zoomed in/out
https://bugs.webkit.org/show_bug.cgi?id=45228

Attachment 119045: Added the Test for scrolling when Scale factor is applied
https://bugs.webkit.org/attachment.cgi?id=119045&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119045&action=review

The change looks OK to me with some comments. r- mostly because of the test is
too fuzzy and the comments to handle.

> LayoutTests/fast/dom/scroll-scaled-page-test.html:1
> +<html>

Please add a doctype here: <!DOCTYPE html>. The rule of thumb is to add it when
you don't need to explicitly test quirks mode.

>> LayoutTests/fast/dom/scroll-scaled-page-test.html:10
>> +	    if (window.layoutTestController) {
> 
> fast & http tests are preferred to use the test harness code in
LayoutTests/fast/js/resources/.  You should include js-test-pre.js before your
test script and js-test-post.js after your test script, and utilize the test
functions.  See
http://trac.webkit.org/browser/trunk/LayoutTests/fast/xmlhttprequest/xmlhttpreq
uest-responsetype-sync-request.html for an example test that uses this harness
code.

Just to add on that, this is a preference not a requirement. Here the test is
simple enough that I wouldn't be shocked if it did not use it.

> LayoutTests/fast/dom/scroll-scaled-page-test.html:24
> +	       if(offset >= 199)

I would be way better to test explicitly what you are expecting. As Kenneth
pointed out, 400px would make the test pass, though I am not sure it is what
you expect here.

> LayoutTests/fast/dom/scroll-scaled-page-test.html:30
> +	       layoutTestController.notifyDone();

You don't need to call layoutTestController.waitUntilDone / notifyDone. This is
used to prevent DRT from dumping the results after dispatching the 'load' event
as it normally does.

> LayoutTests/fast/dom/zoom-scroll-page-test.html:1
> +<html>

Ditto doctype.

> LayoutTests/fast/dom/zoom-scroll-page-test.html:3
> +    <style type="text/css">

No need to put the type here.

> LayoutTests/fast/dom/zoom-scroll-page-test.html:12
> +	       layoutTestController.waitUntilDone();

Same comment about waitUntilDone.

>> Source/WebCore/ChangeLog:11
>> +	    The relative scroll distance must take the zoom factor into
account.
> 
> It's better to put the description up higher, under the bug title & url.

There is no consensus on where to put your description. It is left to the
contributor's taste, not the reviewer's.

> Source/WebCore/ChangeLog:14
> +	   (WebCore::DOMWindow::scrollBy):

It would be nice to put somewhere in your ChangeLog what you are explaining in
the bug about being consistent with scrollTo. That would help reviewers
understand why it is a good change.

> Source/WebCore/page/DOMWindow.cpp:1441
> +    int zoomedX = static_cast<int>(x * m_frame->pageZoomFactor() *
m_frame->frameScaleFactor());
> +    int zoomedY = static_cast<int>(y * m_frame->pageZoomFactor() *
m_frame->frameScaleFactor());

You are consistent with scrollTo which is great but it would be better to
explicitly state which type of float -> int conversion you are expecting
(rounding, flooring, ...). The best would be to see what other browsers are
doing here and try to match them (including some test cases to differentiate
between the different conversion).