Closed Bug 378527 Opened 14 years ago Closed 14 years ago

Localize nsIAlertsService

Categories

(Firefox :: Shell Integration, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(2 files, 3 obsolete files)

The notification name needs to be localized.
Status: NEW → ASSIGNED
Flags: blocking-firefox3+
Attached 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.
(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.
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?
(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+
Attachment #264039 - Flags: review?(mano)
Attached patch v1.1 (obsolete) — Splinter Review
Addresses Mano's comments online.
Attachment #264039 - Attachment is obsolete: true
Attachment #264701 - Flags: review?(mano)
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.
Attached 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)
Attached 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)
+ * @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+
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: 14 years ago
Resolution: --- → FIXED
I missed some files.  It's a good thing we had that fallback code ;)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v1.2.1+Splinter Review
Attachment #265202 - Flags: review?
Attachment #265202 - Flags: review? → review?(mano)
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: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.