97622 - (profile-switching) Put in UI for dynamic profile switching

Status: VERIFIED FIXED

(SeaMonkey :: Startup & Profiles, enhancement)

Description

[Conrad Carlen (not reading bugmail)]

We have supported dynamic profile switching for some time for embedding. It has
never been used by mozilla. Once bug 86021 was checked in, the support for it
was complete. It is being used with -turbo mode when multiple profiles are
present. It would be a great enhancement to allow this to be used outside of the
-turbo feature. If there was a "Profiles..." item on the Edit menu, a user could
go to it and pick a new profile and, without restarting, use that profile.

What needs to be done is:
* There needs to be a profile change observer in xpfe which closes any open
windows in response to a "profile-teardown" notification
* Some minor UI work - adding the "Profiles..." menu item, contextualizing the
buttons in the profile mgr dialog to say "OK" instead of "Start" in this case

Comment 1

[Conrad Carlen (not reading bugmail)]

-> me

Comment 2

[Conrad Carlen (not reading bugmail)]

Gerv, Can you think about the proposed UI? Any help would be great. To save
work, I want to use the existing profile selection dialog but change the button
text. Make sense?

Comment 3

[Gervase Markham [:gerv]]

My initial view is that reusing the Profile Manager dialog is overkill. My
suggestion would be a menu item:

Change Profile > X gerv
                   ccarlen
                   jag

But, if every window gets torn down, perhaps some sort of warning is needed, or
maybe we do need a switching dialog which contains a warning.

CCing mpt for his views.

Gerv

Comment 4

[Conrad Carlen (not reading bugmail)]

> My initial view is that reusing the Profile Manager dialog is overkill.

Why overkill? Why not be able to create and delete profiles as well? This is
what the embedding apps do and it's really handy. Also, using the same dialog is
much less work :-)

> But, if every window gets torn down, perhaps some sort of warning is needed

Yes. That's what I was planning. That's what the embedding apps do (and Outlook
Express on Mac)

Comment 5

[Jaime Rodriguez, Jr.]

Moving outnto 0.9.5 per PDT

Comment 6

[Conrad Carlen (not reading bugmail)]

CC'ing Kai.

Comment 7

[Matthew T (active 1999-2002)]

If you were to provide access to the full-blown Profile Manager in the menus, 
an appropriate place would be `Tools' > `Profile Manager'. Note that since the 
Profile Manager is not a dialog, this menu item would not have an ellipsis.

However, I agree with Gerv that this would be rather messy. One minor problem 
is that the `Switch To' button would (except on Mac OS) have to revert to 
`Start' when the last non-profile-manager window was closed. A more important 
problem is that you would need to alter the Profile Manager UI to make sure 
that the user couldn't delete the profile they were currently using.

So, I think a `Tools' > `Change User...' menu item, opening the Select Profile 
dialog (minus the `Manage Profiles...' button), would make more sense.

Comment 8

[Conrad Carlen (not reading bugmail)]

"Tasks -> Tools -> Change User..." is too obscure. I think it should be beside
"File -> Quit" since really, you are quitting the session as that user. I'm
taking this idea from OE on the Mac which handles profile switching pretty
perfectly. They even have a command key for it (Cmd-Shift-Q)

They also allow you to fully manage profiles, not just select them from this
dialog. In our case, that means using the exact same profile dialog that we
currently use. It comes up in selection mode. Since it's the same dialog,
there's a checkbox "Show At Startup." So, if you want to be bothered by a dialog
at startup, you can. I don't want to be bothered at startup - I just want it to
start in the profile I usually use and if I want to change it, I can.
 

Comment 9

[Gervase Markham [:gerv]]

> "Tasks -> Tools -> Change User..." is too obscure.

I believe mpt is talking about the wonderful new world when we have a Window
menu and a Tools menu, rather than a single Tasks menu.

I like the idea of Mozilla starting with the last profile you used unless "show
at at startup" is checked. We could probably use the full Profile Manager dialog
from a "Change Profile/User/Whatever" option just above Quit. I think that the
familiarity of the dialog being the same in both cases outweighs the
simplification of removing the Manage Profiles button.

Gerv

Comment 10

[Gervase Markham [:gerv]]

Hang on... my mozilla -profilemanager doesn't show a Manage Profiles... button.
Are we talking about the same UI?

Gerv

Comment 11

[Conrad Carlen (not reading bugmail)]

> Are we talking about the same UI?

Yes. It's the same dialog but it has two modes: 'select' and 'manage'.
When you use mozilla (no args) and have multiple profiles, it comes up in
'select' mode. Clicking 'Manage Profiles...' puts it into 'manage' mode.
When  you use mozilla -profileManager, the dialog comes up in 'manage' mode.

When this dialog is brought up from a menu, I think it should come up in
'select' mode because selection is the most common operation, but you would be
able to hit the 'Manage Profiles..." button and get the rest of the (less
common) functionality if you wanted to.

Comment 12

[Conrad Carlen (not reading bugmail)]

-> 0.9.6

Comment 13

[Kai Engert (:KaiE:)]

Changing my e-mail address.

Comment 14

[Kai Engert (:KaiE:)]

Removing old e-mail address.

Comment 15

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.7

Comment 16

[Conrad Carlen (not reading bugmail)]

-> 0.9.8

Comment 17

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.9

Comment 18

[Judson Valeski]

futuring.

Comment 19

[Brian 'netdragon' Bober]

Personally, I think it should be put into the "File" Menu in place of "Edit
Page", which should, be in the "Edit" menu instead.

It should also bring up the Profile Manager imho, not display available
profiles. It would be messy for the UI if someone had a lot of users available.
Also, you would have to also add "Profile Manager..." to that list because
people might want to add a new profile, etc (as I did today). People that use
the "quicklaunch" feature shouldn't have to restart Mozilla in order to add a
new Profile.

Please put this in for 1.0

Comment 20

[Brian 'netdragon' Bober]

Also, if you do this dynamic switching, please also make it work on the
commandline too. New bug?

Comment 21

[John Levon]

*** Bug 125957 has been marked as a duplicate of this bug. ***

Comment 22

[Adam Monsen]

Why is this 

Comment 23

[Adam Monsen]

(oops... hit that button too fast)

Why is this enhancement Future'd?

Comment 24

[timeless]

because someone managing resources at netscape decided it wasn't a high 
priority, if you'd like to work on it, you're welcome to do so. otherwise it 
will have to wait for someone to get around to it.

I'm currently working on live skin and locale switching. profile switching 
would be the next thing on that logical hitlist, but it's not a priority for 
me, I have 5 other profile things i'd like to do and plenty of non switching 
things to do.

Comment 25

[Jesse Ruderman]

See also bug 60299, Profile manager can't be started from mozilla.

Comment 26

[Alfonso Martinez]

*** Bug 160568 has been marked as a duplicate of this bug. ***

Comment 27

[Brian 'netdragon' Bober]

timeless: We really need to make an FAQ for things like: "Why is this bug 
futured?"

Comment 28

[Joel Nelson (don't send email)]

Nominating for nsbeta.

Comment 29

[Conrad Carlen (not reading bugmail)]

Putting on front burner (1.2)

Comment 30

[Conrad Carlen (not reading bugmail)]

Attachment: screenshot #1 (menu)

Comment 31

[Conrad Carlen (not reading bugmail)]

Attachment: screenshot #2 (profile dialog shown from menu)

Here is the same dialog, adapted to this context. Button titles are different,
offline checkbox is gone, and in "Manage" mode, "Delete Profile" is of course
disabled if the current profile is selected. The dialog is completely unchanged
in the app startup context.

Comment 32

[Conrad Carlen (not reading bugmail)]

Attachment: patch

Kai kindly provided the menu diffs. The patch is sizable mainly because a lot
of code needed to move from nsApprunner.cpp to nsAppShellService.cpp. The code
for creating the initial state of open windows according to prefs needed to be
in appshell because it's the profile observer. I think the code belongs in
appshell more than it does in apprunner, anyway.

Comment 33

[Conrad Carlen (not reading bugmail)]

CC'ing pink and alecf for review. Unless the UI people think this needs major
change, I think it's pretty solid. Still need to build & test on the 2
platforms. One thing I would like to do on the Mac is to put "Switch Profile" in
the application menu, rather than the File menu so it's next to "Quit." That
would take some work in the Mac menu code because we don't currently support
putting menu items specified by XUL into the application menu.

Oh, one thing about the screen shots - the profile picker shown is classic and
the navigator windows shown are modern because they are different profiles set
to different themes. The profile picker dialog is in whatever theme of the
current profile - it's not hardwired to Classic or anything.

Comment 34

[Mike Pinkerton (not reading bugmail)]

not sure if i'm the best choice for a review of this stuff, given that i've
never seen any of the profile mgr code. one point i'd make is that i don't think
this item should go into the File menu on win/unix. Tools would seem more
appropriate for those platforms, much more so than File. I think the app menu on
mac is probably the correct place.

Comment 35

[Alec Flett]

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

indenting is a little funny here:
+	 // String0	->	mode
+	 // String1	<-	profileName
+	 // Int0			<-	result code (1 == OK, 0 ==
Cancel)

I'm not sure I like the name "doInitialState" - my philosophy is always that
"do" is redundant in a function name. how about initializeState() or
resetState() or something?

we can't have global NS_NAMED_LITERAL_CSTRINGs:
http://www.mozilla.org/hacking/portable-cpp.html#dont_use_static_constructors

so I think you need to go back to #define's :(

as for OpenWindow() parameters - is there any reason you can't use const
nsAString and PromiseFlatCString()?

in Ensure1Window, the 2nd PR_sscanf, you're passing tempString, not
tempString.get()

In the Enter/ExistWindowClosingSurvivalArea() section of code in Observe(), can
you add a comment warning people not to exit early from that block of code?

other than these minor details, this is a great cleanup. Really nice to see. I
think I agree with pink that I like the Tools menu (and the system menu on mac)
as well... is there a spec you're following, or can it just be moved? jag is
really the frontend owner so he really gets the final say.

overall the patch is fine, but I'll mark needs-work since I had so many
comments :)

Comment 36

[Alex Bishop]

Conrad, are you aware that the Select User Profile dialogue mentions a Start 
Mozilla button that doesn't exist?

Comment 37

[Alex Bishop]

Additionally, I agree with Pink and Alec that the Tools menu would be a better 
place for this than the File menu as the other personalised stuff (Password 
Manager, Forms Manager etc.) is already located there.

Comment 38

[Kai Engert (:KaiE:)]

In file nsAppShellService, you are adding calls to PR_sscanf. However, the
required include prprf.h is only included in debug builds. I suggest to remove
the #idef NS_DEBUG around the include.

Comment 39

[Conrad Carlen (not reading bugmail)]

I fixed what Alec and what Kai pointed out. Before new patch, I have to make it
work on non-Mac platforms (apparently, Enter/ExitWindowClosingSurvivalArea() is
not enough to keep us alive when the last window closes) Also, getting the menu
item to be in the app menu on OSX is going to take some work. 

Comment 40

[Conrad Carlen (not reading bugmail)]

Attachment: patch.v2

Very reworked patch.

1. After posting the last one, I found that if the mail window was the active
window when the profile was switched, the mail window could not be closed when
it should. It had to do with what the current JS context was at the time. Now
the context from which nsIProfile.currentProfile is called is the profile
dialog and that always works. The profile dialog is the last window to close in
a switch, but I think that's OK.
2. It puts a checkbox in the profile selection dialog which allows you skip
profile selection at startup and just have it start with the last profile.
Since you can change it at any time, no reason to be forced to do it at
startup. This problem, and different suggestions for avoiding it, is in bugs
71359, 115331, and 162280.
3. Fixes various bugs after more testing. One notable on was that the mail
account mgr could not be shut down more than once because a flag was not being
reset.
4. Addresses Alec's comments.

Comment 41

[Conrad Carlen (not reading bugmail)]

Attachment: profile dialog in both contexts

Forgot to mention about the patch - it also puts the "Switch Profile..." item
in the Tools menu, not in File. Putting it in the application menu on OSX can
be another bug.

Here's the dialog. The title "Launch With Last Profile" needs improvement. The
meaning could be inverted to "Select Profile At Launch" or "Show This Dialog At
Startup" and maybe it would be more clear. Also, having 2 checkboxes in the
dialog at startup is really ugly. I wish the Offline checkbox could go away.
It's of no use currently when you have 1 profile and wit this patch, it's also
useless if you checked the new checkbox. Also, it has nothing to do with
profiles anyway.

Comment 42

[Conrad Carlen (not reading bugmail)]

Attachment: profile dialog in both contexts

Forgot to mention about the patch - it also puts the "Switch Profile..." item
in the Tools menu, not in File. Putting it in the application menu on OSX can
be another bug.

Here's the dialog. The top view is when it is shown at startup and the bottom
is when it is shown from the menu. The checkbox title "Launch With Last
Profile" needs improvement. The meaning could be inverted to "Show This Dialog
At Startup" and maybe it would be more clear. Also, having 2 checkboxes in the
dialog at startup is really ugly. I wish the Offline checkbox could go away.
It's of no use currently when you have 1 profile and wit this patch, it's also
useless if you checked the new checkbox. Also, it has nothing to do with
profiles anyway.

Comment 43

[Conrad Carlen (not reading bugmail)]

CC'ing Jag since he owns the front end.

Comment 44

[timeless]

does anyone remember how roaming profiles meshed w/ work offline?
for now i'd vote to hide 'work offline' the only thing it really does is avoid
network hits, and if people really care they can configure other things to
prevent that.

as for your checkbox, ug.

[x] always use this profile if available

it's still bad text, but it's somewhat meaningful.

other issues. you haven't mentioned locale switching, or skin switching.  what
happens if i try to select a profile which has a different skin or locale?

I'm pretty sure that bad things will happen (a bunch of my bugs for those have
patches i haven't committed).

also, nss and mailnews tend to leave files open forever, do you have any way to
assure us that they are reformed? (I've seen those bugs recently, so i'm fairly
certain that they're still broken.) these bugs are the reasons that quicklaunch
was changed to 'respawn mozilla after quiting' instead of profile switching.

I actually have an alternative suggestion for this silly dialog which would be
to move [ ] offline to a properties dialog and add skin and locale selection to
it.  Your average user isn't going to need to change these things, so we can
make them click 'manage profiles', 'properties' or know to
alt-double-click/alt-enter a profile if they want to access the screen.  I'd be
pretty happy if I could select my skin or locale in the profile manager, since
it should save me a restart.  Actually properties would be really useful for the
roaming case too, so we really should consider doing that (four consumers .. wow).

Also, where's the '(my) help' button ;-?

Comment 45

[Conrad Carlen (not reading bugmail)]

> other issues. you haven't mentioned locale switching, or skin switching.  what
happens if i try to select a profile which has a different skin or locale?

Works great. The chrome registry is a profile change observer and does the right
thing to make that work.


> also, nss and mailnews tend to leave files open forever, do you have any way
to assure us that they are reformed?

Yes, those things are profile change observers and that works as well.

(I've seen those bugs recently, so i'm fairly
certain that they're still broken.)

NSS is known to be working now - ask Kai.

> these bugs are the reasons that quicklaunch was changed to 'respawn mozilla
after quiting' instead of profile switching.

I'm not sure about that. After the NSS and mailnews problems were fixed with
profile switching, another problem was discovered with localstore where if the
profile *wasn't* switched, it caused problems. At that point, somebody came in
with the quit and respawn slegehammer :-/

So, while all of the subsystems are believed to be working with profile
switching at this point, this still needs to go in (early in a milestone) and
the subsystems need to be made to work. Part of this is to have an XP testcase 
(mozilla) in order to find these bugs in the subsystems sooner so that we can
ensure profile switching works for embedding. Having it in MfcEmbed & PPEmbed is
not getting us enough coverge. 

Comment 46

[Alex Bishop]

I don't want to sound like a broken record but the dialogue text is still
referencing a 'Start Mozilla' button that doesn't exist!

> as for your checkbox, ug.
>
> [x] always use this profile if available
>
> it's still bad text, but it's somewhat meaningful.

The checkbox doesn't do that, does it? I thought it made it so that Mozilla
skips the Select User Profile dialogue on start up and just uses the last profile.

However, I agree that the current text isn't great. I'm not entirely convinced
that this checkbox belongs in the Select User Profile dialogue; perhaps it
should only be in the Profile Manager.

Comment 47

[timeless]

1. Mailnews and a few other culprits cache locale strings without listening for
notifications, the result is various inconsistencies which make things look
really bad.

Given the number of bugs that we continue to receive for red &irc_key; I'd
expect a nice flood of bugs from people who decided to try using profile
switching and ran into the locale switching errata.

2. So have you tested various skins? at the very least we need to be able to
handle switching between: modern, classic, newclassic, littlemozilla (also
wood), pinball and orbit.  until about yesterday there was a scrollbar crash
related to skin switching (i finally checked in a patch which matches a patch
for that problem, so i think that specific crash is gone).

If live skin and locale switching actually work, then i think we should probably
get them activated first before we expose this. we should find the other
gremlins before we end up juggling two profiles and three undertested codepaths.

I know that we want coverage for this feature, but it really is too late to
target 1.2 (your tm is 1.2b, the keyword is 1.2, and this is why i'm flagging
about things which i would ignore for an alpha milestone).

The changes you have here do too many things [code reorganization, live
switching, use most recent profile (but only from a gui)], and imo do them in
the wrong order.

Since you're doing command line parser movement, perhaps you could at least add
something like -PreviousProfile (which would be equivalent to that silly
checkbox), somewhere in one of my many dead trees i had done this (sorry).

--
Alex is of course right, listen to him :)

yeah, move the checkbox to profile manager, 
[x] use most recent profile if possible

we should be able to try to find a live mozilla using that profile under windows
(using semaphores?) and X11 (using xremote).

and definitely make sure the descriptive text (which should be much shorter)
makes sense :)

Comment 48

[Alec Flett]

well, if we land this early enough in 1.3 (I'm assuming that's where this is
destined?) then we have the whole 1.3 cycle to fix that... right?

Comment 49

[Alec Flett]

(oops, that was about the caching of UI strings)

Comment 50

[Conrad Carlen (not reading bugmail)]

Yes - early in 1.3 is where it belongs, not now in 1.2. Stale milestone field :-/

> 2. So have you tested various skins? at the very least we need to be able to
handle switching between: modern, classic, newclassic, littlemozilla (also
wood), pinball and orbit.  until about yesterday there was a scrollbar crash
related to skin switching

Modern and classic anyway. Don't confuse what happens in a profile switch with
what happens in a skin switch (which had problems) In a profile switch, all
windows are closed, and the RDF data source of the chrome is completely reset
and reloaded once the new profile is available. Skin switching which at one
point was live from the menus, attempted to change the skin on all open windows
which is much more fraught with peril.

Comment 51

[Conrad Carlen (not reading bugmail)]

> I don't want to sound like a broken record but the dialogue text is still
referencing a 'Start Mozilla' button that doesn't exist!

You're right, I missed that. Actually, that text is a good place to mention
that, in a profile switch, all open windows will be closed, etc.

As far as putting the checkbox in the profile manager dialog, I don't think so.
The profile selection dialog is the one to which the checkbox applies, so that's
where it should be.

> Since you're doing command line parser movement, perhaps you could at least
add something like -PreviousProfile (which would be equivalent to that silly
checkbox)

Why? If you're using a command line, what's wrong with -P "last-used-profile?"
Most people probably don't launch from a command line and some are on platforms
where you *can't* launch from a command line.

Comment 52

[timeless]

you can on macos (MOZZARGS) and on windows (.lnk). shortcuts allow arguments.

why? because i want the last used profile whose name i don't know, and i'm
actually most likely to have a bunch of shortcuts that i double click on. one
would launch the most recent, one might launch some specific one, or maybe i
only have two, launch most recent and launch manager. but i'd like to be able to
have this option controllable from outside of the binary. your pref system
doesn't allow that.

Comment 53

[Alex Bishop]

> As far as putting the checkbox in the profile manager dialog, I don't think so.
> The profile selection dialog is the one to which the checkbox applies, so that's
> where it should be.

On second thoughts, I think I agree with you. Having the checkbox in the Select
User Profile dialogue will make it easy for a user who is annoyed with the
dialogue popping up every time they start Mozilla to turn it off (sort of like
the 'Don't show me this again' checkboxes featured in some alerts). However, I
think that the checkbox should be in the Profile Manager as well, because it
provides a superset of the Select User Profile functionality.

Comment 54

[Brian 'netdragon' Bober]

See also bug 176961 for the ability to dynamic switch a profile but leave it 
in memory for later use without any windows/etc being closed.

Comment 55

[Kai Engert (:KaiE:)]

We need to fix bug 177260 before this feature will work. When Conrad said in a
previous comment that PSM is working now, that was indeed a statement from me,
but more research showed we still have leaks. Luckily we meanwhile have tools to
test the correctness better. With the fixes from bug 177260 and its dependent
NSS fixes, I am not able to make it fail any more. And with this bug 97622
implemented, we will have an even better testing environment. I'm confident, if
there are more problems in PSM/NSS, we will find them soon.

Comment 56

[jag (Peter Annema)]

CC'ing browser UE (Patrice). I think the UI is okay, I'm actually more worried
about the back-end :-)

Comment 57

[Conrad Carlen (not reading bugmail)]

> I'm actually more worried about the back-end

The backend for profile switching has been in place for about 2 years and has
been used by major embedding clients in that time. What has not been as well
tested in the back end has been mailnews since it's not been embedded. However,
account manager and address book are profile change observers and currently
behave well. One reason for exposing this in mozilla is to get more testing of
the backend implementation by the mozilla community on all platforms. And, if
this goes in early in 1.3, it will be made reliable by 1.3 final. That's quite a
bit of time considering there's only small parts of the codebase which don't
have the amount of testing that the embeddable parts do.

Comment 58

[patrice caire]

UI looks good to me.

Comment 59

[Kai Engert (:KaiE:)]

Attachment: Patch v2b

Patch v2 does no longer apply to the trunk.
I hope this updated patch has the conflicts merged correctly.

However, the patch does not work as expected.

When I try to switch the profile, the windows go away and a new browser window
gets opened. But regardless which profile I select, it always uses the profile
that was initially loaded.

Tested on Linux.

Comment 60

[Conrad Carlen (not reading bugmail)]

Thanks - I'll see what's broken since I posted the last patch :-/

Comment 61

[timeless]

profile switching isn't ready to land. bz and i discovered that filehandling
doesn't listen. it's on my plate (which also includes getting the other live
switches to work -- i don't keep an ordered list of tasks, and i'm running with
the patch, so i should probably run with this patch to see if things work).

I discovered it using the null profile provider (which is really good for
detecting dumb assumptions).

If people are interested in a null profile provider so that they can test for
the various assertions people make, i can post that patch :) -- the fireworks
are pretty impressive.

Comment 62

[Conrad Carlen (not reading bugmail)]

> bz and i discovered that filehandling doesn't listen.

Is there a bug on this? Point is, profile switching *has* landed and embedding
clients are using it. Bugs like that you mention with file handling are not
blocking profile switching from landing, they are bugs right here and now
whether or not mozilla exposes this functionality.

Comment 63

[Kai Engert (:KaiE:)]

Attachment: Patch v3 (Kai's v8)

This patch has two changes.

First, it merges in patch from bug 175320.

Second, it introduces an additional notification that is required for the fix
to bug 177260. The new notification is "profile-change-teardown-veto", which
only gets sent, if somebody calls veto while we notify
"profile-change-teardown".

Comment 64

[Conrad Carlen (not reading bugmail)]

Attachment: current snapshot

No changes, just the diffs as they've been kept up to date in my tree. I've
gotta clean my tree out and the blockers are still here.

Comment 65

[Kai Engert (:KaiE:)]

Attachment: Updated snapshot

This patch has just two hunks removed, they are now in bug 175320.

Comment 66

[Conrad Carlen (not reading bugmail)]

Attachment: patch v3

This patch fixes the problem mentioned in bug 177260, comment 30. After doing
the teardown phases of shutting down the profile, it does JS GC. This causes
things to be freed before "profile-do-change" is sent out, fixing the crash
with NSS. This also fixes bug 156940, for the same reason.

It also does some UI cleanup. The explaination text in the dialog is changed,
depending on the context. It no longer mentions a "Start Mozilla" button that
doesn't exist when brought up from the menu. The new checkbox is now called
"Don't Ask at Startup" I was going to call it "Don't bother me with this dialog
every time..."

Anyway, this is still blocked. But, since the GC on profile shutdown does quite
a bit of good by itself, and the movent of code from nsAppRunner to
nsAppShellService is nice, low-risk cleanup, I think I'd to check in most of
this patch soon. Basically, just leave out the changes to the menu overlays
which actually expose this.

Comment 67

[Kai Engert (:KaiE:)]

Attachment: Patch v3 (no changes, merged with latest trunk)

Comment 68

[Kai Engert (:KaiE:)]

I would like to know whether the application behaves correctly and does not fail
when attempting a profile switch.
If you want to help testing, please use crypto functionality (like SSL, https,
certificate management) in a browser session, and try the profile switching
feature at the end of your session.

Test builds for Windows (.zip), Linux (.tar.gz) and Mac (.dmg) are available at:
  http://www.kuix.de/mozilla/moz-20030118-97622/
checksums:
  ae075e73426313b3c790f190820d62cb  moz-20030118-97622.dmg
  3290f80b30eab85048cc16895f4a0f80  moz-20030118-97622.tar.gz
  5f110a2ce93f6c816cc83b83c2423634  moz-20030118-97622.zip

Note that there is one known exception. If you have configured your mail
addressing to look up address data from a LDAP server using a secure connection,
switching the profile can fail with the error message: "The operation can not be
completed because of an internal failure. A
secure network communication has not been cleaned up correctly."

Comment 69

[Conrad Carlen (not reading bugmail)]

This is currently suffering from bug 189779, but there's a fix in hand for that one.

Comment 70

[Alec Flett]

Comment on attachment 111917 [details] [diff] [review]
Patch v3 (no changes, merged with latest trunk)

man I hate nsIDialogParamBlock :)

I have mixed feelings about forcing the JS_GC - is there no way to release the
objects in question? (often there is not)

in HandleArbitraryStartup() can you switch from the (const char*) cast to a
.get() and a test for .IsEmpty()

sr=alecf with those nits - I know that some of this is just code you moved, but
if you can just get them while we're here, it would be great. Thanks!

Comment 71

[Kai Engert (:KaiE:)]

Comment on attachment 111917 [details] [diff] [review]
Patch v3 (no changes, merged with latest trunk)

1)

>+      if( delbutton.getAttribute( "disabled" ) == "true" )
>+        delbutton.removeAttribute( "disabled", "true" );

Instead of checking and removing with name and value, should we just call
removeAttribute("disabled"); ?
I suspect this change makes it safer, just in case getAttribute throws an
exception if there is no such attribute.
In addition, I believe removeAttribute only takes one parameter.


2)

I can't judge whether the call to JS GC is ok, however, it makes sense to me.
However, in addition to checking the out param from GetSafeJSContext, should
you also check NS_SUCCEEDED(GetSafeJSContext())?


3)

In nsProfileAccess::GetStartWithLastUsedProfile, should you check that you got
a non-null pointer?


r=kaie - Please decide yourself whether you want to change the above.


Your patch compiles and works for me on Linux, Win & OSX.

Comment 72

[Kai Engert (:KaiE:)]

Comment on attachment 111917 [details] [diff] [review]
Patch v3 (no changes, merged with latest trunk)

mid air collission changed alecf's superreview flag, setting back to sr=alecf

Comment 73

[Conrad Carlen (not reading bugmail)]

Checked in, after adressing Kai's and Alec's comments. I kept the JS GC in
because, during the teardown phase when all windows are closed, everything owned
by the window chrome which might depend on the profile really needs to be
destroyed right then. Possibly, it's an unescesary safety net, but the most
recent round of testing was done with it in place and it's a bit scary to remove
it at the last minute :-/

Thanks for reviews and help.

Comment 74

[[e-mail]]

Ctrl+Alt+Q? Whose idea was that?
Firstly, Ctrl+Alt+ is currently busted on Windows
Secondly, it's reserved on Windows for shortcut hotkeys anyway...

Comment 75

[Conrad Carlen (not reading bugmail)]

Attachment: add nsCloseAllWindows.js to packages.

The js component which closes all windows is only currently packaged on Mac.
Without this, this patch won't work in any official build :-/

Comment 76

[sairuh (rarely reading bugmail)]

cc'ing aaronl (he tracks keyboard shortcuts).

Comment 77

[Aaron Leventhal]

All keyboard assignments have to follow the rules in the keyboard FAQ.
FAQ: http://www.mozilla.org/projects/ui/accessibility/mozkeyintro.html
Ctrl+Alt letters are not allowed, for various reasons.

You can do the work of finding an available keystroke using the FAQ, and the
chart at http://www.mozilla.org/projects/ui/accessibility/mozkeylist.html. Or,
just ask me and I'll find you a key.

Is this currently checked in with a Ctrl+Alt letter shortcut?

Comment 78

[Conrad Carlen (not reading bugmail)]

> Is this currently checked in with a Ctrl+Alt letter shortcut?

Yes. I'll find something else for it.

Comment 79

[Conrad Carlen (not reading bugmail)]

Bogus keyboard shortcut is bug 190172. 

Comment 80

[Conrad Carlen (not reading bugmail)]

Comment on attachment 112300 [details] [diff] [review]
add nsCloseAllWindows.js to packages.

Doug - anything I need to now about adding to packages WRT GRE?

Comment 81

[Alec Flett]

oh! I've never seen nsCloseAllWindows.js before, it wasn't in the earlier
patches - is this similar to nsKillAll.js? Can we roll the functionality into that?

Comment 82

[Conrad Carlen (not reading bugmail)]

It wasn't in earlier patches because it's not new. It's been around for a while
before this patch. What I failed to see was that it's only packaged on Mac. I
should have known that because I was the one that checked it in (long ago).

Comment 83

[Doug Turner (:dougt)]

Comment on attachment 112300 [details] [diff] [review]
add nsCloseAllWindows.js to packages.

yup.  I think you are going to have to add a line to your Makefile which
exports this file into the GRE.  talk to seawood about doing this.

ooc - why are we implementing such things in js?

Comment 84

[Alec Flett]

Comment on attachment 112300 [details] [diff] [review]
add nsCloseAllWindows.js to packages.

ok, odd. We should really see if we can roll them together at some point -
should help startup performance
sr=alecf

Comment 85

[Conrad Carlen (not reading bugmail)]

Comment on attachment 112300 [details] [diff] [review]
add nsCloseAllWindows.js to packages.

Request for a= comment:
 This JS component has always been packaged on Mac, but not on all platforms. I
overlooked this when the rest of this went in Tuesday.

Comment 86

[Asa Dotzler [:asa]]

Comment on attachment 112300 [details] [diff] [review]
add nsCloseAllWindows.js to packages.

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 87

[Conrad Carlen (not reading bugmail)]

Attachment 112300 [details] [diff] is checked in. This should be usable in tomorrow's builds.

Comment 88

[Torben]

*** Bug 60299 has been marked as a duplicate of this bug. ***

Comment 89

[Grace Bush]

verified

Comment 90

[Torben]

*** Bug 100981 has been marked as a duplicate of this bug. ***