We have several FooNotificationName constants in our code that lack a "k" prefix. This should be fixed.
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";
Status: NEW → ASSIGNED
Hardware: PowerPC → All
Stuart, do you have an opinion here re: comment 1?
Re-assigning back to Chris Peterson. Not sure why this got changed...
Assignee: nobody → mozilla.org
Status: NEW → ASSIGNED
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.
Created attachment 507049 [details] [diff] [review] 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?
Attachment #507049 - Flags: review?(cl-bugs-new2)
(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 on attachment 507049 [details] [diff] [review] notification-names-v1.patch I'm not gonna have time for this, so kicking r? to ilya.
Attachment #507049 - Flags: review?(cl-bugs-new2) → review?(ishermandom+bugs)
Created attachment 515849 [details] [diff] [review] notification-names-v2.patch rebase kNotificationName patch against Camino tip.
Comment on attachment 515849 [details] [diff] [review] notification-names-v2.patch r=ilya (apologies for the delay in reviewing this!)
Attachment #515849 - Flags: superreview?(stuart.morgan+bugzilla)
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 :)
Attachment #515849 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
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
Let's just stick with k.
Created attachment 519325 [details] [diff] [review] notification-names-v3.patch Incorporate smorgan's feedback: do not add k- prefix to CHPermission* and CHCertificate* flags.
Attachment #515849 - Attachment is obsolete: true
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.)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
oops. Sorry! New bug 641852 - Disable nib-stripping in Camino/prefPanes and Sparkle.
You need to log in before you can comment on or make changes to this bug.