Bug 122152 - [mac] 1x emulation on Retina hardware shouldn't use SPI
Summary: [mac] 1x emulation on Retina hardware shouldn't use SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks: 122006
  Show dependency treegraph
 
Reported: 2013-10-01 03:37 PDT by Tim Horton
Modified: 2013-10-01 13:10 PDT (History)
4 users (show)

See Also:


Attachments
patch (4.96 KB, patch)
2013-10-01 11:39 PDT, Tim Horton
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2013-10-01 03:37:38 PDT
There's a NSHighResolutionCapable Info.plist key that is much more official than the SPI we're using. This will un-break some things re: snapshotting down the road.
Comment 1 Radar WebKit Bug Importer 2013-10-01 03:37:48 PDT
<rdar://problem/15119384>
Comment 2 Alexey Proskuryakov 2013-10-01 08:57:29 PDT
Will using it break the few HiDPI tests we have?
Comment 3 Tim Horton 2013-10-01 09:02:39 PDT
(In reply to comment #2)
> Will using it break the few HiDPI tests we have?

It shouldn't; they change the deviceScaleFactor at a purely WebKit level.
Comment 4 Tim Horton 2013-10-01 11:39:56 PDT
Created attachment 213102 [details]
patch
Comment 5 Tim Horton 2013-10-01 11:45:43 PDT
http://trac.webkit.org/changeset/156724
Comment 6 Alexey Proskuryakov 2013-10-01 12:51:26 PDT
Comment on attachment 213102 [details]
patch

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

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:954
> +    NSDictionary *defaults = [[NSDictionary alloc] initWithObjectsAndKeys:[NSNumber numberWithBool:YES], @"AppleMagnifiedMode", nil];

@{@"AppleMagnifiedMode": @YES}

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:955
> +    [[NSUserDefaults standardUserDefaults] registerDefaults:defaults];

Registering defaults is not what we need here. A persistent preference stuck somewhere would override a registered default, and it's inconsistent with what we do in resetDefaultsToConsistentValues(). We just use regular setBool:forKey:.

Is it important to set this default before entering NSApplication? Otherwise, you could just add this to resetDefaultsToConsistentValues().

> Tools/WebKitTestRunner/mac/main.mm:38
> +    NSDictionary *defaults = [[NSDictionary alloc] initWithObjectsAndKeys:[NSNumber numberWithBool:YES], @"AppleMagnifiedMode", nil];
> +    [[NSUserDefaults standardUserDefaults] registerDefaults:defaults];
> +    [defaults release];

Same comment about registerDefaults:. We already had two styles of setting defaults in this function, which was one more than we should have.

Do we only want AppleMagnifiedMode set in UI process?
Comment 7 Tim Horton 2013-10-01 12:59:24 PDT
(In reply to comment #6)
> (From update of attachment 213102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213102&action=review
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:954
> > +    NSDictionary *defaults = [[NSDictionary alloc] initWithObjectsAndKeys:[NSNumber numberWithBool:YES], @"AppleMagnifiedMode", nil];
> 
> @{@"AppleMagnifiedMode": @YES}

I wasn't sure if I could use this here, but Anders said yes, so I will switch it up.

> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:955
> > +    [[NSUserDefaults standardUserDefaults] registerDefaults:defaults];
> 
> Registering defaults is not what we need here. A persistent preference stuck somewhere would override a registered default, and it's inconsistent with what we do in resetDefaultsToConsistentValues(). We just use regular setBool:forKey:.
> 
> Is it important to set this default before entering NSApplication? Otherwise, you could just add this to resetDefaultsToConsistentValues().
> 
> > Tools/WebKitTestRunner/mac/main.mm:38
> > +    NSDictionary *defaults = [[NSDictionary alloc] initWithObjectsAndKeys:[NSNumber numberWithBool:YES], @"AppleMagnifiedMode", nil];
> > +    [[NSUserDefaults standardUserDefaults] registerDefaults:defaults];
> > +    [defaults release];
> 
> Same comment about registerDefaults:. We already had two styles of setting defaults in this function, which was one more than we should have.

Yeah, Dan pointed this out. Apparently setVolatileDomain is the way to go for temporary defaults that override ones up the chain, though I didn't know this at the time. Is there any reason to actually use setBool:forKey: instead of that?

> Do we only want AppleMagnifiedMode set in UI process?

We only need it set in the UI process.
Comment 8 Alexey Proskuryakov 2013-10-01 13:10:50 PDT
> Is there any reason to actually use setBool:forKey: instead of that?

You can only set any volatile domain once, I think (I assume that repeated calls will replace its content).

In fact, I'm not sure if what we are doing between tests in WebKitTestRunner is right.