506470 - Alert service needs to not initialize Growl on app startup

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: don't add AlertsService to app-startup

Connecting to Growl takes an age (upwards of 1s on cold start, or 100ms or so on warm start).  There's no need for it to be in the app-startup category; it'll get connected the first time an alert needs to come up.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Bug 498366 comment 3 looks relevant. I'm not really in the best position to comment on whether that benefit is worth the cost, though (I'm thinking: no).

If you're removing it as an app-startup observer, you might as well get rid of the "final-ui-startup" observing code as well.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I'm willing to assert "no" there.

Comment 3

[Mark Banner (:standard8)]

Am I missing something, or will this patch cause growl only to be initialised if a user of the alerts service initialises it before final-ui-startup?

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> Am I missing something, or will this patch cause growl only to be initialised
> if a user of the alerts service initialises it before final-ui-startup?
That is correct - growl will only be initialized once the service is initialized, but it doesn't matter if that happens before or after final-ui-startup.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 390652 [details] [diff] [review]
don't add AlertsService to app-startup

I think this is the wrong user experience, but everyone else seems to disagree.

r=sdwilsh on the code changes.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #4)
> (In reply to comment #3)
> > Am I missing something, or will this patch cause growl only to be initialised
> > if a user of the alerts service initialises it before final-ui-startup?
> That is correct - growl will only be initialized once the service is
> initialized, but it doesn't matter if that happens before or after
> final-ui-startup.

However before-growl-registration only gets set round from final-ui-startup, so if growl isn't initialised before final-ui-startup then the notification doesn't get sent round and observers won't be able to register.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 390652 [details] [diff] [review]
don't add AlertsService to app-startup

Comment 6 makes a good point.  That needs to be addressed before this can get r+.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v2

Ah right, forgot to move that chunk to Init.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 394844 [details] [diff] [review]
v2

>+  NS_ASSERTION([GrowlApplicationBridge growlDelegate] == nil,
>+               "We already registered with Growl!");
Won't this assert every time but the first time we call this method?

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

As well it should -- it's a service, its Init function should never be called more than once, right?

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 394844 [details] [diff] [review]
v2

I misread the function it was in.  I thought we were doing this every time we called showAlertNotification

r=sdwilsh

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/19b9494ef411

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 394844 [details] [diff] [review]
v2

This patch also "fixes" (works around) bug 510740.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 394844 [details] [diff] [review]
v2

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ed58f8421896

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 394844 [details] [diff] [review]
v2

past code-freeze for 1.9.1.4, removing non-blocker approval.