[webkit-reviews] review granted: [Bug 57605] Frame::pageScaleFactor() should not affect getBoundingClientRect() or getClientRects() : [Attachment 87822] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 57605: Frame::pageScaleFactor() should not affect getBoundingClientRect()
or getClientRects()
https://bugs.webkit.org/show_bug.cgi?id=57605

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87822&action=review

> Source/WebCore/dom/Element.cpp:550
> +    float pageScale = 1;
> +    if (Page* page = document()->page()) {
> +	   if (Frame* frame = page->mainFrame())
> +	       pageScale = frame->pageScaleFactor();
> +    }

Seems strange to go from document to page and then to frame. You can go
directly from a document to a frame. But I guess that subframes always have a
scale factor of 1. This is not a great design. Page-wide settings should be on
the page rather than the main frame. Or if they need to be on frames they
should be on all frames.

The fact that you can’t get the scale factor without going to the main frame is
not good. We should fix that later. This entire paragraph of code should be
simple. I don’t want to have to repeat this dance any time we need to get the
scale factor.

Not really about this patch, though.

> Source/WebCore/dom/Element.cpp:601
> +    if (Page* page = document()->page()) {
> +	   if (Frame* frame = page->mainFrame())
> +	       adjustFloatRectForPageScale(result, frame->pageScaleFactor());
> +    }

There it is, the same dance again. Yuck.

> Source/WebCore/rendering/RenderObject.h:1085
> +    if (pageScale == 1)
> +	   return point;

Why the early exit for 1 here, but not in the quad or rect versions?