Bug 129438 - Add highDPI support to DumpRenderTree/WebKitTestRunner without the need of reloading the test case.
Summary: Add highDPI support to DumpRenderTree/WebKitTestRunner without the need of re...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on: 129483
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-27 11:02 PST by zalan
Modified: 2014-02-28 13:37 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.67 KB, patch)
2014-02-27 11:21 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (14.60 KB, patch)
2014-02-27 14:25 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (14.56 KB, patch)
2014-02-27 15:12 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (14.59 KB, patch)
2014-02-28 11:32 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2014-02-27 11:02:54 PST
ssia
Comment 1 zalan 2014-02-27 11:21:47 PST
Created attachment 225397 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-02-27 11:48:48 PST
Comment on attachment 225397 [details]
Patch

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?
Comment 3 zalan 2014-02-27 14:25:59 PST
Created attachment 225409 [details]
Patch
Comment 4 zalan 2014-02-27 14:27:31 PST
(In reply to comment #2)
> (From update of attachment 225397 [details])
> 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.
done.

> 
> > 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.
DumpRenderTree/WebKitTestRunner.

> 
> > 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.
> 
done.

> > 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//
done.

> 
> > Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:257
> > +    NSMutableDictionary *notificationUserInfo = [[NSMutableDictionary alloc] initWithCapacity:1];
> 
> RetainPtr?
done.
Comment 5 zalan 2014-02-27 15:12:39 PST
Created attachment 225419 [details]
Patch
Comment 6 WebKit Commit Bot 2014-02-28 07:18:46 PST
Comment on attachment 225419 [details]
Patch

Clearing flags on attachment: 225419

Committed r164859: <http://trac.webkit.org/changeset/164859>
Comment 7 WebKit Commit Bot 2014-02-28 07:18:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Commit Bot 2014-02-28 07:42:56 PST
Re-opened since this is blocked by bug 129483
Comment 9 zalan 2014-02-28 11:32:02 PST
Created attachment 225478 [details]
Patch
Comment 10 zalan 2014-02-28 11:32:32 PST
Comment on attachment 225478 [details]
Patch

EWS testing
Comment 11 WebKit Commit Bot 2014-02-28 13:37:44 PST
Comment on attachment 225478 [details]
Patch

Clearing flags on attachment: 225478

Committed r164882: <http://trac.webkit.org/changeset/164882>
Comment 12 WebKit Commit Bot 2014-02-28 13:37:46 PST
All reviewed patches have been landed.  Closing bug.