Joe Walker [:jwalker] Reporter
	Description • 10 years ago

      No description provided.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 1 • 10 years ago

Almost there.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 2 • 10 years ago

This is what the preference dialog looks like.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 3 • 10 years ago

This is what in-content preferences look like.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 4 • 10 years ago

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

	Francesco Lodolo [:flod]
	Comment 5 • 10 years ago

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

	Joe Walker [:jwalker] Reporter
	Comment 6 • 10 years ago

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

Open about:accounts.

	Joe Walker [:jwalker] Reporter
	Comment 7 • 10 years ago

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.

	Francesco Lodolo [:flod]
	Comment 8 • 10 years ago

(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.

	Francesco Lodolo [:flod]
	Comment 9 • 10 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 11 • 10 years ago

Small tweaks to the previous version.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 12 • 10 years ago

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.

	Benjamin Smedberg
	Comment 13 • 10 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 14 • 10 years ago

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.

	Stephen Horlander [:shorlander]
	Comment 15 • 10 years ago

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.

	Stephen Horlander [:shorlander]
	Comment 16 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 17 • 10 years ago

(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.

	Joe Walker [:jwalker] Reporter
	Comment 18 • 10 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 19 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 20 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 21 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 22 • 10 years ago

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

	Matej Novak [:matej]
	Comment 23 • 10 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 24 • 10 years ago

(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?

	Francesco Lodolo [:flod]
	Comment 25 • 10 years ago

(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?

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 26 • 10 years ago

(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.

	Matej Novak [:matej]
	Comment 28 • 10 years ago

(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?

	Matej Novak [:matej]
	Comment 29 • 10 years ago

(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.

	Francesco Lodolo [:flod]
	Comment 30 • 10 years ago

(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.

	Stephen Horlander [:shorlander]
	Comment 31 • 10 years ago

(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.

	Matej Novak [:matej]
	Comment 32 • 10 years ago

(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.

	Stephen Horlander [:shorlander]
	Comment 33 • 10 years ago

(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…"

	Stephen Horlander [:shorlander]
	Comment 34 • 10 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 35 • 10 years ago

(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?

	Stephen Horlander [:shorlander]
	Comment 36 • 10 years ago

(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" :)

	Joe Walker [:jwalker] Reporter
	Comment 37 • 10 years ago

(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.

	Joe Walker [:jwalker] Reporter
	Comment 38 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 39 • 10 years ago

So, does this look satisfying to everyone?

	Matej Novak [:matej]
	Comment 40 • 10 years ago

(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"

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 41 • 10 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 42 • 10 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 43 • 10 years ago

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).

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 44 • 10 years ago

(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.

	Benjamin Smedberg
	Comment 45 • 10 years ago

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!

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 46 • 10 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 47 • 10 years ago

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".

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 48 • 10 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 49 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 50 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 51 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 52 • 10 years ago

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

	Camelia Badau [:cbadau], Desktop QA
	Comment 53 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 54 • 10 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 55 • 10 years ago

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.

	Francesco Lodolo [:flod]
	Comment 56 • 10 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 57 • 10 years ago

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?

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 58 • 10 years ago

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.

	Francesco Lodolo [:flod]
	Comment 59 • 10 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 60 • 10 years ago

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.

	Francesco Lodolo [:flod]
	Comment 61 • 10 years ago

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).

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 62 • 10 years ago

OK, this is what should go to beta then?

	Francesco Lodolo [:flod]
	Comment 63 • 10 years ago

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).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 64 • 10 years ago

Landing that patch on beta sounds fine.

	Ryan VanderMeulen [:RyanVM]
	Comment 65 • 10 years ago

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

	Francesco Lodolo [:flod]
	Comment 66 • 10 years ago

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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 67 • 10 years ago

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

Thanks for the reminder Francesco.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 68 • 10 years ago

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