Closed
Bug 367155
Opened 18 years ago
Closed 13 years ago
Notification names need "k" prefix
Categories
(Camino Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bugzilla-graveyard, Assigned: cpeterson)
Details
(Keywords: polish)
Attachments
(1 file, 2 obsolete files)
157.14 KB,
patch
|
Details | Diff | Splinter Review |
We have several FooNotificationName constants in our code that lack a "k" prefix. This should be fixed.
Updated•17 years ago
|
Target Milestone: Camino1.6 → ---
Assignee | ||
Comment 1•14 years ago
|
||
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
Updated•14 years ago
|
Status: ASSIGNED → NEW
Reporter | ||
Comment 3•14 years ago
|
||
Re-assigning back to Chris Peterson. Not sure why this got changed...
Assignee: nobody → mozilla.org
Status: NEW → ASSIGNED
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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 ;)
Reporter | ||
Comment 7•13 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
rebase kNotificationName patch against Camino tip.
Attachment #507049 -
Attachment is obsolete: true
Attachment #515849 -
Flags: review?(ishermandom+bugs)
Attachment #507049 -
Flags: review?(ishermandom+bugs)
Comment 9•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #515849 -
Flags: review?(ishermandom+bugs) → review+
Comment 10•13 years ago
|
||
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+
Assignee | ||
Comment 11•13 years ago
|
||
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•13 years ago
|
||
Let's just stick with k.
Assignee | ||
Comment 13•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•13 years ago
|
||
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.
Description
•