1086936 - Add a UI switch to disable custom profile selection process in Aurora

Status: VERIFIED FIXED

(DevTools :: General, defect)

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

Almost there.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Preference dialog

This is what the preference dialog looks like.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: In-content preference

This is what in-content preferences look like.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

I should have mentioned that the strings are being added in bug 1086938. Stephen, how does it look?

Comment 5

[Francesco Lodolo [:flod]]

What is the "Get Started" button supposed to do?

Comment 6

[Joe Walker [:jwalker]]

(In reply to Francesco Lodolo [:flod] from comment #5)
> What is the "Get Started" button supposed to do?

Open about:accounts.

Comment 7

[Joe Walker [:jwalker]]

The text used by Panos here is what I noted in an email, but I've misgivings about it.
The biggest is that we're mentioning profiles as though the user understands them. We should probably avoid this buzzword.
Also I think we should use the full version of the product name if there's space.

Comment 8

[Francesco Lodolo [:flod]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #7)
> The biggest is that we're mentioning profiles as though the user understands
> them. We should probably avoid this buzzword.

Link to SUMO on the word profiles? Given the target of this version (and that's the rationale behind some other decisions), I don't consider that jargon.

> Also I think we should use the full version of the product name if there's space.

I'd prefer not, considering that's not localizable, while the short version sounds like a code name and it's passable.

Comment 9

[Francesco Lodolo [:flod]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #6)
> (In reply to Francesco Lodolo [:flod] from comment #5)
> > What is the "Get Started" button supposed to do?
> 
> Open about:accounts.

I would have never guessed based on the structure. Something more explicit (with "Sync" in it) would be useful.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 1 - Add a UI switch to disable custom profile selection process in Aurora

Small tweaks to the previous version.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 2 - Add a flag to nsIAppStartup::quit that restarts the browser without using the same profile and use it for the Aurora UI switch

Restarting after toggling the checkbox reuses the current profile, which is exactly the wrong thing to do in this case. This patch adds a new flag to nsIAppStartup::quit, eRestartNotSameProfile that does the right thing. I bet I have to rev some IDs, but I'm not sure which ones.

Comment 13

[Benjamin Smedberg]

Comment on attachment 8509662 [details] [diff] [review]
part 2 - Add a flag to nsIAppStartup::quit that restarts the browser without using the same profile and use it for the Aurora UI switch

You don't, actually.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 3 - Hide the profile manager's autoSelectLastProfile option in Developer Edition when a separate default profile is used

I wrote this and then I realized that we don't really need it, as I explain in bug 1024110 comment 39. Posting here for posterity.

Comment 15

[Stephen Horlander [:shorlander]]

Attachment: DevFox-i01-(OSX)-PreferencesWindow - i01

Suggestions:
- "Keep Firefox Developer Edition data separate from Firefox data" or maybe
- "Keep Firefox Developer Edition data and Firefox data separate"
-- Don't mention profiles

- Tip Text: "Tip: You can use Firefox Sync to share data between applications. Use Sync…"
-- Make "Tip" text slightly smaller to maintain visual hierarchy (it's part of the above option and it's a suggestion, not essential)
-- Make "Use Sync…" text a link since it takes you to the accounts setup/sign-in page

- I think we can find a shorter string for the "Make Default" button that doesn't involve the full product name. It is already listed twice in the proceeding text…

needinfoing Matej for a sanity check.

Comment 16

[Stephen Horlander [:shorlander]]

Oh, something i meant to ask: What happens if someone unchecks it? Does it merge the data into the main Firefox profile?

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Stephen Horlander [:shorlander] from comment #16)
> Oh, something i meant to ask: What happens if someone unchecks it? Does it
> merge the data into the main Firefox profile?

No, it just restarts with the default profile instead of the dev-edition profile. Apart from Sync there is no other sharing of profile data going on. And only clicking "Use Sync" or the similar button from the UI Tour will do the actual copying of Fx Account username from the default profile to the dev-edition profile.

Comment 18

[Joe Walker [:jwalker]]

(In reply to Stephen Horlander [:shorlander] from comment #15)
> Created attachment 8509836 [details]
> DevFox-i01-(OSX)-PreferencesWindow - i01
> 
> Suggestions:
> - "Keep Firefox Developer Edition data separate from Firefox data" or maybe
> - "Keep Firefox Developer Edition data and Firefox data separate"
> -- Don't mention profiles

The point of this is to allow parallel running. I think we need to mention the effect on the user rather than just the cause of the problem.

I think :flod makes a valid point - this text isn't in release so the text 'profile' is only visible to people that have signed up for something advanced - maybe that makes it ok.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 1 - Add a UI switch to disable custom profile selection process in Aurora

Updated per Stephen's comments. Waiting for a final approval before requesting review.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Prefs screenshot

Here is a combined screenshot of how things look now for both dialog and in-content prefs. What do you think?

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

In case you were wondering, I didn't have the branding changes applied when I took that last screenshot.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Prefs screenshot

Here is a screenshot with the branding patch applied to avoid any confusion.

Comment 23

[Matej Novak [:matej]]

(In reply to Stephen Horlander [:shorlander] from comment #15)
> Created attachment 8509836 [details]
> DevFox-i01-(OSX)-PreferencesWindow - i01
> 
> Suggestions:
> - "Keep Firefox Developer Edition data separate from Firefox data" or maybe
> - "Keep Firefox Developer Edition data and Firefox data separate"
> -- Don't mention profiles

I'm guessing this could also refer to Beta or Nightly, so maybe this to be really clear:

"Keep Firefox Developer Edition data separate from other Firefox data"


> - Tip Text: "Tip: You can use Firefox Sync to share data between
> applications. Use Sync…"

We've been trying to avoid "Firefox Sync" where possible. "Between applications" also feels a little misleading.

"Tip: Use Sync to share data between versions of Firefox. Start using Sync…"


> - I think we can find a shorter string for the "Make Default" button that
> doesn't involve the full product name. It is already listed twice in the
> proceeding text…

"Always check if this is your default browser"

Also, "default browser" should be lowercase in both lines.

Thanks.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Matej Novak [:matej] from comment #23)
> (In reply to Stephen Horlander [:shorlander] from comment #15)
> > - I think we can find a shorter string for the "Make Default" button that
> > doesn't involve the full product name. It is already listed twice in the
> > proceeding text…
> 
> "Always check if this is your default browser"
> 
> Also, "default browser" should be lowercase in both lines.

I think there is a disconnect between the suggestion above, which applies to the label above the button and the button label that Stephen wants to shrink. Should I switch both or just the text and not the button?

To be clear, here are the strings I'm thinking of changing:

- "Always check to see if &brandShortName; is the default browser on startup" -> "Always check if this is your default browser"
- "Make &brandShortName; My Default Browser" -> "Make this my default browser"

Is this correct?

Comment 25

[Francesco Lodolo [:flod]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #24)
> - "Always check to see if &brandShortName; is the default browser on
> startup" -> "Always check if this is your default browser"
> - "Make &brandShortName; My Default Browser" -> "Make this my default
> browser"

Do you plan to have these changes only on Dev edition?

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Francesco Lodolo [:flod] from comment #25)
> Do you plan to have these changes only on Dev edition?

That's a question for Stephen and Matej really, but my guess is no.

Comment 28

[Matej Novak [:matej]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #24)
> (In reply to Matej Novak [:matej] from comment #23)
> > (In reply to Stephen Horlander [:shorlander] from comment #15)
> > > - I think we can find a shorter string for the "Make Default" button that
> > > doesn't involve the full product name. It is already listed twice in the
> > > proceeding text…
> > 
> > "Always check if this is your default browser"
> > 
> > Also, "default browser" should be lowercase in both lines.
> 
> I think there is a disconnect between the suggestion above, which applies to
> the label above the button and the button label that Stephen wants to
> shrink. Should I switch both or just the text and not the button?
> 
> To be clear, here are the strings I'm thinking of changing:
> 
> - "Always check to see if &brandShortName; is the default browser on
> startup" -> "Always check if this is your default browser"
> - "Make &brandShortName; My Default Browser" -> "Make this my default
> browser"
> 
> Is this correct?

Sorry, I'm a little lost as to which of the above strings you're proposing we use. Can you clarify?

Comment 29

[Matej Novak [:matej]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #26)
> (In reply to Francesco Lodolo [:flod] from comment #25)
> > Do you plan to have these changes only on Dev edition?
> 
> That's a question for Stephen and Matej really, but my guess is no.

Definitely not my call. I'm just here for the words.

Comment 30

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> Do you plan to have these changes only on Dev edition?

To clarify my question: we're trying to fix this on Aurora, which means landing string 2 weeks into the cycle on a string frozen branch. And that's one not so hidden piece of UI displayed to users when they open settings.

Comment 31

[Stephen Horlander [:shorlander]]

Attachment: DevFox-i01-(OSX)-PreferencesWindow — i02

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #26)
> (In reply to Francesco Lodolo [:flod] from comment #25)
> > Do you plan to have these changes only on Dev edition?
> 
> That's a question for Stephen and Matej really, but my guess is no.

To be clear I am fine with naming the product in both the question about default checking and the statement about default status (pink). I don't think we need to list the name again in the button (teal). We should make this change everywhere.

Comment 32

[Matej Novak [:matej]]

(In reply to Stephen Horlander [:shorlander] from comment #31)
> Created attachment 8511063 [details]
> DevFox-i01-(OSX)-PreferencesWindow — i02
> 
> (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> comment #26)
> > (In reply to Francesco Lodolo [:flod] from comment #25)
> > > Do you plan to have these changes only on Dev edition?
> > 
> > That's a question for Stephen and Matej really, but my guess is no.
> 
> To be clear I am fine with naming the product in both the question about
> default checking and the statement about default status (pink). I don't
> think we need to list the name again in the button (teal). We should make
> this change everywhere.

We could remove the words "to see" to make the string shorter.

Comment 33

[Stephen Horlander [:shorlander]]

Attachment: DevFox-i01-(OSX)-PreferencesWindow — i03

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #18)
> (In reply to Stephen Horlander [:shorlander] from comment #15)
> > Created attachment 8509836 [details]
> > DevFox-i01-(OSX)-PreferencesWindow - i01
> > 
> > Suggestions:
> > - "Keep Firefox Developer Edition data separate from Firefox data" or maybe
> > - "Keep Firefox Developer Edition data and Firefox data separate"
> > -- Don't mention profiles
> 
> The point of this is to allow parallel running. I think we need to mention
> the effect on the user rather than just the cause of the problem.

So we are creating a separate profile by default to allow for parallel use, but AFAICT, we aren't messaging this anywhere outside of prefs; where most people won't look. We might want to mention this somewhere in on the product page or first-run.

I guess we could say: 

"Allow Firefox Developer Edition and Firefox to run at the same time."
"Tip: This uses separate profiles. Use Sync to share data between versions of Firefox. Start using Sync…"

Comment 34

[Stephen Horlander [:shorlander]]

(In reply to Matej Novak [:matej] from comment #32)
> (In reply to Stephen Horlander [:shorlander] from comment #31)
> > Created attachment 8511063 [details]
> > DevFox-i01-(OSX)-PreferencesWindow — i02
> > 
> > (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> > comment #26)
> > > (In reply to Francesco Lodolo [:flod] from comment #25)
> > > > Do you plan to have these changes only on Dev edition?
> > > 
> > > That's a question for Stephen and Matej really, but my guess is no.
> > 
> > To be clear I am fine with naming the product in both the question about
> > default checking and the statement about default status (pink). I don't
> > think we need to list the name again in the button (teal). We should make
> > this change everywhere.
> 
> We could remove the words "to see" to make the string shorter.

Works for me.

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Matej Novak [:matej] from comment #28)
> (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> comment #24)
> > (In reply to Matej Novak [:matej] from comment #23)
> > > (In reply to Stephen Horlander [:shorlander] from comment #15)
> > > > - I think we can find a shorter string for the "Make Default" button that
> > > > doesn't involve the full product name. It is already listed twice in the
> > > > proceeding text…
> > > 
> > > "Always check if this is your default browser"
> > > 
> > > Also, "default browser" should be lowercase in both lines.
> > 
> > I think there is a disconnect between the suggestion above, which applies to
> > the label above the button and the button label that Stephen wants to
> > shrink. Should I switch both or just the text and not the button?
> > 
> > To be clear, here are the strings I'm thinking of changing:
> > 
> > - "Always check to see if &brandShortName; is the default browser on
> > startup" -> "Always check if this is your default browser"
> > - "Make &brandShortName; My Default Browser" -> "Make this my default
> > browser"
> > 
> > Is this correct?
> 
> Sorry, I'm a little lost as to which of the above strings you're proposing
> we use. Can you clarify?

Sorry for the confusion, the format I used was:

"existing string" -> "replacement string"

And I'm not really proposing, just trying to understand what you folks wanted the strings to look like.

(In reply to Stephen Horlander [:shorlander] from comment #34)
> > We could remove the words "to see" to make the string shorter.
> 
> Works for me.

I thought your main issue was with the length of the button label though? Although if you are concerned about the wrapping of the statement label, then shrinking that label would also work (but "to see" is not present in that label).

Or are you suggesting we do both, remove "to see" and remove the name from the button label?

Comment 36

[Stephen Horlander [:shorlander]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #35)
> (In reply to Stephen Horlander [:shorlander] from comment #34)
> > > We could remove the words "to see" to make the string shorter.
> > 
> > Works for me.
> 
> I thought your main issue was with the length of the button label though?
> Although if you are concerned about the wrapping of the statement label,
> then shrinking that label would also work (but "to see" is not present in
> that label).
> 
> Or are you suggesting we do both, remove "to see" and remove the name from
> the button label?

I am primarily concerned about the size of the button and the redundancy of listing the product name three times in the same entry. But if we can make the other strings more concise then we should. 

Although that is less of a concern, especially since the common case for this will just say "Firefox" which is quite a bit shorter than "Firefox Developer Edition" :)

Comment 37

[Joe Walker [:jwalker]]

(In reply to Stephen Horlander [:shorlander] from comment #33)
> Created attachment 8511069 [details]
> DevFox-i01-(OSX)-PreferencesWindow — i03
> 
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #18)
> > (In reply to Stephen Horlander [:shorlander] from comment #15)
> > > Created attachment 8509836 [details]
> > > DevFox-i01-(OSX)-PreferencesWindow - i01
> > > 
> > > Suggestions:
> > > - "Keep Firefox Developer Edition data separate from Firefox data" or maybe
> > > - "Keep Firefox Developer Edition data and Firefox data separate"
> > > -- Don't mention profiles
> > 
> > The point of this is to allow parallel running. I think we need to mention
> > the effect on the user rather than just the cause of the problem.
> 
> So we are creating a separate profile by default to allow for parallel use,
> but AFAICT, we aren't messaging this anywhere outside of prefs; where most
> people won't look. We might want to mention this somewhere in on the product
> page or first-run.

It is mentioned in first-run.

> I guess we could say: 
> 
> "Allow Firefox Developer Edition and Firefox to run at the same time."
> "Tip: This uses separate profiles. Use Sync to share data between versions
> of Firefox. Start using Sync…"

Perfect.
Thanks.

Comment 38

[Joe Walker [:jwalker]]

There's enough talk about 'Firefox Developer Edition' in this patch that I'm taking it MoCo private. Sorry!

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Prefs screenshot

So, does this look satisfying to everyone?

Comment 40

[Matej Novak [:matej]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #39)
> Created attachment 8511097 [details]
> Prefs screenshot
> 
> So, does this look satisfying to everyone?

A couple of suggestions:

"Tip: This uses separate profiles. Use Sync to share data between them. Start using Sync…"

"Always check if Firefox Developer Edition is your default browser"

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Prefs screenshot

This is what it looks like with the latest feedback I got from Stephen and Matej. I think the patch is ready for a review, but I'm happy to make further UI tweaks if you have any more comments.

Comment 42

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 1 - Add a UI switch to disable custom profile selection process in Aurora

There is a frontend part and a platform part to this patch, so I think bsmedberg would be ideal for the platform bits and jaws for the frontend bits.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8511137 [details] [diff] [review]
part 1 - Add a UI switch to disable custom profile selection process in Aurora

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

I didn't review the changes to nsToolkitProfileService.cpp.

Mostly looks fine, r- due to the issues with main.dtd. I'd like to see what you need to do to work around the issue that I presented there.

::: browser/components/preferences/in-content/main.js
@@ +209,5 @@
> +      }
> +    }
> +
> +    // Revert the checkbox in case we didn't quit
> +    revertCheckbox();

Is this right? The 'then' fulfillment function for both the checked/unchecked tries to quit the app, but this reverts the checkbox. It looks to me like this will always revert the checkbox before the app is quit. Is that a fair interpretation of the code here?

Further, if we don't quit, it looks like revertCheckbox will get called twice. Can these two lines be removed?

@@ +213,5 @@
> +    revertCheckbox();
> +  },
> +
> +  onGetStarted: function (aEvent) {
> +    aEvent.stopPropagation();

Why is stopPropagation needed here? Either it should be removed or we should add a comment explaining its purpose.

@@ +217,5 @@
> +    aEvent.stopPropagation();
> +    const Cc = Components.classes, Ci = Components.interfaces;
> +    let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                .getService(Ci.nsIWindowMediator);
> +    let win = wm.getMostRecentWindow("navigator:browser");

in-content preferences are always going to be within a navigator:browser, so you should just be able to use window here.

@@ +220,5 @@
> +                .getService(Ci.nsIWindowMediator);
> +    let win = wm.getMostRecentWindow("navigator:browser");
> +
> +    if (win && win.document.documentElement
> +                  .getAttribute("windowtype") == "navigator:browser") {

We don't need to check the windowtype here, just make sure that `win` is truthy.

::: browser/components/preferences/in-content/main.xul
@@ +142,5 @@
>      <deck id="setDefaultPane">
>        <hbox align="center" class="indent">
>          <label id="isNotDefaultLabel" flex="1">&isNotDefault.label;</label>
>          <button id="setDefaultButton"
> +                label="&setAsMyDefaultBrowser2.label;" accesskey="&setAsMyDefaultBrowser.accesskey;"

The label and accesskey need to have the same prefix so that localization tools recognize that they are related.

::: browser/components/preferences/main.js
@@ +95,5 @@
> +        Cu.reportError("Failed to toggle separate profile mode: " + error);
> +      }
> +    }
> +
> +    const Cc = Components.classes, Ci = Components.interfaces;

Cc and Ci should already be available, no need to redeclare.

@@ +133,5 @@
> +    aEvent.stopPropagation();
> +    let win;
> +    if (document.documentElement.instantApply) {
> +      const Cc = Components.classes, Ci = Components.interfaces;
> +      // If we're in instant-apply mode, use the most recent browser window

This makes an assumption that all instantApply=true environments open their preferences in a sheet vs. a popup. Some Windows or Linux users may have instantApply=true set.

Can this be switched to just always use getMostRecentWindow?

::: browser/locales/en-US/chrome/browser/preferences/main.dtd
@@ +35,2 @@
>  <!ENTITY alwaysCheckDefault2.accesskey    "w">
> +<!ENTITY setAsMyDefaultBrowser2.label     "Make Default">

Why did the string change here? IMO, the old string was friendlier.

@@ +37,5 @@
>  <!ENTITY setAsMyDefaultBrowser.accesskey  "D">
>  <!ENTITY isDefault.label                  "&brandShortName; is currently your default browser">
>  <!ENTITY isNotDefault.label               "&brandShortName; is not your default browser">
>  
> +<!ENTITY separateProfileMode.label        "Allow &brandFullName; and Firefox to run at the same time">

&brandFullName; is not used often, as it's a bit formal: "Mozilla Firefox" versus the more frequently used &brandShortName; "Firefox".

This needs to use &brandShortName; instead of "Firefox". But &brandShortName; is "Firefox Developer Edition" in these builds, right? So you may need to figure out something else here, but we can't embed the Firefox name directly in strings like this.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +111,5 @@
>  /* General Pane */
>  
> +#useFirefoxSync  {
> +  font-size: 90%;
> +  -moz-margin-end: 8px !important;

Why is the !important needed here? Is there a different selector that is getting higher precedence? Maybe we can adjust this selector so that the !important isn't necessary (!important infects everything and sooner or later everything needs !important in order to apply).

Comment 44

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 1 - Add a UI switch to disable custom profile selection process in Aurora

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #43)
> Is this right? The 'then' fulfillment function for both the
> checked/unchecked tries to quit the app, but this reverts the checkbox. It
> looks to me like this will always revert the checkbox before the app is
> quit. Is that a fair interpretation of the code here?
> 
> Further, if we don't quit, it looks like revertCheckbox will get called
> twice. Can these two lines be removed?

The revertCheckbox call at the end of the function is required for the cases where the user cancels the restart prompt or a quit-application-requested observer does. Removing it will not quit the application, but leave the checkbox modified. I actually copied the logic from the e10s pref earlier in this file.

However, you are right that on a successful restart the checkbox briefly flashes before the app quits (I had to consciously look for it, as it is rather imperceptible). This doesn't do any harm, but it's certainly annoying enough, so I added an early return inside the inner "if (shouldProceed)" branch.

> @@ +217,5 @@
> > +    aEvent.stopPropagation();
> > +    const Cc = Components.classes, Ci = Components.interfaces;
> > +    let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> > +                .getService(Ci.nsIWindowMediator);
> > +    let win = wm.getMostRecentWindow("navigator:browser");
> 
> in-content preferences are always going to be within a navigator:browser, so
> you should just be able to use window here.
> 
> @@ +220,5 @@
> > +                .getService(Ci.nsIWindowMediator);
> > +    let win = wm.getMostRecentWindow("navigator:browser");
> > +
> > +    if (win && win.document.documentElement
> > +                  .getAttribute("windowtype") == "navigator:browser") {
> 
> We don't need to check the windowtype here, just make sure that `win` is
> truthy.

But 'window' in this file is the content window, right? In this case I want to reach out and get a reference to the tab. Is there a simpler way to do it starting from 'window'?

> ::: browser/locales/en-US/chrome/browser/preferences/main.dtd
> @@ +35,2 @@
> >  <!ENTITY alwaysCheckDefault2.accesskey    "w">
> > +<!ENTITY setAsMyDefaultBrowser2.label     "Make Default">
> 
> Why did the string change here? IMO, the old string was friendlier.

Stephen suggested we change it and Matej wasn't opposed. See comments 15 through 41.

> @@ +37,5 @@
> >  <!ENTITY setAsMyDefaultBrowser.accesskey  "D">
> >  <!ENTITY isDefault.label                  "&brandShortName; is currently your default browser">
> >  <!ENTITY isNotDefault.label               "&brandShortName; is not your default browser">
> >  
> > +<!ENTITY separateProfileMode.label        "Allow &brandFullName; and Firefox to run at the same time">
> 
> &brandFullName; is not used often, as it's a bit formal: "Mozilla Firefox"
> versus the more frequently used &brandShortName; "Firefox".
> 
> This needs to use &brandShortName; instead of "Firefox". But
> &brandShortName; is "Firefox Developer Edition" in these builds, right? So
> you may need to figure out something else here, but we can't embed the
> Firefox name directly in strings like this.

&brandShortName; will actually be FirefoxDevEdition in these builds. I only used the full name here because that's what Stephen suggested in comment 15 and it had remained uncontested in the followup discussion so far. I have changed it to &brandShortName; as it sounds reasonable to me.

I'm not sure if you are arguing for removing the bare Firefox name from the string, but if so, I think keeping it shouldn't be a problem: these strings will only be displayed when MOZ_DEV_EDITION=1, which will only apply to one specific product that will have a special relationship with the Firefox product. Indeed, this code is not designed to be of general use by embeddings or downstream consumers, and the only supported simultaneous configuration for profiles is between Firefox and FirefoxDevEdition.

That being said, if you can think of any way to use an appropriate entity name in such a configuration, I'll be happy to make the change.

> ::: browser/themes/shared/incontentprefs/preferences.css
> @@ +111,5 @@
> >  /* General Pane */
> >  
> > +#useFirefoxSync  {
> > +  font-size: 90%;
> > +  -moz-margin-end: 8px !important;
> 
> Why is the !important needed here? Is there a different selector that is
> getting higher precedence? Maybe we can adjust this selector so that the
> !important isn't necessary (!important infects everything and sooner or
> later everything needs !important in order to apply).

It is needed to override the following rule from common.inc.css:

xul|groupbox xul|label {
  /* !important needed to override toolkit !important rule */
  -moz-margin-start: 0 !important;
  -moz-margin-end: 0 !important;
}

If you know of a better way, I'm all ears.

I've made all the other suggested changes.

Comment 45

[Benjamin Smedberg]

Comment on attachment 8512023 [details] [diff] [review]
part 1 - Add a UI switch to disable custom profile selection process in Aurora

I reviewed only the nsToolkitProfileService changes. I do think that the presence of the magic file is a good way to handle this, but I also think this should be documented somewhere, probably in the .idl file. It's a magic API surface!

Comment 46

[Panos Astithas (he/him) [:past] (please ni?)]

I've added the following hunk to the patch locally:

diff --git a/toolkit/profile/nsIToolkitProfileService.idl b/toolkit/profile/nsIToolkitProfileService.idl
--- a/toolkit/profile/nsIToolkitProfileService.idl
+++ b/toolkit/profile/nsIToolkitProfileService.idl
@@ -24,16 +24,22 @@ interface nsIToolkitProfileService : nsI
      */
     attribute nsIToolkitProfile selectedProfile;
 
     /**
      * The default profile (the one used or about to be used by the
      * browser if no other profile is specified at runtime). This is the profile
      * marked with Default=1 in profiles.ini and is usually the same as
      * selectedProfile, except on Developer Edition.
+     *
+     * Developer Edition uses a profile named "dev-edition-default" as the
+     * default profile (which it creates if it doesn't exist), unless a special
+     * empty file named "ignore-dev-edition-profile" is present next to
+     * profiles.ini. In that case Developer Edition behaves the same as any
+     * other build of Firefox.
      */
     attribute nsIToolkitProfile defaultProfile;
 
     /**
      * Get a profile by name. This is mainly for use by the -P
      * commandline flag.
      *
      * @param aName The profile name to find.

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8512023 [details] [diff] [review]
part 1 - Add a UI switch to disable custom profile selection process in Aurora

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

r+, as reviewed using the Bugzilla diff tool. I got confirmation from Gavin that the "Firefox" name in the string is OK to use.

::: browser/components/preferences/in-content/main.js
@@ +220,5 @@
> +                .getService(Ci.nsIWindowMediator);
> +    let win = wm.getMostRecentWindow("navigator:browser");
> +
> +    if (win && win.document.documentElement
> +                  .getAttribute("windowtype") == "navigator:browser") {

Sorry, you're right. `window` here won't be sufficient, but we can still remove the check for windowtype == "navigator:browser".

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 1 - Add a UI switch to disable custom profile selection process in Aurora

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #47)
> Sorry, you're right. `window` here won't be sufficient, but we can still
> remove the check for windowtype == "navigator:browser".

Good point, I took it out from both places.

Comment 49

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/9f9eedf05cdc
https://hg.mozilla.org/integration/fx-team/rev/a2d602d1c723

Comment 50

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

https://hg.mozilla.org/mozilla-central/rev/9f9eedf05cdc
https://hg.mozilla.org/mozilla-central/rev/a2d602d1c723

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8512175 [details] [diff] [review]
part 1 - Add a UI switch to disable custom profile selection process in Aurora

Approval Request Comment
[Feature/regressing bug #]: new feature introduced by bug 1024110
[User impact if declined]: existing Aurora users may get confused/annoyed by the changes coming with dev-edition. This patch gives them an easy way to make aurora like it was before
[Describe test coverage new/current, TBPL]: gum, m-c, fx-team
[Risks and why]: there is a very contained risk in the preferences dialog/tab and a rather small risk (due to the size of that particular change) in the profile service code
[String/UUID change made/needed]: 3 new strings were introduced for dev-edition builds only, and another 2 strings were improved at the request of the UX team

Comment 52

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8509662 [details] [diff] [review]
part 2 - Add a flag to nsIAppStartup::quit that restarts the browser without using the same profile and use it for the Aurora UI switch

Approval Request Comment
[Feature/regressing bug #]: new feature introduced by bug 1024110
[User impact if declined]: existing Aurora users may get confused/annoyed by the changes coming with dev-edition. This patch gives them an easy way to make aurora like it was before
[Describe test coverage new/current, TBPL]: gum, m-c, fx-team
[Risks and why]: there is some risk in the profile service code that this patch touches, but QA, product and engineering folks who tested this on gum and m-c didn't find any issues
[String/UUID change made/needed]: none

Comment 53

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed with latest gum build (buildID: 20141104133445) on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5.

Comment 54

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 0 - Add strings for a UI switch to disable custom profile selection process in Aurora

This is the part of the approved patch with just the string additions that I'm going to land on Aurora ASAP. This is to make the l10n team's job easier, by not having to wait for the rest to land.

Comment 55

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eedbfc1a6c53

The part1 patch that will land soon will contain no more changes to the main.dtd file.

Comment 56

[Francesco Lodolo [:flod]]

Some more notes on this bug from a l10n point of view.

This is landing very late in the aurora cycle, which means we're going to get very few localizations.

Several strings are visible only on Aurora, but not the button to set browser as default and "Always check to see if &brandShortName; is the default browser on startup". This is really bad, because we're going to display very prominent strings in English for several locales, especially those working on external tools hooked to mozilla-aurora.

Since we're leaving the strings in the repo in Fx35, can we have a patch, to land on mozilla-beta right after merge day, that reverts changes to the General panel for Firefox 35? Then we can let the change ride the trains as usual for Firefox 36.

Comment 57

[:Gavin Sharp [email: [e-mail]]]

It's not clear to me why the alwaysCheckDefault2/setAsMyDefaultBrowser2 string changes landed on Aurora, they don't seem related to the dev-edition-specific fix here, and should be able to ride the 36 train normally.

Panos, can we back them out and split them into a separate bug?

Comment 58

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #57)
> It's not clear to me why the alwaysCheckDefault2/setAsMyDefaultBrowser2
> string changes landed on Aurora, they don't seem related to the
> dev-edition-specific fix here, and should be able to ride the 36 train
> normally.
> 
> Panos, can we back them out and split them into a separate bug?

I believe they are related as it's the longer name that makes both the button and the preceding label far too long, but I can take it out if you want.

Comment 59

[Francesco Lodolo [:flod]]

At this point we already have the string on mozilla-aurora, I'm afraid a back-out would create more noise than actually help. 

What we absolutely need is to disable the feature for Firefox 35 when it moves to beta, and keep using the old Preferences window for one more cycle.

Comment 60

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Revert string changes for beta

So is this what you need? This is based on fx-team, because aurora is still in an intermediate state, but I will rebase it on top of aurora tip next week to its final state.

Comment 61

[Francesco Lodolo [:flod]]

Comment on attachment 8518871 [details] [diff] [review]
Revert string changes for beta

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

Yes and no. The XUL part is good, but you should leave the .dtd file untouched (strings will move to beta and be unused for a cycle).

Comment 62

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Revert string changes for beta v2

OK, this is what should go to beta then?

Comment 63

[Francesco Lodolo [:flod]]

Comment on attachment 8518896 [details] [diff] [review]
Revert string changes for beta v2

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

Yes (that assuming that Gavin doesn't want to revert the change on aurora).

Comment 64

[:Gavin Sharp [email: [e-mail]]]

Landing that patch on beta sounds fine.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8162870a208
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0abb6e26c09

Comment 66

[Francesco Lodolo [:flod]]

Setting NI as a reminder that we'll need this on beta (I guess we need review and flag).

Comment 67

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8518896 [details] [diff] [review]
Revert string changes for beta v2

Thanks for the reminder Francesco.

Comment 68

[Panos Astithas (he/him) [:past] (please ni?)]

Landed the patch to revert the string changes on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/43fbd1a46c10