[Webkit-unassigned] [Bug 45228] window.scrollBy() scrolls incorrectly when zoomed in/out

Wed Dec 28 02:03:38 PST 2011

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

Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #119045|review?                     |review-
               Flag|                            |

--- Comment #21 from Julien Chaffraix <jchaffraix at webkit.org>  2011-12-28 02:03:37 PST ---
(From update of attachment 119045)
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/xmlhttprequest-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).

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