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

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Andrew Schultz]

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.

Comment 1

[Andrew Schultz]

Attachment: patch

This just moves things from LoadDefaultProfileDir into SetCurrentProfile.

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Andrew Schultz]

It seems this stuff is not used by toolkit apps

Comment 4

[Andrew Schultz]

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

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 6

[[e-mail]]

(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

[Robert Kaiser]

Hmm, can we get a fix into 1.8 branch within the next week, so we can get 1.1a out in time?

Comment 8

[Andrew Schultz]

Attachment: fix update notifier

This makes the update notifier not notify twice after switching profiles.

Comment 9

[[e-mail]]

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.

Comment 10

[[e-mail]]

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.

Comment 11

[Robert Kaiser]

Comment on attachment 228494 [details] [diff] [review]
patch

a=me for 1.1 given it works on trunk.

Comment 12

[Robert Kaiser]

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

a=me for 1.1 given it works on trunk.

Comment 13

[Andrew Schultz]

FIXED on trunk and branch

Comment 14

[Andrew Schultz]

this exposed the fact that nsUpdateNotifier gets leaked (bug 349323)

Comment 15

[Andrew Schultz]

oops, meant to leave this fixed