Bug 57605 – Frame::pageScaleFactor() should not affect getBoundingClientRect() or getClientRects()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

I don't think that Frame::pageScaleFactor() should affect getBoundingClientRect() or getClientRects(). Patch forthcoming.

<rdar://problem/9194541>

[Beth Dakin]

Created attachment 87822 [details]
Patch

[WebKit Review Bot]

Attachment 87822 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

(In reply to comment #2)
> Attachment 87822 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Total errors found: 1 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

What!! I thought I was meeting your crazy demands by putting the but title on a separate line from the bug number. You just never let up, style bot.

[Adam Roben (:aroben)]

(In reply to comment #3)
> (In reply to comment #2)
> > Attachment 87822 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> > 
> > Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> > Total errors found: 1 in 3 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> What!! I thought I was meeting your crazy demands by putting the but title on a separate line from the bug number. You just never let up, style bot.

Stylebot hates "Fix for". But bug 56989 might be your salvation.

[Adam Roben (:aroben)]

I mean bug 57579.

[Simon Fraser (smfr)]

What about event.clientX/clientY, scroll offets and other things in "client" coordinates?

Is this bug fixing an issue on a live site?

[Darin Adler]

Comment on attachment 87822 [details]
Patch

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?

[Sam Weinig]

Do we need to do this for the Range version of these functions?  I think we should test this fix.

[Beth Dakin]

Created attachment 87932 [details]
Patch with layout test support

Thanks for the review, Darin. Sam added DRT support to test this feature (included in this patch), and I have some tests now.

[Beth Dakin]

Committed the WebCore portion with revision 82778. Will commit DRT+WK2+tests momentarily.

[Beth Dakin]

Committed everything else with r82780.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82778 might have broken Leopard Intel Release (Tests)

[WebKit Review Bot]

http://trac.webkit.org/changeset/82780 might have broken SnowLeopard Intel Release (Tests)

[Beth Dakin]

Oops! And revision 82782.