378527 - Localize nsIAlertsService

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Shawn Wilsher :sdwilsh]

The notification name needs to be localized.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Colin Barrett [: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.

Comment 3

[Shawn Wilsher :sdwilsh]

(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.

Comment 4

[Asaf Romano (gone)]

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?

Comment 5

[Shawn Wilsher :sdwilsh]

(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 6

[Asaf Romano (gone)]

I cannot see why not.

Comment 7

[Colin Barrett [:cbarrett]]

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.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses Mano's comments online.

Comment 9

[Asaf Romano (gone)]

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.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

This also fixes the Bug 380142 because I was doing something stupid with the category manager.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.2.1

s/nsAutoString/nsString/ when using the GetString function as per discussion on irc with bz.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 264792 [details] [diff] [review]
v1.2.1

r=mano.

Comment 13

[Wayne Woods]

+ * @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 14

[Colin Barrett [:cbarrett]]

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 15

[Colin Barrett [:cbarrett]]

Comment on attachment 264792 [details] [diff] [review]
v1.2.1

forgot to set the flag

r+ with the changes I mentioned.

Comment 16

[Shawn Wilsher :sdwilsh]

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

Comment 17

[Shawn Wilsher :sdwilsh]

I missed some files.  It's a good thing we had that fallback code ;)

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v1.2.1+

Comment 19

[Asaf Romano (gone)]

Comment on attachment 265202 [details] [diff] [review]
v1.2.1+

r=mano

Comment 20

[Shawn Wilsher :sdwilsh]

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