[webkit-reviews] review denied: [Bug 129438] Add highDPI support to DumpRenderTree/WebKitTestRunner without the need of reloading the test case. : [Attachment 225397] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Zalan Bujtas
<zalan at apple.com>'s request for review:
Bug 129438: Add highDPI support to DumpRenderTree/WebKitTestRunner without the
need of reloading the test case.
https://bugs.webkit.org/show_bug.cgi?id=129438

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225397&action=review


> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1445
> +static void changeOffscreenWindowScaleIfNeeded()

I don't think the "offscreen" here is useful.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1448
> +    bool needsHighDPIOffscreenWindow =
(gTestRunner->testPathOrURL().find("highDPI-") != string::npos);

This should share code with changeOffscreenWindowScaleIfNeeded(). Please don't
do the patch check in two places.

> Tools/WebKitTestRunner/TestInvocation.cpp:132
> +    bool needsHighDPIOffscreenWindow = strstr(pathOrURL, "highDPI-");

I think you should do a case insensitive check. Many will be running on
case-insensitive file systems.

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:256
> +    // Changing the scaling factor on the window does not trigger
NSWindowDidChangeBackingPropertiesNotification. We need to send the
notification off manually.

s/off//

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:257
> +    NSMutableDictionary *notificationUserInfo = [[NSMutableDictionary alloc]
initWithCapacity:1];

RetainPtr?