Closed
Bug 506470
Opened 15 years ago
Closed 15 years ago
Alert service needs to not initialize Growl on app startup
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: vlad, Assigned: vlad)
References
Details
(Whiteboard: [ts][needs 1.9.1 landing])
Attachments
(1 file, 1 obsolete file)
4.40 KB,
patch
|
sdwilsh
:
review+
vlad
:
approval1.9.2+
dveditz
:
approval1.9.1.4-
|
Details | Diff | Splinter Review |
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.
Attachment #390652 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [ts]
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
Yeah, I'm willing to assert "no" there.
Comment 3•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
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.
Attachment #390652 -
Flags: review?(sdwilsh) → review+
Comment 6•15 years ago
|
||
(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•15 years ago
|
||
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+.
Attachment #390652 -
Flags: review+ → review-
Assignee | ||
Comment 8•15 years ago
|
||
Ah right, forgot to move that chunk to Init.
Assignee: nobody → vladimir
Attachment #390652 -
Attachment is obsolete: true
Attachment #394844 -
Flags: review?(sdwilsh)
Comment 9•15 years ago
|
||
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?
Assignee | ||
Comment 10•15 years ago
|
||
As well it should -- it's a service, its Init function should never be called more than once, right?
Comment 11•15 years ago
|
||
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
Attachment #394844 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 12•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/19b9494ef411
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Attachment #394844 -
Flags: approval1.9.2?
Updated•15 years ago
|
blocking1.9.1: needed → ---
Updated•15 years ago
|
Attachment #394844 -
Flags: approval1.9.1.4?
Comment 13•15 years ago
|
||
Comment on attachment 394844 [details] [diff] [review] v2 This patch also "fixes" (works around) bug 510740.
No longer blocks: 510740
Assignee | ||
Updated•15 years ago
|
Attachment #394844 -
Flags: approval1.9.2? → approval1.9.2+
Comment 14•15 years ago
|
||
Comment on attachment 394844 [details] [diff] [review] v2 Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #394844 -
Flags: approval1.9.1.4? → approval1.9.1.4+
Updated•15 years ago
|
Whiteboard: [ts] → [ts][needs 1.9.2 landing][needs 1.9.1 landing]
Assignee | ||
Comment 15•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ed58f8421896
status1.9.2:
--- → beta1-fixed
Assignee | ||
Updated•15 years ago
|
Whiteboard: [ts][needs 1.9.2 landing][needs 1.9.1 landing] → [ts][needs 1.9.1 landing]
Comment 16•15 years ago
|
||
Comment on attachment 394844 [details] [diff] [review] v2 past code-freeze for 1.9.1.4, removing non-blocker approval.
Attachment #394844 -
Flags: approval1.9.1.4+ → approval1.9.1.4-
You need to log in
before you can comment on or make changes to this bug.
Description
•