[webkit-reviews] review canceled: [Bug 64883] Zoom only scales frames' content, not the frames themselves. : [Attachment 142632] Updated Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 21 10:40:43 PDT 2012


Julien Chaffraix <jchaffraix at webkit.org> has canceled Uday Kiran
<udaykiran at motorola.com>'s request for review:
Bug 64883: Zoom only scales frames' content, not the frames themselves.
https://bugs.webkit.org/show_bug.cgi?id=64883

Attachment 142632: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=142632&action=review

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


I would like to see another round.

> Source/WebCore/rendering/RenderFrameSet.cpp:218
> +	       gridLayout[i] = max(static_cast<int>(lroundf(grid[i].value() *
style()->effectiveZoom())), 0);

no static_cast please. If it's needed to compile, it's better to specify which
specialization of max you are using:

gridLayout[i] = max<int>(lroundf(grid[i].value() * style()->effectiveZoom()),
0);

> LayoutTests/fast/frames/frame-set-zoom-page.html:5
> +    function init() {

It's not really an initialization (let's not abbreviate), more than the test
itself. Better name: zoomIn, testZooming, ...

> LayoutTests/fast/frames/frame-set-zoom-page.html:17
> +  <!--
> +  Test case for https://bugs.webkit.org/show_bug.cgi?id=64883. Zoom only
scales frames' content, not the frames themselves.
> +  This test passes if there is horizontal frame of 60px height at the top
and below it a vertical frame of width 60px to left.
> +  -->

This should be dumped into the ouput as it's useful for anyone looking at the
test. The expected file will need to be updated accordingly.

Please add somewhere how to test the page manually.


More information about the webkit-reviews mailing list