Status

()

defect
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
PowerPC
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

12 years ago
The notification name needs to be localized.
Assignee

Updated

12 years ago
Status: NEW → ASSIGNED
Flags: blocking-firefox3+
Assignee

Comment 1

12 years ago
Posted patch v1.0 (obsolete) — Splinter Review
Attachment #264039 - Flags: review?(cbarrett)
Comment on attachment 264039 [details] [diff] [review]
v1.0

Looks good. Couple questions:

- We register with Growl on XPCOM startup, and we're just randomly somewhere in the list of things that register, right? Is it possible that things after we load would want to add notifications? I might be misunderstanding how this works, so feel free to correct me.

- It might be worth it to add a helper method somewhere to the notification name loading. You've got it duplicated twice as is.

Other than that, it looks fine from a Cocoa perspective.
Assignee

Comment 3

12 years ago
(In reply to comment #2)
> - We register with Growl on XPCOM startup, and we're just randomly somewhere in
> the list of things that register, right? Is it possible that things after we
> load would want to add notifications? I might be misunderstanding how this
> works, so feel free to correct me.

Looking at the code, we actually do it in app startup, which will be fine.

> - It might be worth it to add a helper method somewhere to the notification
> name loading. You've got it duplicated twice as is.

As per our discussion today, there's no good place to do this.
Assignee

Updated

12 years ago
Attachment #264039 - Flags: review?(mano)
Comment on attachment 264039 [details] [diff] [review]
v1.0

So given that you cannot really bail out here.. would it be better to keep a hard-coded string here as a fall-back?
Assignee

Comment 5

12 years ago
(In reply to comment #4)
> (From update of attachment 264039 [details] [diff] [review])
> So given that you cannot really bail out here.. would it be better to keep a
> hard-coded string here as a fall-back?

Certainly, but I wasn't sure if that was acceptable behavior - if it is, I can do that.
Comment on attachment 264039 [details] [diff] [review]
v1.0

Looks good. File a bug on moving the registering to final ui startup, as we discussed, but this one looks good to go.
Attachment #264039 - Flags: review?(cbarrett) → review+
Assignee

Updated

12 years ago
Attachment #264039 - Flags: review?(mano)
Assignee

Comment 8

12 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Addresses Mano's comments online.
Attachment #264039 - Attachment is obsolete: true
Attachment #264701 - Flags: review?(mano)
Assignee

Updated

12 years ago
Attachment #264701 - Flags: review?(cbarrett)
Comment on attachment 264701 [details] [diff] [review]
v1.1

I would rather set the fall-back strings on the end of this function; on success, set the localized string and just return early, then you don't need the fall-backs duplicated.
Assignee

Comment 10

12 years ago
Posted patch v1.2 (obsolete) — Splinter Review
This also fixes the Bug 380142 because I was doing something stupid with the category manager.
Attachment #264701 - Attachment is obsolete: true
Attachment #264779 - Flags: review?(mano)
Attachment #264779 - Flags: review?(cbarrett)
Attachment #264701 - Flags: review?(mano)
Attachment #264701 - Flags: review?(cbarrett)
Assignee

Comment 11

12 years ago
Posted patch v1.2.1Splinter Review
s/nsAutoString/nsString/ when using the GetString function as per discussion on irc with bz.
Attachment #264779 - Attachment is obsolete: true
Attachment #264792 - Flags: review?(mano)
Attachment #264792 - Flags: review?(cbarrett)
Attachment #264779 - Flags: review?(mano)
Attachment #264779 - Flags: review?(cbarrett)

Comment 13

12 years ago
+ * @param aName   The notificatino name to dispatch

I can't contribute anything meaningful to the patch, so I'll have to resort to minor typos instead ;)
Comment on attachment 264792 [details] [diff] [review]
v1.2.1

>     mDict = [[NSMutableDictionary dictionaryWithCapacity: 8] retain];
>+    
>+    mNames   = [[NSMutableArray arrayWithCapacity: 1] retain];
>+    mEnabled = [[NSMutableArray arrayWithCapacity: 1] retain];

Don't do it like that. Do this:

[[NSMutableArray alloc] init];

Or if you must, use initWithCapacity:, but there's really no need to do so.

r=cbarrett with those changes.
Comment on attachment 264792 [details] [diff] [review]
v1.2.1

forgot to set the flag

r+ with the changes I mentioned.
Attachment #264792 - Flags: review?(cbarrett) → review+
Assignee

Comment 16

12 years ago
Checking in toolkit/components/alerts/src/mac/Makefile.in;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/alerts/src/mac/mozGrowlDelegate.h;
new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/alerts/src/mac/mozGrowlDelegate.mm;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/alerts/src/mac/nsAlertsImageLoadListener.h;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/alerts/src/mac/nsAlertsImageLoadListener.mm;
new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/alerts/src/mac/nsAlertsService.mm;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/alerts/src/mac/nsAlertsServiceModule.cpp;
new revision: 1.4; previous revision: 1.3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee

Comment 17

12 years ago
I missed some files.  It's a good thing we had that fallback code ;)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 18

12 years ago
Posted patch v1.2.1+Splinter Review
Attachment #265202 - Flags: review?
Assignee

Updated

12 years ago
Attachment #265202 - Flags: review? → review?(mano)
Assignee

Comment 20

12 years ago
Checking in toolkit/locales/jar.mn;
new revision: 1.36; previous revision: 1.35
Checking in toolkit/locales/en-US/chrome/alerts/notificationNames.properties;
initial revision: 1.1
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.