Mac/clang: Possibly contentious changes.

Mac/clang: Possibly contentious changes.

As discussed on irc, we're getting rid of const NSObjects, and we're making all properties nonatomic.

const: All of cocoa takes nonconst NSObjects, and passing a const NSString to a function that takes NSString is a const violation. gcc doesn't complain about this, but clang intentionally does. constness is usually done via immutable base classes and mutable subclasses in cocoa anyway, so getting rid of const isn't that bad. Also, we don't really have a choice.

nonatomic: Some of our classes have custom setters that are not @synchronized. If the @property for that is not non-atomic, then the interface claims that the method is synchronized but the implementation actually isn't. That's a bug. gcc happens not to complain about this, but clang does. Since we shouldn't need atomic properties anywhere, the simple rule is now to just make all properties nonatomic.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49808

[Mark Mentovai, 2010-06-12 00:04:36]

http://codereview.chromium.org/2769014/diff/11001/5002
File app/clipboard/clipboard_mac.mm (right):

http://codereview.chromium.org/2769014/diff/11001/5002#newcode22
app/clipboard/clipboard_mac.mm:22: NSString* kUTTypeURLName =
@"public.url-name";
You could still make all of these |NSString* const| so that people wouldn’t be
able to come along and write bad code like |kUTTypeURLName = @"bogus";|. This is
already how things are in keystone_glue.mm.

We can do this with our own “constants.” We still can’t prevent anyone from
writing |NSWindowWillMoveNotification = NSWindowWillCloseNotification;|. Oh
well.

http://codereview.chromium.org/2769014/diff/11001/5002#newcode26
app/clipboard/clipboard_mac.mm:26: NSString *kWebSmartPastePboardType = @"NeXT
smart paste pasteboard type";
Fix the star so that it’s consistent with the other declarations in this file.

http://codereview.chromium.org/2769014/diff/11001/5009
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/2769014/diff/11001/5009#newcode52
chrome/browser/cocoa/bookmark_editor_base_controller.h:52: @property (nonatomic,
retain) NSArray* folderTreeArray;
The try bot says:

/b/slave/mac/build/src/chrome/browser/cocoa/bookmark_editor_base_controller.mm:23:
error: use class continuation to override 'readonly' property 'folderTreeArray'
with 'readwrite' in class 'BookmarkEditorBaseController'

http://codereview.chromium.org/2769014/diff/11001/5018
File chrome/browser/cocoa/hover_close_button.mm (right):

http://codereview.chromium.org/2769014/diff/11001/5018#newcode17
chrome/browser/cocoa/hover_close_button.mm:17: }
Lemme hear a “}  // namespace” while you’re in here.

http://codereview.chromium.org/2769014/diff/11001/5020
File chrome/browser/cocoa/keystone_glue.h (right):

http://codereview.chromium.org/2769014/diff/11001/5020#newcode47
chrome/browser/cocoa/keystone_glue.h:47: enum BrandFileType {
And while you’re doing this one, fix the indentation: things don’t get indented
inside a namespace. (But you can leave a blank line between “namespace” and
“enum.”)

http://codereview.chromium.org/2769014/diff/11001/5026
File chrome/renderer/render_process_impl.cc (right):

http://codereview.chromium.org/2769014/diff/11001/5026#newcode288
chrome/renderer/render_process_impl.cc:288: return NULL;
Uncontentious!

[Mark Mentovai, 2010-06-12 00:06:52]

I haven’t looked at any of the properties to see if they need to be
atomic/@synchronized, but as we discussed (and as you captured in the changelist
description), I’d be incredibly surprised if we had anything that couldn’t be
nonatomic.

[Nico, 2010-06-15 16:01:05]

http://codereview.chromium.org/2769014/diff/11001/5002
File app/clipboard/clipboard_mac.mm (right):

http://codereview.chromium.org/2769014/diff/11001/5002#newcode22
app/clipboard/clipboard_mac.mm:22: NSString* kUTTypeURLName =
@"public.url-name";
On 2010/06/12 00:04:36, Mark Mentovai wrote:
> You could still make all of these |NSString* const| so that people wouldn’t be
> able to come along and write bad code like |kUTTypeURLName = @"bogus";|. This
is
> already how things are in keystone_glue.mm.
> 
> We can do this with our own “constants.” We still can’t prevent anyone from
> writing |NSWindowWillMoveNotification = NSWindowWillCloseNotification;|. Oh
> well.

Done.

http://codereview.chromium.org/2769014/diff/11001/5002#newcode26
app/clipboard/clipboard_mac.mm:26: NSString *kWebSmartPastePboardType = @"NeXT
smart paste pasteboard type";
On 2010/06/12 00:04:36, Mark Mentovai wrote:
> Fix the star so that it’s consistent with the other declarations in this file.

Done.

http://codereview.chromium.org/2769014/diff/11001/5009
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/2769014/diff/11001/5009#newcode52
chrome/browser/cocoa/bookmark_editor_base_controller.h:52: @property (nonatomic,
retain) NSArray* folderTreeArray;
On 2010/06/12 00:04:36, Mark Mentovai wrote:
> The try bot says:
> 
>
/b/slave/mac/build/src/chrome/browser/cocoa/bookmark_editor_base_controller.mm:23:
> error: use class continuation to override 'readonly' property
'folderTreeArray'
> with 'readwrite' in class 'BookmarkEditorBaseController'

I saw that. I don't get that error locally. I guess I'll just add back the
"readonly" and be done with it :-/

http://codereview.chromium.org/2769014/diff/11001/5018
File chrome/browser/cocoa/hover_close_button.mm (right):

http://codereview.chromium.org/2769014/diff/11001/5018#newcode17
chrome/browser/cocoa/hover_close_button.mm:17: }
On 2010/06/12 00:04:36, Mark Mentovai wrote:
> Lemme hear a “}  // namespace” while you’re in here.

Done.

http://codereview.chromium.org/2769014/diff/11001/5020
File chrome/browser/cocoa/keystone_glue.h (right):

http://codereview.chromium.org/2769014/diff/11001/5020#newcode47
chrome/browser/cocoa/keystone_glue.h:47: enum BrandFileType {
On 2010/06/12 00:04:36, Mark Mentovai wrote:
> And while you’re doing this one, fix the indentation: things don’t get
indented
> inside a namespace. (But you can leave a blank line between “namespace” and
> “enum.”)

Done.

[Mark Mentovai, 2010-06-15 16:42:09]

LG with these changes.

http://codereview.chromium.org/2769014/diff/17001/18004
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18004#newcode88
chrome/browser/chrome_browser_application_mac.mm:88: static NSString*
kKnownNSExceptionNames[] = {
  static NSString* const kKnownNSExceptionNames[] = {

http://codereview.chromium.org/2769014/diff/17001/18010
File chrome/browser/cocoa/bug_report_window_controller.h (right):

http://codereview.chromium.org/2769014/diff/17001/18010#newcode112
chrome/browser/cocoa/bug_report_window_controller.h:112: 
Kill the blank line at the end of the file while you’re in here.

http://codereview.chromium.org/2769014/diff/17001/18021
File chrome/browser/cocoa/keystone_glue.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18021#newcode60
chrome/browser/cocoa/keystone_glue.mm:60: @"CheckSuccessful";
Looks like you had room for this on the previous line.

http://codereview.chromium.org/2769014/diff/17001/18021#newcode61
chrome/browser/cocoa/keystone_glue.mm:61: NSString* const
KSUpdateCheckSuccessfullyInstalledKey = @"SuccessfullyInstalled";
On the other hand, this line is 81 characters long.

http://codereview.chromium.org/2769014/diff/17001/18021#newcode181
chrome/browser/cocoa/keystone_glue.mm:181: @"AutoupdateStatusNotification";
There’s room for this one, though.

http://codereview.chromium.org/2769014/diff/17001/18024
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18024#newcode35
chrome/browser/cocoa/status_bubble_mac.mm:35: NSString* kFadeAnimationKey =
@"alphaValue";
NSString* const?

http://codereview.chromium.org/2769014/diff/17001/18025
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18025#newcode249
chrome/common/sandbox_mac.mm:249: NSString* const kCommonSandboxPrefixFileName =
@"common";
static NSString* const

But this doesn’t need to be a variable at all, and it doesn’t need to be at the
top of the function. It’s only used once below, and writing @"common" inline
would be fine there. If you feel strongly about the variable, at the very least,
you should move the declaration down to where it’s used.

[Nico, 2010-06-15 16:49:44]

I forgot quite a few @properties. Your opportunity to say "while you're here" 50
more times!

http://codereview.chromium.org/2769014/diff/17001/18004
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18004#newcode88
chrome/browser/chrome_browser_application_mac.mm:88: static NSString*
kKnownNSExceptionNames[] = {
On 2010/06/15 16:42:09, Mark Mentovai wrote:
>   static NSString* const kKnownNSExceptionNames[] = {

Done.

http://codereview.chromium.org/2769014/diff/17001/18010
File chrome/browser/cocoa/bug_report_window_controller.h (right):

http://codereview.chromium.org/2769014/diff/17001/18010#newcode112
chrome/browser/cocoa/bug_report_window_controller.h:112: 
On 2010/06/15 16:42:09, Mark Mentovai wrote:
> Kill the blank line at the end of the file while you’re in here.

Done.

http://codereview.chromium.org/2769014/diff/17001/18021
File chrome/browser/cocoa/keystone_glue.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18021#newcode60
chrome/browser/cocoa/keystone_glue.mm:60: @"CheckSuccessful";
On 2010/06/15 16:42:09, Mark Mentovai wrote:
> Looks like you had room for this on the previous line.

Done.

http://codereview.chromium.org/2769014/diff/17001/18021#newcode61
chrome/browser/cocoa/keystone_glue.mm:61: NSString* const
KSUpdateCheckSuccessfullyInstalledKey = @"SuccessfullyInstalled";
On 2010/06/15 16:42:09, Mark Mentovai wrote:
> On the other hand, this line is 81 characters long.

Done.

http://codereview.chromium.org/2769014/diff/17001/18021#newcode181
chrome/browser/cocoa/keystone_glue.mm:181: @"AutoupdateStatusNotification";
On 2010/06/15 16:42:09, Mark Mentovai wrote:
> There’s room for this one, though.

Done.

http://codereview.chromium.org/2769014/diff/17001/18024
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18024#newcode35
chrome/browser/cocoa/status_bubble_mac.mm:35: NSString* kFadeAnimationKey =
@"alphaValue";
On 2010/06/15 16:42:09, Mark Mentovai wrote:
> NSString* const?

Done.

http://codereview.chromium.org/2769014/diff/17001/18025
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/2769014/diff/17001/18025#newcode249
chrome/common/sandbox_mac.mm:249: NSString* const kCommonSandboxPrefixFileName =
@"common";
Canned it.

[Mark Mentovai, 2010-06-15 16:57:53]

I’m disinclined to search all of these files for more @properties, so I’ll trust
that you’ve done it.

(Did you grep around for others?)

[Mark Mentovai, 2010-06-15 16:58:00]

Oh, yeah. LGTM.

[Nico, 2010-06-15 17:01:35]

On 2010/06/15 16:57:53, Mark Mentovai wrote:
> I’m disinclined to search all of these files for more @properties, so I’ll
trust
> that you’ve done it.
> 
> (Did you grep around for others?)

Yes, I did `git gs @property | grep -v nonatomic`.

[Mark Mentovai, 2010-06-15 17:03:52]

git git git git git

[Mark Mentovai, 2010-06-15 17:09:38]

Wow, that seems to be Turkish.

[joth, 2010-06-15 18:44:25]

http://codereview.chromium.org/2769014/diff/28001/21054
File chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm (right):

http://codereview.chromium.org/2769014/diff/28001/21054#newcode41
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:41: @property
(nonatomic, readonly) BOOL isIBSS;
quick sanity check - as mentioned in the comment this section is based on the
interface declared in
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreWLAN.framework/Versions/A/Headers/CWNetwork.h

1/ will adding nonatomic impact compatibility with that interface?
2/ will the fact that interface lacks nonatomic cause problems when we switch to
used the 10.6 SDK headers directly (as oer the above TODO)?

[Mark Mentovai, 2010-06-15 19:30:04]

http://codereview.chromium.org/2769014/diff/28001/21054
File chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm (right):

http://codereview.chromium.org/2769014/diff/28001/21054#newcode41
chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:41: @property
(nonatomic, readonly) BOOL isIBSS;
joth wrote:
> quick sanity check - as mentioned in the comment this section is based on the
> interface declared in
>
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreWLAN.framework/Versions/A/Headers/CWNetwork.h

We should remove nonatomic from these, but to answer your questions:
 
> 1/ will adding nonatomic impact compatibility with that interface?

No.

> 2/ will the fact that interface lacks nonatomic cause problems when we switch
to
> used the 10.6 SDK headers directly (as oer the above TODO)?

Not directly. What will cause problems is a duplicate definition of the
@interface, but we face that problem regardless of whether the properties are
specified as atomic or not.

Clearly, we are not #importing (directly or indirectly) <CoreWLAN/CWNetwork.h>
anywhere relevant to this file with the 10.6 SDK now, or the 10.6 SDK build
would already be broken. (Some developers, myself included, routinely build and
test this configuration.)

[joth, 2010-06-16 07:13:34]

Great, thanks for the clarification.

On 15 June 2010 20:30, <mark@chromium.org> wrote:

>
> http://codereview.chromium.org/2769014/diff/28001/21054
> File chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm
> (right):
>
> http://codereview.chromium.org/2769014/diff/28001/21054#newcode41
> chrome/browser/geolocation/wifi_data_provider_corewlan_mac.mm:41:
> @property (nonatomic, readonly) BOOL isIBSS;
> joth wrote:
>
>> quick sanity check - as mentioned in the comment this section is based
>>
> on the
>
>> interface declared in
>>
>
>
>
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreWLAN.framework/Versions/A/Headers/CWNetwork.h
>
> We should remove nonatomic from these, but to answer your questions:
>
>
>  1/ will adding nonatomic impact compatibility with that interface?
>>
>
> No.
>
>
>  2/ will the fact that interface lacks nonatomic cause problems when we
>>
> switch to
>
>> used the 10.6 SDK headers directly (as oer the above TODO)?
>>
>
> Not directly. What will cause problems is a duplicate definition of the
> @interface, but we face that problem regardless of whether the
> properties are specified as atomic or not.
>
> Clearly, we are not #importing (directly or indirectly)
> <CoreWLAN/CWNetwork.h> anywhere relevant to this file with the 10.6 SDK
> now, or the 10.6 SDK build would already be broken. (Some developers,
> myself included, routinely build and test this configuration.)
>
>
> The TODO envisions both adding the #include and removing the local
definition as a single change.
My questions was more, if all properties need (nonatomic) to work with
clang, will the lack of (nonatomic) on the system header version not be a
problem. But I think you've already answered that.



> http://codereview.chromium.org/2769014/show
>

[Mark Mentovai, 2010-06-16 13:25:00]

Jonathan Dixon wrote:
> The TODO envisions both adding the #include and removing the local
> definition as a single change.

Correct, that’d be the only way to do it, unless we were to use an SDK
version check. Then we could have both the #include and our own copy
of the @interface, each wrapped in an appropriate macro.

> My questions was more, if all properties need (nonatomic) to work with
> clang, will the lack of (nonatomic) on the system header version not be a
> problem. But I think you've already answered that.

I didn’t answer this directly. Some of the key information is probably
buried earlier in the thread or in IRC conversations.

According to Nico, clang complains when it sees an atomic @property in
an @interface, but the corresponding @implementation does not use
@synchronized. This is an error when compiling the @implementation.
This doesn’t mean that all properties need to be nonatomic to use
clang, or even to use clang to compile the @implementation.

Apple controls the @implementation (and, for that matter, @interface)
of CoreWLAN.framework. They’re the ones who compile the
@implementation. If they choose to compile it with clang, it’s up to
them to make sure it builds. If they choose to do this, and the
@implementation is not already @synchronized, they can add
@synchronized (as one option.)

On the Chrome side, we only see the @interface, so we wouldn’t be
subject to problems regardless of what happens to the @implementation.

Mark