Bug 70050 – DRT and WTR should have HiDPI testing capabilities

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Beth Dakin]

DRT and WRT should have HiDPI testing capabilities. Patch forthcoming.

[Beth Dakin]

Created attachment 110899 [details]
Patch

[Darin Adler]

Comment on attachment 110899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110899&action=review

> Source/WebKit/mac/WebView/WebView.mm:2703
> +- (CGFloat)_getBackingScaleFactor

Why the word “get” in this method name?

> Tools/DumpRenderTree/LayoutTestController.cpp:2204
> +    // The second argument is a callbac that is called once the backing scale factor has been set.

The word “callback” here is missing its final “k”.

> Tools/DumpRenderTree/LayoutTestController.cpp:2488
> +        { "setBackingScaleFactor", setBackingScaleFactorCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },

How about using a property that you set instead of a function that you call?

> Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:198
> +        if (deviceScaleFactor != 1) {
> +            [view displayRectIgnoringOpacity:[view bounds] inContext:nsContext];
> +            if ([view isTrackingRepaints])
> +                paintRepaintRectOverlay(view, context);

This code could use a why comment.

> Tools/WebKitTestRunner/TestController.cpp:451
> +    // Re-set to the default backing scale factor by setting the custom scale factor to 0.
> +    WKPageSetCustomBackingScaleFactor(m_mainWebView->page(), 0);

It’s typically more elegant to use a separate function rather than a magic value for this sort of thing. But I guess we decided that before.

> Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:164
> +    // Don't use window snapshots for HiDPI tests.
> +    bool testIsHiDPI = WKPageGetBackingScaleFactor(webView->page()) != 1;

This comment says “what” the code does, but the code already says that. What the comment needs to do is to say “why”.

Also, the code would match the comment better if it set windowSnapshot to 0 instead of using a separate boolean that’s tested below. Otherwise, the comment belongs below where testIsHiDPI is used, not here where it says.

> LayoutTests/ChangeLog:8
> +        New HiDPI tests and results. These should be skipped on all non-Lion platforms.

We’d want to run these on iOS too, so all non-Lion seems a little wrong.

> LayoutTests/ChangeLog:9
> +        * fast/scaling-zooming-hidpi: Added.

Maybe this should just be named “hidpi”. I presume it would not be good to put scaling and zooming tests in this directory since they should not be skipped.

[WebKit Review Bot]

Comment on attachment 110899 [details]
Patch

Attachment 110899 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10060323

New failing tests:
fast/scaling-zooming-hidpi/broken-image-icon-hidpi.html
fast/scaling-zooming-hidpi/video-controls-in-hidpi.html
fast/scaling-zooming-hidpi/clip-text-in-hidpi.html
fast/scaling-zooming-hidpi/resize-corner-hidpi.html
fast/scaling-zooming-hidpi/broken-image-with-size-hidpi.html

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 110899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110899&action=review
> 
> > Source/WebKit/mac/WebView/WebView.mm:2703
> > +- (CGFloat)_getBackingScaleFactor
> 
> Why the word “get” in this method name?
> 

"get" is a part of the equivalent WK2 function name (WKPageGetBackingScaleFactor). But I agree it doesn't seem very WebKit 1-ish. I removed the "get."

> > Tools/DumpRenderTree/LayoutTestController.cpp:2204
> > +    // The second argument is a callbac that is called once the backing scale factor has been set.
> 
> The word “callback” here is missing its final “k”.
> 

Oops, thanks! Fixed.

> > Tools/DumpRenderTree/LayoutTestController.cpp:2488
> > +        { "setBackingScaleFactor", setBackingScaleFactorCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
> 
> How about using a property that you set instead of a function that you call?
> 

I don't think I understand this suggestion. Could you elaborate?

> > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:198
> > +        if (deviceScaleFactor != 1) {
> > +            [view displayRectIgnoringOpacity:[view bounds] inContext:nsContext];
> > +            if ([view isTrackingRepaints])
> > +                paintRepaintRectOverlay(view, context);
> 
> This code could use a why comment.
> 

Added one.

> > Tools/WebKitTestRunner/TestController.cpp:451
> > +    // Re-set to the default backing scale factor by setting the custom scale factor to 0.
> > +    WKPageSetCustomBackingScaleFactor(m_mainWebView->page(), 0);
> 
> It’s typically more elegant to use a separate function rather than a magic value for this sort of thing. But I guess we decided that before.
> 

We could definitely revisit this. I remember discussing it before, but I don't remember why we decided that the 0 was okay. I thought we at least wanted a constant for 0? Maybe I just forgot to do that…

> > Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:164
> > +    // Don't use window snapshots for HiDPI tests.
> > +    bool testIsHiDPI = WKPageGetBackingScaleFactor(webView->page()) != 1;
> 
> This comment says “what” the code does, but the code already says that. What the comment needs to do is to say “why”.
> 
> Also, the code would match the comment better if it set windowSnapshot to 0 instead of using a separate boolean that’s tested below. Otherwise, the comment belongs below where testIsHiDPI is used, not here where it says.
> 

Adjusted. Thanks.

> > LayoutTests/ChangeLog:8
> > +        New HiDPI tests and results. These should be skipped on all non-Lion platforms.
> 
> We’d want to run these on iOS too, so all non-Lion seems a little wrong.
> 

Fixed wording.

> > LayoutTests/ChangeLog:9
> > +        * fast/scaling-zooming-hidpi: Added.
> 
> Maybe this should just be named “hidpi”. I presume it would not be good to put scaling and zooming tests in this directory since they should not be skipped.

True. Re-named.

Committed changes with revision 97407.