[webkit-reviews] review granted: [Bug 70050] DRT and WRT should have HiDPI testing capabilities : [Attachment 110899] 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 70050: DRT and WRT should have HiDPI testing capabilities
https://bugs.webkit.org/show_bug.cgi?id=70050

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.