Bug 57605

Summary: Frame::pageScaleFactor() should not affect getBoundingClientRect() or getClientRects()
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: Layout and RenderingAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, bdakin, eric, sam, simon.fraser, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
darin: review+
Patch with layout test support sam: review+

Description Beth Dakin 2011-03-31 20:17:07 PDT
I don't think that Frame::pageScaleFactor() should affect getBoundingClientRect() or getClientRects(). Patch forthcoming.

<rdar://problem/9194541>
Comment 1 Beth Dakin 2011-03-31 20:26:27 PDT
Created attachment 87822 [details]
Patch
Comment 2 WebKit Review Bot 2011-03-31 20:28:13 PDT
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.
Comment 3 Beth Dakin 2011-03-31 20:29:59 PDT
(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.
Comment 4 Adam Roben (:aroben) 2011-04-01 05:58:33 PDT
(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.
Comment 5 Adam Roben (:aroben) 2011-04-01 05:59:03 PDT
I mean bug 57579.
Comment 6 Simon Fraser (smfr) 2011-04-01 08:08:04 PDT
What about event.clientX/clientY, scroll offets and other things in "client" coordinates?

Is this bug fixing an issue on a live site?
Comment 7 Darin Adler 2011-04-01 10:02:35 PDT
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?
Comment 8 Sam Weinig 2011-04-01 11:09:49 PDT
Do we need to do this for the Range version of these functions?  I think we should test this fix.
Comment 9 Beth Dakin 2011-04-01 15:46:50 PDT
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.
Comment 10 Beth Dakin 2011-04-02 15:37:55 PDT
Committed the WebCore portion with revision 82778. Will commit DRT+WK2+tests momentarily.
Comment 11 Beth Dakin 2011-04-02 16:47:04 PDT
Committed everything else with r82780.
Comment 12 WebKit Review Bot 2011-04-02 17:14:03 PDT
http://trac.webkit.org/changeset/82778 might have broken Leopard Intel Release (Tests)
Comment 13 WebKit Review Bot 2011-04-02 17:28:38 PDT
http://trac.webkit.org/changeset/82780 might have broken SnowLeopard Intel Release (Tests)
Comment 14 Beth Dakin 2011-04-02 17:38:54 PDT
Oops! And revision 82782.