367155 - Notification names need "k" prefix

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Chris Lawson (gone)]

We have several FooNotificationName constants in our code that lack a "k" prefix. This should be fixed.

Comment 1

[Chris Peterson [:cpeterson]]

I fixed these notification name prefixes, but some of Camino's notification constants are named |FooNotificationName| and other simply |FooNotification|. Which is Camino's naming convention?

Apple's Cocoa naming convention uses |Notification| (and the string value itself matches the constant's name). For example:

APPKIT_EXTERN NSString *NSWindowWillCloseNotification; // = @"NSWindowWillCloseNotification";

Comment 2

[Chris Lawson (gone)]

Stuart, do you have an opinion here re: comment 1?

Comment 3

[Chris Lawson (gone)]

Re-assigning back to Chris Peterson. Not sure why this got changed...

Comment 4

[Stuart Morgan]

I'm not sure we really have a convention; I'd lean toward kFooNotification--the "Name" part seems superfluous, and they are long enough without adding unnecessary words.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: notification-names-v1.patch

This patch looks big, but the individual changes are pretty minor:

1. Ensure all notification name identifiers (and some int constants) have a |k| prefix.
2. Ensure all notification name identifiers have a |Notification| suffix.
3. Fix some misspellings of "notification".

Even though Apple's naming convention for Notification name strings is to match the Notification identifier (e.g. NSWindowWillCloseNotification = @"NSWindowWillCloseNotification"), this patch DOES NOT change Camino's notification name strings to match.

QUESTION: Some global constants (e.g. |kCHPermissionUnknown| and |kCHPermissionTypeCookie|) are marked |__attribute__((visibility("default")))|. Are these symbols exported for some external use? Or is the visibility attribute used to avoid some linker symbol stripping?

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> QUESTION: Some global constants (e.g. |kCHPermissionUnknown| and
> |kCHPermissionTypeCookie|) are marked |__attribute__((visibility("default")))|.
> Are these symbols exported for some external use? Or is the visibility
> attribute used to avoid some linker symbol stripping?

The ld in Xcode 2.5 dead-strips them otherwise (and they're needed by prefPanes), and we continue to support building on 10.4.  See bug 352451 comment 10 et seq. (and bug 475201 comment 35-38 for round 2) for more details ;)

Comment 7

[Chris Lawson (gone)]

Comment on attachment 507049 [details] [diff] [review]
notification-names-v1.patch

I'm not gonna have time for this, so kicking r? to ilya.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: notification-names-v2.patch

rebase kNotificationName patch against Camino tip.

Comment 9

[Ilya Sherman]

Comment on attachment 515849 [details] [diff] [review]
notification-names-v2.patch

r=ilya

(apologies for the delay in reviewing this!)

Comment 10

[Stuart Morgan]

Comment on attachment 515849 [details] [diff] [review]
notification-names-v2.patch

>-  [self addPermissionForSelection:CHPermissionAllow];
>+  [self addPermissionForSelection:kCHPermissionAllow];

Please undo the CHPermission* changes; they are named in the same style as NS* constants for various types (e.g., string encodings).

I know Apple uses that style for notifications too, but the fact that we've done something different historically for notifications doesn't mean we need to change everything else with it. I deliberately chose to use this style for the CH* wrappers.

>-extern const int CHCertificateOverrideFlagUntrusted;
>-extern const int CHCertificateOverrideFlagDomainMismatch;
>-extern const int CHCertificateOverrideFlagInvalidTime;
>+extern const int kCHCertificateOverrideFlagUntrusted;
>+extern const int kCHCertificateOverrideFlagDomainMismatch;
>+extern const int kCHCertificateOverrideFlagInvalidTime;

Same with these.


Hm. After seeing the scope of this patch I'm actually questioning the "done something different historically" part of the above. I hadn't really questioned the underlying assumption of this bug, which is that we general do kFoo for notifications and some were inconsistent. Is it actually the other way around?

But I guess most of our notifications have no prefix at all, which is not really the usual style for notification names either, and we have a patch that makes them consistent, which is good, so rather than bikeshedding I guess let's just say sr=smorgan withe the changes above, and move on with a consistent style :)

Comment 11

[Chris Peterson [:cpeterson]]

grep reports an even balance between Notification definitions with and without a 'k' prefix. Same for Notification definitions with and without a "Name" suffix. So there is not much of an established precedent. I can change the patch to which style you recommend.

grep -r "\<k\w*Notification\W*=\W*@" src | wc -l
7
grep -r "\<k\w*NotificationName\W*=\W*@" src | wc -l
9
grep -r "\<[^k]\w*Notification\W*=\W*@" src | wc -l
7
grep -r "\<[^k]\w*NotificationName\W*=\W*@" src | wc -l
9

Comment 12

[Stuart Morgan]

Let's just stick with k.

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: notification-names-v3.patch

Incorporate smorgan's feedback: do not add k- prefix to CHPermission* and CHCertificate* flags.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/4856ebc8d912

(I did not land the spurious Sparkle config hunk ;)  Chris, can you file a bug on disabling nib-stripping in Camino/prefPanes and Sparkle and make it block bug 514495?  There's no reason for us not to be doing that per bug 514495 comment 22 anyway, and since Sparkle still hasn't fixed their nib, turning stripping off in-tree removes one small level of hack needed for building on 10.6.)

Comment 15

[Chris Peterson [:cpeterson]]

oops. Sorry!

New bug 641852 - Disable nib-stripping in Camino/prefPanes and Sparkle.