Localize nsIAlertsService

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---
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

Updated

12 years ago
Flags: blocking-firefox3+
(Assignee)

Comment 1

12 years ago
Created attachment 264039 [details] [diff] [review]
v1.0
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
Created attachment 264701 [details] [diff] [review]
v1.1

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
Created attachment 264779 [details] [diff] [review]
v1.2

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
Created attachment 264792 [details] [diff] [review]
v1.2.1

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
Last Resolved: 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
Created attachment 265202 [details] [diff] [review]
v1.2.1+
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
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.