profile-startup-category notifications don't happen if profile is specified

RESOLVED FIXED

Status

SeaMonkey
Startup & Profiles
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Andrew Schultz, Assigned: Andrew Schultz)

Tracking

({fixed-seamonkey1.1a})

Trunk
fixed-seamonkey1.1a
Bug Flags:
blocking-seamonkey1.1a +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

5.58 KB, patch
Biesinger
: review+
neil@parkwaycc.co.uk
: superreview+
Robert Kaiser
: approval-seamonkey1.1a+
Details | Diff | Splinter Review
2.25 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Robert Kaiser
: approval-seamonkey1.1a+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
profile-startup-category notifications happen in nsProfile::LoadDefaultProfileDir, but that method is not called if the commandline included the profile (ProcessArgs would have called SetCurrentProfile).  It seems that the notifications should happen at the end of SetCurrentProfile.

LoadDefaultProfileDir also does pref conversion to UTF8, which seems like another thing that should live in SetCurrentProfile.
(Assignee)

Comment 1

12 years ago
Created attachment 228494 [details] [diff] [review]
patch

This just moves things from LoadDefaultProfileDir into SetCurrentProfile.
Assignee: nobody → ajschult
Status: NEW → ASSIGNED
Attachment #228494 - Flags: review?(benjamin)

Comment 2

12 years ago
Comment on attachment 228494 [details] [diff] [review]
patch

I'm sorry, I don't even kinda-sorta own this code and don't want to.
Attachment #228494 - Flags: review?(benjamin)
(Assignee)

Updated

12 years ago
Attachment #228494 - Flags: review?(cbiesinger)
(Assignee)

Comment 3

12 years ago
It seems this stuff is not used by toolkit apps
Component: Profile: BackEnd → Profile: Manager
Product: Core → Mozilla Application Suite
(Assignee)

Comment 4

12 years ago
The only effect of this bug is that the update notification doesn't work.  But that seems like a serious enough problem to need this for 1.1
Flags: blocking-seamonkey1.1a?

Updated

12 years ago
Flags: blocking-seamonkey1.1a? → blocking-seamonkey1.1a+
Comment on attachment 228494 [details] [diff] [review]
patch

I don't know this code very well but it looks OK to me. Please don't add trailing whitespace on those new lines. (This does make the listener run on live profile switching, but that seems ok)
Attachment #228494 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

12 years ago
Attachment #228494 - Flags: superreview?(neil)

Comment 6

12 years ago
(In reply to comment #5)
>This does make the listener run on live profile switching, but that seems ok)
No it doesn't, because the update notifier assumes it only starts once.

Comment 7

12 years ago
Hmm, can we get a fix into 1.8 branch within the next week, so we can get 1.1a out in time?
(Assignee)

Comment 8

12 years ago
Created attachment 234338 [details] [diff] [review]
fix update notifier

This makes the update notifier not notify twice after switching profiles.
Attachment #234338 - Flags: superreview?(neil)
Attachment #234338 - Flags: review?(neil)

Comment 9

12 years ago
Comment on attachment 234338 [details] [diff] [review]
fix update notifier

>-const kDebug               = false;
>+const kDebug               = true;
Did you really mean this? ;-)

>+    // Check if we've already been called.
>+    if (initialized)
>+      return;
>+    initialized = true;
You should use this.mInitialized etc, not a global. sr=me with this fixed.
Attachment #234338 - Flags: superreview?(neil)
Attachment #234338 - Flags: superreview+
Attachment #234338 - Flags: review?(neil)
Attachment #234338 - Flags: review+

Comment 10

12 years ago
Comment on attachment 228494 [details] [diff] [review]
patch

>+               nsCOMPtr <nsIProfileStartupListener> listener = do_CreateInstance(contractidString.get(), &rv);
Since we'll notify on a profile switch, we need do_GetService here.
Otherwise we'll create new update notifiers on each profile switch!
sr=me with this fixed.
Attachment #228494 - Flags: superreview?(neil) → superreview+

Comment 11

12 years ago
Comment on attachment 228494 [details] [diff] [review]
patch

a=me for 1.1 given it works on trunk.
Attachment #228494 - Flags: approval-seamonkey1.1a+

Comment 12

12 years ago
Comment on attachment 234338 [details] [diff] [review]
fix update notifier

a=me for 1.1 given it works on trunk.
Attachment #234338 - Flags: approval-seamonkey1.1a+
(Assignee)

Comment 13

12 years ago
FIXED on trunk and branch
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed-seamonkey1.1a
Resolution: --- → FIXED
(Assignee)

Comment 14

12 years ago
this exposed the fact that nsUpdateNotifier gets leaked (bug 349323)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

12 years ago
oops, meant to leave this fixed
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.