1179129 - Would be nice to have about:profiles

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: patch 1 - about:profiles

What about an 'about:profiles' page where we can show the existing profiles, having an easy way to rename them, create new ones, delete them and so on...

I wrote a patch for that. If the reviewer agrees, maybe we can land it.
Bonus goal: have a full integration of about:profiles in the ff UI.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Does the patch ensure that currently used profiles can't be removed?


But this is great.

Comment 2

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Does the patch ensure that currently used profiles can't be removed?

Yes. You cannot remove the active profile. But you can rename it, and that's still ok.
I guess the next step, if this patch is ok, is to allow multiple profiles running at the same time. This would be great!

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

yes, I was asking about multiple running profiles. That is what I do all the time. 2-3 profiles running and perhaps 10 others for random testing purposes, but those are used only rarely, and tend to forget what all I even have.

Comment 4

[Dave Townsend [:mossop]]

We already have one profile manager that we've on and off been trying to get rid of, does it make sense to add a second?

Comment 5

[Benjamin Smedberg]

Indeed, although we might want to do something with better profile management in the future as part of great-or-dead, this is not code that we want to maintain.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I've never understood why we've tried to get rid of useful features, like profile manager, without
any replacement. about:profiles could be the replacement, at least for most of the features.

Comment 7

[Andrea Marchesini [:baku]]

I guess we should talk before marking a bug wontfix. Especially when there is a patch and especially after a work-week where we discuss about implementing this feature. Plus, getting rid of that profile manager doesn't meant that we cannot have something better.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

bsmedberg, could you explain why you're against about:profiles?

Comment 9

[Andrea Marchesini [:baku]]

Actually, having about:profiles would be a nice way to get rid of that profile manager. What about implementing a simple:  <xul:browser src="about:profiles" /> instead of that horrible UI we show with '-P'.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 2 - Profile Manager

This is just a prototype about replacing the existing ProfileManager with about:profiles. In this way we have the same UI and the same code for these 2 features.

Comment 11

[(no longer active)]

Comment on attachment 8628129 [details] [diff] [review]
patch 1 - about:profiles

Review of attachment 8628129 [details] [diff] [review]:
-----------------------------------------------------------------

I'm clearing the flag for now while we're waiting for Benjamin to respond, so that this doesn't keep showing up in my queue...

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Especially about:profiles page is rather nice. I see it as a similar level feature as
about:plugins or perhaps about:memory. Something for a tiny bit more educated users.

The version profile manager uses could perhaps hide Root/Local directory, and perhaps
Default Profile could be a checkbox or radiobutton.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Also, it would be nice to have the default profile as the first one.

Comment 14

[Andrea Marchesini [:baku]]

I agree with comment 12. I can improve the UI once we have a feedback from Benjamin.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8628749 [details] [diff] [review]
patch 2 - Profile Manager

So about:profile should perhaps have a mode for profile manager which hides some of the information by default.

Comment 16

[Andrea Marchesini [:baku]]

Do you think we can proceed somehow with these patches?

Comment 17

[(no longer active)]

I'm not sure.  I'll email Benjamin.

Comment 18

[Benjamin Smedberg]

I have several inter-related concerns.

* Profile management in any of its current forms is not good enough to expose to ordinary users. There are too many edge cases around startup, remoting, etc. I'm worried that exposing about:profiles will cause advanced users to start using it, which an anti-goal.
* Without also getting rid of the existing profile manager, this is duplicated UI and I'd reject it simply on that account.
* I primarily want to get rid of the existing profile manager so that I don't have to keep supporting XPCOM/gecko without a profile, and the startup/restart environment machinery.
* We need to be selective about all new features and the total amount of supported code. We're dying under a glut of unsupported features.
* I'm clearly the owner for the profile code, and I don't think that supporting this is a good tradeoff.
* I don't know whether it's possible to implement this as an addon, but I'd suspect so and I would strongly recommend that instead of building this in-tree.

Comment 19

[(no longer active)]

FWIW, the patches here seem to actually help with at least the 3rd points in comment 18 (even though they may not solve them completely), and it is also removing the existing profile manager code for the functionality it's implementing, as far as I can tell.

Comment 20

[Andrea Marchesini [:baku]]

Hi Benjamin, thanks for these points.

> * Profile management in any of its current forms is not good enough to
> expose to ordinary users. There are too many edge cases around startup,
> remoting, etc. I'm worried that exposing about:profiles will cause advanced
> users to start using it, which an anti-goal.

About this, my second patch removes the current form and replace it with about:profiles.
In this way we have just 1 single UI. Plus: we are not going to expose this about:profiles to ordinary uses. (I have to admin I would like to :) but this is a second step.

> * Without also getting rid of the existing profile manager, this is
> duplicated UI and I'd reject it simply on that account.

Right. Second patch is for that reason. Plus: we cannot get rid of the profile manager because for developers this is extremely useful. And aurora (developer edition) uses separate profiles. There are also several addons that use this profile manager.
We should get rid of the UI and replace it with something better.

I would like to point out that chrome has a similar feature (multiple-profiles) and this is very very useful. We have everything in place, except exposing it.

> * I'm clearly the owner for the profile code, and I don't think that
> supporting this is a good tradeoff.

I'm happy to help to manage the profile code.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

As far as I see, the patches simplify the code and the setup we have here. - in other words takes useful functionality out from profile manager and reuse it also for about:profile.
It is really hard to see the reluctantcy to improve the setup we have.


"get rid of the existing profile manager so that I don't have to keep supporting XPCOM/gecko without a profile" is probably the most annoying problem related to profile manager, but it is a separate issue from what is being done in this bug, and I think we should just have some separate profile for that case. But anyhow, different bug.

Comment 22

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #21)
> "get rid of the existing profile manager so that I don't have to keep
> supporting XPCOM/gecko without a profile" is probably the most annoying
> problem related to profile manager, but it is a separate issue from what is
> being done in this bug, and I think we should just have some separate
> profile for that case. But anyhow, different bug.

I don't understand this part.  It seems to me that when Andrea finishes building about:profiles, that problem would be completely solved.  Wouldn't it?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Oh, sure, if we just always start browser with some profile, and then selecting some other profile would go via about:profiles.

Comment 24

[Andrea Marchesini [:baku]]

if we run ff -ProfileManager, with my second patch we open about:profiles and from there we can open any existing profile.

Comment 25

[Andrea Marchesini [:baku]]

I really would like to proceed with this bug. Do we prefer to continue the discussion by email?
Or do we prefer to organize a meeting?

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8628129 [details] [diff] [review]
patch 1 - about:profiles

Benjamin, can you review these 2 patches?

Comment 27

[(no longer active)]

Comment on attachment 8628129 [details] [diff] [review]
patch 1 - about:profiles

Sorry, I'll get back to these very soon, promise!

Comment 28

[Andrea Marchesini [:baku]]

Attachment: patch 3 - 'Profiles' entry in the 'tool' menu

Comment 29

[(no longer active)]

Comment on attachment 8628129 [details] [diff] [review]
patch 1 - about:profiles

Review of attachment 8628129 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/locales/en-US/chrome/global/aboutProfiles.dtd
@@ +2,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY aboutProfiles.title "About Profiles">
> +<!ENTITY aboutProfiles.subtitle "This page helps you to manage your profiles. Each profile is a separate world who contains its own history, bookmarks, settings and Add-ons. Having multiple profiles can improve the security and the privacy of your web experience.">

Here is a suggestion for a better text:

This page helps you to manage your profiles. Each profile is a separate world which contains separate history, bookmarks, settings and add-ons.

Also, I would personally remove the following sentence.

"Having multiple profiles can improve the security and the privacy of your web experience."

I know what you're trying to explain here but I think this one sentence doesn't capture it.  If you think it's useful, we can have a "Learn More" link that links to an article on SUMO which explains things in more detail.

@@ +3,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY aboutProfiles.title "About Profiles">
> +<!ENTITY aboutProfiles.subtitle "This page helps you to manage your profiles. Each profile is a separate world who contains its own history, bookmarks, settings and Add-ons. Having multiple profiles can improve the security and the privacy of your web experience.">
> +<!ENTITY aboutProfiles.create "Create a new Profile">

Nit: "New"

@@ +6,5 @@
> +<!ENTITY aboutProfiles.subtitle "This page helps you to manage your profiles. Each profile is a separate world who contains its own history, bookmarks, settings and Add-ons. Having multiple profiles can improve the security and the privacy of your web experience.">
> +<!ENTITY aboutProfiles.create "Create a new Profile">
> +<!ENTITY aboutProfiles.restart.title "Restart">
> +<!ENTITY aboutProfiles.restart.inSafeMode "Restart with Add-ons Disabled...">
> +<!ENTITY aboutProfiles.restart.normal "Restart normally...">

Please use an ellipsis for the last two strings.

::: toolkit/locales/en-US/chrome/global/aboutProfiles.properties
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +name = Profile: %S
> +isDefault: Default Profile

Hmm, why the colons?  I was under the impression that the format of the properties file is lines containing |name = value| pairs.  Am I missing something?  Please use =.

@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +name = Profile: %S
> +isDefault: Default Profile
> +isSelected: Profile in-use

Looks unused?  Please re-check all strings to make sure everything is being used.

@@ +5,5 @@
> +name = Profile: %S
> +isDefault: Default Profile
> +isSelected: Profile in-use
> +rootDir: Root Directory
> +# LOCALIZATION NOTE: information about local directory can be read in nsIProfileLock

Please change this l10n note according to the nsIProfileLock.localDirectory comments.  An example would be nice.  Also please mention that if this is the same as rootDir, it won't be displayed.

Comment 30

[(no longer active)]

Comment on attachment 8628749 [details] [diff] [review]
patch 2 - Profile Manager

Review of attachment 8628749 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/aboutProfiles.js
@@ +15,5 @@
>    '@mozilla.org/toolkit/profile-service;1',
>    'nsIToolkitProfileService'
>  );
>  
> +const gProfileManager = window.location.href.indexOf('?profileManager') != -1;

How about "?manage"?  about:profiles?manage looks like a more sane URL.

Also, please name the variable gManage.

::: toolkit/locales/en-US/chrome/global/aboutProfiles.dtd
@@ +6,5 @@
>  <!ENTITY aboutProfiles.subtitle "This page helps you to manage your profiles. Each profile is a separate world who contains its own history, bookmarks, settings and Add-ons. Having multiple profiles can improve the security and the privacy of your web experience.">
>  <!ENTITY aboutProfiles.create "Create a new Profile">
>  <!ENTITY aboutProfiles.restart.title "Restart">
> +<!ENTITY aboutProfiles.restart.inSafeMode "Restart with Add-ons Disabled…">
> +<!ENTITY aboutProfiles.restart.normal "Restart normally…">

These will need to go in the previous patch.

::: toolkit/locales/en-US/chrome/global/aboutProfiles.properties
@@ +12,5 @@
>  
>  rename: Rename
>  remove: Remove
>  setAsDefault: Set as default profile
> +open: Open

Same comment about the colons.

::: toolkit/profile/content/profileSelection.js
@@ +17,5 @@
>  var gProfileService;
>  
>  function startup()
>  {
> +  gDialogParams = window.arguments[0].QueryInterface(I.nsIDialogParamBlock);

Do you mind converting the C and I variables to the canonical names in a follow-up, please?

Comment 31

[(no longer active)]

Comment on attachment 8696741 [details] [diff] [review]
patch 3 - 'Profiles' entry in the 'tool' menu

Let's not do this in this bug.  We can consider it in the future...

Comment 32

[Andrea Marchesini [:baku]]

ehsan, thanks a lot for these reviews. I want to land these patches but in separately:
First I want the about:profile, then in a separate bug, I want to land the replacement of ProfileManager.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da84e99a44da

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da84e99a44da

Comment 35

[Sören Hentzschel]

(In reply to Andrea Marchesini (:baku) from comment #2)
> (In reply to Olli Pettay [:smaug] from comment #1)
> > Does the patch ensure that currently used profiles can't be removed?
> 
> Yes. You cannot remove the active profile.

You cannot remove the *default profile* but you *can* remove the active profile if you don't use the default profile. Reported by a user in the German Firefox forum and confirmed by me.

Comment 36

[Tom Levels]

Is it an idea to optimise the about:profiles a bit? At the moment it is taking up a huge amount of space just to show 2 profiles?

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Please file new bugs about the tweaks the new profile manager or about:profiles need and make those bugs block this one.
I like the new UI, but agree it needs some changes.

Comment 38

[Andrea Marchesini [:baku]]

Yes, please, file a separate bug and CC/NI/Assign it to me. Thanks!

Comment 39

[Ryan Feeley [:rfeeley]]

I think improving the Profile Manager is definitely worth doing; without a good one users are often given no choice but to use another browser. Chrome has true multi-user support, which I assume for us is a long away away, but would love to see.

I'm hoping Madhava can assign someone from UX to improve the design of this feature and am NIing him.

IMO it should primarily be about launching profiles, and secondarily about CRUD.