Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
191293017:
Mac: Fix registration of user defaults. (Closed)
|
Created:
6 years, 9 months ago by Andre Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMac: Fix registration of user defaults.
We should be using -registerDefaults instead of -setObject:forKey:.
Also, we were setting the string @"YES" to NSWindowHostsLayersInWindowServer,
changed it to be the boolean NSNumber @YES instead.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256233
Patch Set 1 #Patch Set 2 : Fix for avi & ccameron. #Messages
Total messages: 21 (0 generated)
This is indeed a better choice, but you dropped the version check for the window hosting part. ccameron, is that important?
On 2014/03/10 21:04:13, Avi wrote: > This is indeed a better choice, but you dropped the version check for the window > hosting part. ccameron, is that important? I'm not sure if does anything <10.9, but if it does, we don't want it.
On 2014/03/10 21:04:13, Avi wrote: > This is indeed a better choice, but you dropped the version check for the window > hosting part. ccameron, is that important? Right, now that I think about it some more, it is possible that 10.8 reads this value but defaults it to NO, while 10.9 defaults it to YES. I'm not sure though. So I guess the question is if 10.8 supports it but defaults it to NO, do we want it on?
On 2014/03/10 21:07:02, ccameron1 wrote:
> On 2014/03/10 21:04:13, Avi wrote:
> > This is indeed a better choice, but you dropped the version check for the
> window
> > hosting part. ccameron, is that important?
>
> I'm not sure if does anything <10.9, but if it does, we don't want it.
I see, so should I change it like so?
@"NSWindowHostsLayersInWindowServer":
@(base::mac::IsOSMavericksOrLater()),
On 2014/03/10 21:09:32, Andre wrote: > On 2014/03/10 21:07:02, ccameron1 wrote: > > On 2014/03/10 21:04:13, Avi wrote: > > > This is indeed a better choice, but you dropped the version check for the > > window > > > hosting part. ccameron, is that important? > > > > I'm not sure if does anything <10.9, but if it does, we don't want it. > > I see, so should I change it like so? > @"NSWindowHostsLayersInWindowServer": > @(base::mac::IsOSMavericksOrLater()), 10.8 actually has -[NSWindow _setHostsLayersInWindowServer:] but it's not connected to that pref key. (That link showed up in 10.9.) You propose a change that is _still_ different than your original code. If you want to deliberately change the behavior, fine, but acknowledge that you're doing so.
I'm good with explicitly setting it to NO for <10.9 and YES for >=10.9 (with the slight behavior change).
On 2014/03/10 22:24:09, ccameron1 wrote: > I'm good with explicitly setting it to NO for <10.9 and YES for >=10.9 (with the > slight behavior change). Uploaded new patch with this change. The behavior change is that the value used to be unset for < 10.9, now it is set to @NO.
Sweet. LGTM
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/191293017/20001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
lgtm stamp
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/191293017/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/191293017/20001
Message was sent while issue was closed.
Change committed as 256233 |
