866229 - Change - Add a preference for not doing updates in Metro

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Install/Update, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

Attachment: Screenshot of proposed new preference and conflicting preference

We'd like to add a preference into Desktop Firefox (or possibly Metro Firefox?) which would disable updates completely in Metro.

We'd like this preference to be enabled by default.

The purpose of this preference is because Metro updates have the chance of updating users and breaking their desktop add-ons, if the add-on was not compatible with the newer version.  On Desktop we by default check if they are compatible before updating.

The problem is that this new preference (which we want on by default) conflicts with an existing preference (which is on by default) and may give users the wrong impression about their add-ons breaking.  

We need Yuan and Asa (or possibly shorlander) to get together to determine what this preference should look like.

To help understand all of the above please see the attached screenshot for the proposed new preference.  Also circled in the screenshot is the conflicting preference.

Comment 1

[Brian R. Bondy [:bbondy]]

Flagging needinfo for Asa and Yuan as per the meeting I just had with Asa.

Comment 2

[Asa Dotzler [:asa]]

I've thought about this some more and it's really going to be tricky to communicate what's going on if we preserve the current defaults. The simplest solution I can think of that still provides for warnings for add-on bustage is to change the default to no warning but preserve the option. That would look something like this:

( ) Automatically Update desktop Firefox
    [ ] Warn me of add-on incompatibility
(o) Automatically Update desktop and Metro Firefox

If Automatically install for both radio button is selected, it would disable (grey it out or something) the Warn me checkbox. If Automatically update only desktop radio is selected then the Warn me checkbox becomes active (and checked by default?)

Something like this would let those people who are afraid of being surprised by add-on incompat receive the warning while keeping everyone else up to date and secure.

Yuan, I'd be thrilled if you could come up with something better / simpler / etc. so  please don't read this as our only option. It's just that it's somewhat different from what I'd suggested to Robert and Brian in our short pow-wow this morning so I wanted to write it down.

Comment 3

[Brian R. Bondy [:bbondy]]

I was thinking the exact same thing relating to changing the default for add-on compatibility.  Yuan if we go this route, please note what happens with disabling the compatibility option when you select the manual install updates and no updates radio buttons.

Another note is that this option will only be present on Windows 8 and above.  So we can preserve the old UI on below Windows 8.  I'd need to know if we want to change the option for all OS (which I think security would prefer), or just Windows 8.

Comment 4

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

If this UI is going to be only for Windows 8 users, we should promote updating Metro version FX in both Classic and Metro environments. Optimize our solution for what the Metro users want.

I understand why desktop users are worried about add-on compatibility on desktop. But this concern should not block them from updating the Metro version. Assuming that Metro-only update is feasible(please correct me if I'm wrong), when a compatibility warning occurs, we should provide an option for the users to just update Metro version. 

If that sounds right, then I don't think there will be any change to the setting panel, but the compatibility warning UI. So by default when auto-update is on:

In Classic:
1. When there is no compatibility issue, we update Classic and Metro version together. 
2. When there is a compatibility issue, we show the warning window, but we provide one more option: "Update Metro version Firefox only".

In Metro:
1. When there is no compatibility issue, we update Classic and Metro version together. 
2. When there is a compatibility issue, we show the warning in a Windows 8 metro style window, and offer the same options: Update Both, or Update Metro version only.

This is just a solution on a strategic level. I am happy to figure out the UI details if this solution makes sense to the team.

Comment 5

[Brian R. Bondy [:bbondy]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #4)
> If this UI is going to be only for Windows 8 users, we should promote
> updating Metro version FX in both Classic and Metro environments. Optimize
> our solution for what the Metro users want.
> 
> I understand why desktop users are worried about add-on compatibility on
> desktop. But this concern should not block them from updating the Metro
> version. 
> Assuming that Metro-only update is feasible(please correct me if
> I'm wrong), 

The problem is that there is only one update that updates both Metro and Desktop.  There is no separate updating of Metro without updating Desktop. 

We allow this update to go through if the user is using the Metro browser, or the desktop browser.
But if you update with the Metro browser it doesn't do a Desktop profile addon compatibility check warning.

Comment 6

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> (In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #4)
> > If this UI is going to be only for Windows 8 users, we should promote
> > updating Metro version FX in both Classic and Metro environments. Optimize
> > our solution for what the Metro users want.
> > 
> > I understand why desktop users are worried about add-on compatibility on
> > desktop. But this concern should not block them from updating the Metro
> > version. 
> > Assuming that Metro-only update is feasible(please correct me if
> > I'm wrong), 
> 
> The problem is that there is only one update that updates both Metro and
> Desktop.  There is no separate updating of Metro without updating Desktop. 
> 
> We allow this update to go through if the user is using the Metro browser,
> or the desktop browser.
> But if you update with the Metro browser it doesn't do a Desktop profile
> addon compatibility check warning.


Thanks for clarifying, Brian.

So the meta question here is why to separate Metro and Desktop updates? what's the value behind it? 

In my understanding, the benefits are: 
1. For Windows 8 users who are concerned about desktop add-on compatibility, they could find a way to update Metro only. 
2. More people will have up-to-date Firefox Metro version
I know it's a hard problem. But I still think it's worthy investigating whether we could update only the Metro browser. 

Thoughts on the solutions Asa and Brian proposed:
1. People who are concerned about desktop compatibility are not going to update desktop Firefox anyway. Separating desktop updates doesn't help create more value to these people. Did I miss anything here?

2. Setting panel for update is completely out of users' context. Instead of adding more controls on setting panel, we should think about the entire update process. Would it be better if we could solve the problem in context?



If I have to find a middle solution, I would prefer to always ask the users before automatically do anything for them. Otherwise, people who care about add-on compatibility will be complaining: "I updated Firefox Metro and now most of my desktop FX add-ons are not working! :( "

When the users choose to update from Metro browser, we'll need to inform our users when there is an incompatibility issue, and provide 2 options: Update Anyway, Cancel. or 3 options: "Always Update", "Update once", "Cancel".
The tradeoff is these people may not get the up-to-date version. But it was their choice, and they still have the choice to update if they want.

UI-wise, there should not be any change on the settings panel, but only the compatibility dialog window. People who are concerned about compatibility will have a choice to stay on current version. People who are not concerned, just get both browsers up-to-date, and they've made their choice to do so.

Comment 7

[Brian R. Bondy [:bbondy]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #6)

> So the meta question here is why to separate Metro and Desktop updates?
> what's the value behind it? 

It's not a choice, Desktop Firefox and Metro Firefox both start from the same files. There's no way to update one without the other.  Also they have to be the same files, there is only one default browser path.

I'll wait for a re-answer given that info.

Comment 8

[Brian R. Bondy [:bbondy]]

Just to clarify.
1. If you update from Metro, both Metro and Desktop Firefox will be updated.
2. If you update from Desktop, both Metro and Desktop Firefox will be updated.

There's no way to only update the Metro portion of the browser.

Comment 9

[Brian R. Bondy [:bbondy]]

I'm going to go ahead with Comment 2, but just flagging one last time for needinfo in case you had any objections based on Comment 7 and Comment 8 info.

Comment 10

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Based on the conversation I had with Brian, I'm okay with going for the suggestions in Comment 2.

And since it may not be clear to the users that checking "Warn me about add-on incompatibility" will disable Metro updates completely, we should use a message dialog to confirm: "To protect your add-on compatibility, this will disable updates in Windows 8 style Nightly.", with options "Ok" and "Cancel".

Just came to my mind, if the user has disabled Metro updates, we should show a text note on the Metro About flyout. Just to remind the users that Metro updates is disabled and where to change it in Classic Firefox.

Brian, does it sound like a feasible idea? Thanks!

Comment 11

[Brian R. Bondy [:bbondy]]

Sure please let me know the text to display on the Metro side of things

Comment 12

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Attachment: [Mockup] About flyout_updates disabled

When Metro updates are disabled, we will show a note on the About flyout, in order to remind the users the current state, and how to change the preference.

The texts are: You have disabled updates on Nightly. To enable updates, open desktop Firefox Nightly, then go to Options/Advanced/Update. 

Used Segoe UI, 12pt to fit the texts in 3 lines.

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #10)
> Based on the conversation I had with Brian, I'm okay with going for the
> suggestions in Comment 2.
> 
> And since it may not be clear to the users that checking "Warn me about
> add-on incompatibility" will disable Metro updates completely, we should use
> a message dialog to confirm: "To protect your add-on compatibility, this
> will disable updates in Windows 8 style Nightly.", with options "Ok" and
> "Cancel".
> 

If the user presses OK to that prompt, and we clear the update through Metro checkbox, then the user clicks on the update through Metro checkbox, what do you want to happen?  Do we clear the add-on compatibility box? Or give another prompt? If another prompt I need text for that prommpt too please.

One idea I had was just to simply disable the incompatible add-ons by default checkbox (like we do when you select manual updates already) when Metro updates is enabled.  If you disable metro updates, then the incompatible updates checkbox would enable.

Comment 14

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

I think the simplified approach you proposed makes sense.

We should inform the users that New Windows 8 style Firefox might be incompatible with your installed desktop add-ons. But I don't think we need another prompt.

So here is what I would suggest:

Nightly updates:
( ) Automatically update desktop Firefox only
    [ ] Warn me of add-on incompatibility
(o) Automatically update desktop and Windows 8 style Firefox
    (Windows 8 style Firefox updates might make your desktop add-ons incompatible)


Changes are:
1. Added "only" to the 1st option
2. Add a description below 2nd option, to indicate the risks of add-on incompatibility.
3. Use "Windows 8 style" instead of "Metro"


About the prompt:
1. A prompt will be triggered when the user clicks the radio button of "( ) Automatically update desktop Firefox only". 

2. The texts are going to be slightly different from Comment 12: 
"This will completely disable updates in Windows 8 style Nightly." 

3.If there is a way to modify the options. We should show "I Understand", and "Cancel". Choosing "I Understand" should close the prompt, and select the desktop only option. Choosing "Cancel" should close the prompt, and keep the original choice.

In the future, I would like to insert a "Learn more" link to a SUMO help article on the panel. It should aim to help the users understand reasons behind those options.


Let me know if there is any question. Thanks, Brian.

Comment 15

[Brian R. Bondy [:bbondy]]

changed this: 
Automatically update *from* desktop and Windows 8 style Firefox

Comment 16

[Brian R. Bondy [:bbondy]]

Changed:
(Windows 8 style Firefox updates might make your desktop add-ons incompatible)

to:

Windows 8 style &brandShortName; updates do not check for desktop add-on incompatibility


The problem with the first one is that it makes it sound like there is some kind of problem with Metro updates.

Comment 17

[Brian R. Bondy [:bbondy]]

Talked to Yuan and showed that it seems unnatural to have a prompt there when there is no similar prompt warning the user that updates will be disabled completely. She said it is ok to not have the prompt.

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: New option (no prompts as per discussion)

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This adds an option to the options panel and syncs it to metro.  The other review you just did will land before this one.

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 750603 [details] [diff] [review]
Patch v1.

forgot about in content prefs, will resubmit when done

Comment 21

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Comment on attachment 750599 [details]
New option (no prompts as per discussion)

We should move the default setting up as the 1st option.
Everything else looks good to me. Thanks!

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Expose Metro enabled pref

New preference for allowing updates in Metro mode.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 750691 [details] [diff] [review]
Patch v2 - Expose Metro enabled pref

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

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ +95,5 @@
>  
>  <!ENTITY updateAutoAddonWarn.label       "Warn me if this will disable any of my add-ons">
>  <!ENTITY updateAutoAddonWarn.accesskey   "W">
>  
> +<!ENTITY updateAutoMetroWarn.label       "(Windows 8 style &brandShortName; updates do not check for desktop add-on incompatibility)">

I think this is going to be too long in some languages so it'll either wrap or clip. We might want to come up with a shorter string or just get rid of it.

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Use metro enabled pref

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Expose Metro enabled pref

Changed to:
(Windows 8 style &brandShortName; does not check add-on compatibility)

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Metro update enabled pref telemetry

Comment 27

[Robert Strong (they/them - no direct email)]

(In reply to Jim Mathies [:jimm] from comment #23)
> Comment on attachment 750691 [details] [diff] [review]
> Patch v2 - Expose Metro enabled pref
> 
> Review of attachment 750691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
> @@ +95,5 @@
> >  
> >  <!ENTITY updateAutoAddonWarn.label       "Warn me if this will disable any of my add-ons">
> >  <!ENTITY updateAutoAddonWarn.accesskey   "W">
> >  
> > +<!ENTITY updateAutoMetroWarn.label       "(Windows 8 style &brandShortName; updates do not check for desktop add-on incompatibility)">
> 
> I think this is going to be too long in some languages so it'll either wrap
> or clip. We might want to come up with a shorter string or just get rid of
> it.
Agreed. Perhaps something simpler along the lines of "will not warn if the update will disable any add-ons".

Comment 28

[Brian R. Bondy [:bbondy]]

Already changed and got yuan's approval in Comment 25.

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 750713 [details] [diff] [review]
Patch v1 - Use metro enabled pref

>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
>--- a/toolkit/mozapps/update/content/updates.js
>+++ b/toolkit/mozapps/update/content/updates.js
>@@ -14,16 +14,17 @@ const CoI = Components.interfaces;
> const CoR = Components.results;
> 
> const XMLNS_XUL               = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> 
> const PREF_APP_UPDATE_BACKGROUNDERRORS   = "app.update.backgroundErrors";
> const PREF_APP_UPDATE_BILLBOARD_TEST_URL = "app.update.billboard.test_url";
> const PREF_APP_UPDATE_CERT_ERRORS        = "app.update.cert.errors";
> const PREF_APP_UPDATE_ENABLED            = "app.update.enabled";
>+const PREF_APP_UPDATE_METRO_ENABLED       = "app.update.metro.enabled";
This isn't used in this diff.

> const PREF_APP_UPDATE_LOG                = "app.update.log";
> const PREF_APP_UPDATE_MANUAL_URL         = "app.update.url.manual";
> const PREF_APP_UPDATE_NEVER_BRANCH       = "app.update.never.";
> const PREF_APP_UPDATE_TEST_LOOP          = "app.update.test.loop";
> const PREF_PLUGINS_UPDATEURL             = "plugins.update.url";
> 
> const PREF_EM_HOTFIX_ID                  = "extensions.hotfix.id";
> 
>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -646,16 +647,26 @@ XPCOMUtils.defineLazyGetter(this, "gCanC
>   // If the administrator has locked the app update functionality
>   // OFF - this is not just a user setting, so disable the manual
>   // UI too.
>   var enabled = getPref("getBoolPref", PREF_APP_UPDATE_ENABLED, true);
>   if (!enabled && Services.prefs.prefIsLocked(PREF_APP_UPDATE_ENABLED)) {
>     LOG("gCanCheckForUpdates - unable to automatically check for updates, " +
>         "disabled by pref");
>     return false;
>+
>+  let metroUtils = Cc["@mozilla.org/windows-metroutils;1"].
>+                   createInstance(Ci.nsIWinMetroUtils);
>+  if (metroUtils && metroUtils.immersive) {
>+    let metroUpdate getPref("getBoolPref", PREF_APP_UPDATE_METRO_ENABLED, true);
>+    if (!metroUpdate) {
>+      LOG("gCanCheckForUpdates - unable to automatically check for metro " +
>+          "updates, disabled by pref");
>+      return false;
>+    }
>   }
Why not ifdef this and other cases since this file is preprocessed?

Also, you lost a closing bracket.

>...
>@@ -3358,16 +3380,25 @@ Checker.prototype = {
>     this._request = null;
>   },
> 
>   /**
>    * Whether or not we are allowed to do update checking.
>    */
>   _enabled: true,
>   get enabled() {
>+    let metroUtils = Cc["@mozilla.org/windows-metroutils;1"].
>+                   createInstance(Ci.nsIWinMetroUtils);
nit: indentation

>+    if (metroUtils && metroUtils.immersive) {
>+      let metroUpdate getPref("getBoolPref", PREF_APP_UPDATE_METRO_ENABLED, true);
>+      if (!metroUpdate) {
>+        return false;
>+      }
>+    }
>+
>     return getPref("getBoolPref", PREF_APP_UPDATE_ENABLED, true) &&
>            gCanCheckForUpdates && this._enabled;
>   },
> 
>   /**
>    * See nsIUpdateService.idl
>    */
>   stopChecking: function UC_stopChecking(duration) {

Comment 30

[Brian R. Bondy [:bbondy]]

> Also, you lost a closing bracket.

Sorry that was fixed locally.

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 750713 [details] [diff] [review]
Patch v1 - Use metro enabled pref

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -646,16 +647,26 @@ XPCOMUtils.defineLazyGetter(this, "gCanC
>   // If the administrator has locked the app update functionality
>   // OFF - this is not just a user setting, so disable the manual
>   // UI too.
>   var enabled = getPref("getBoolPref", PREF_APP_UPDATE_ENABLED, true);
>   if (!enabled && Services.prefs.prefIsLocked(PREF_APP_UPDATE_ENABLED)) {
>     LOG("gCanCheckForUpdates - unable to automatically check for updates, " +
>         "disabled by pref");
>     return false;
>+
>+  let metroUtils = Cc["@mozilla.org/windows-metroutils;1"].
>+                   createInstance(Ci.nsIWinMetroUtils);
>+  if (metroUtils && metroUtils.immersive) {
>+    let metroUpdate getPref("getBoolPref", PREF_APP_UPDATE_METRO_ENABLED, true);
>+    if (!metroUpdate) {
>+      LOG("gCanCheckForUpdates - unable to automatically check for metro " +
>+          "updates, disabled by pref");
>+      return false;
>+    }
>   }
It also looks like this new code block is repeated a couple of times... might make sense to add a function for it.

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3' - Expose Metro enabled pref

Rebased.
Robert I thought you might want to do a quick pass on this too, Jim already reviewed it, but just wanted to pass the pref change in firefox.js in particular through you.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Use metro enabled pref

Implemented review comments

Comment 34

[Marco Mucci [:MarcoM]]

Carrying over to Iteration #8.

Comment 35

[Robert Strong (they/them - no direct email)]

Comment on attachment 752914 [details] [diff] [review]
Patch v3' - Expose Metro enabled pref

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js
>--- a/browser/components/preferences/advanced.js
>+++ b/browser/components/preferences/advanced.js
>@@ -562,20 +562,31 @@ var gAdvancedPane = {
>    *                   ii    *f*    t/f     *true*
>    *                   iii   0/1/2  f       false
>    *                   iii   0/1/2  *t*     *true*
>    */
>   updateReadPrefs: function ()
>   {
>     var enabledPref = document.getElementById("app.update.enabled");
>     var autoPref = document.getElementById("app.update.auto");
>+#ifdef XP_WIN
>+#ifdef MOZ_METRO
>+    var metroEnabledPref = document.getElementById("app.update.metro.enabled");
>+#endif
>+#endif
>     var radiogroup = document.getElementById("updateRadioGroup");
> 
>     if (!enabledPref.value)   // Don't care for autoPref.value in this case.
>-      radiogroup.value="manual"     // 3. Never check for updates.
>+      radiogroup.value="manual";    // 3. Never check for updates.
>+#ifdef XP_WIN
>+#ifdef MOZ_METRO
>+    else if (metroEnabledPref.value)  // enabledPref.value && autoPref.value && metroEnabledPref.value
>+      radiogroup.value="autoMetro"; // 0. Automatically install updates
>+#endif
>+#endif
>     else if (autoPref.value)  // enabledPref.value && autoPref.value
>       radiogroup.value="auto";      // 1. Automatically install updates
You have comments as follow:
0. Automatically install updates
1. Automatically install updates

Please extend the comments to differentiate between them as you did elsewhere.


>diff --git a/browser/components/preferences/in-content/advanced.xul b/browser/components/preferences/in-content/advanced.xul
>--- a/browser/components/preferences/in-content/advanced.xul
>+++ b/browser/components/preferences/in-content/advanced.xul
>@@ -78,16 +78,21 @@
>  <!-- Update tab -->
> #ifdef MOZ_UPDATER
>   <preference id="app.update.enabled"
>               name="app.update.enabled"
>               type="bool"/>
>   <preference id="app.update.auto"
>               name="app.update.auto"
>               type="bool"/>
>+#ifdef XP_WIN
>+#ifdef MOZ_METRO
>+  <preference id="app.update.metro.enabled" name="app.update.metro.enabled"        type="bool"/>
Please use the same style for this line as others.

>diff --git a/browser/metro/profile/metro.js b/browser/metro/profile/metro.js
>--- a/browser/metro/profile/metro.js
>+++ b/browser/metro/profile/metro.js
>@@ -411,16 +411,24 @@ pref("app.update.auto", true);
> // AUM Set to:        Minor Releases:     Major Releases:
> // 0                  download no prompt  download no prompt
> // 1                  download no prompt  download no prompt if no incompatibilities
> // 2                  download no prompt  prompt
> //
> // See chart in nsUpdateService.js source for more details
> pref("app.update.mode", 0);
> 
>+#ifdef XP_WIN
>+#ifdef MOZ_METRO
>+// Enables update checking in the Metro environment.
>+// add-on incompatibilities are ignored by updates in Metro.
>+pref("app.update.metro.enabled", true);
>+#endif
>+#endif
>+
I think I'm ok with this but it seems weird that these ifdefs are here... seems to me that this file wouldn't be included unless both of the above were already true.

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Expose Metro enabled pref

Implemented rstrong's comments. Carrying forward r+.

Comment 37

[Robert Strong (they/them - no direct email)]

So, this UI is displayed on non Metro systems which is a little confusing. You might want to check Windows version and only display it in Windows 8 and above.

Comment 38

[Brian R. Bondy [:bbondy]]

Good point, will do in another patch in this bug.

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 752917 [details] [diff] [review]
Patch v2 - Use metro enabled pref

>diff --git a/browser/metro/base/content/aboutPanel.js b/browser/metro/base/content/aboutPanel.js
>--- a/browser/metro/base/content/aboutPanel.js
>+++ b/browser/metro/base/content/aboutPanel.js
>@@ -158,17 +158,18 @@ appUpdater.prototype =
>   get updateDisabledAndLocked() {
>     return !this.updateEnabled &&
>            Services.prefs.prefIsLocked("app.update.enabled");
>   },
> 
>   // true when updating is enabled.
>   get updateEnabled() {
>     try {
>-      return Services.prefs.getBoolPref("app.update.enabled");
>+      return Services.prefs.getBoolPref("app.update.enabled") &&
>+             Services.prefs.getBoolPref("app.update.metro.enabled");
I am slightly concerned that if either of these prefs are missing it defaults to true. For example, if app.update.enabled is missing and app.update.metro.enabled is false then it defaults to true.

>     }
>     catch (e) { }
>     return true; // Firefox default is true
>   },
> 
>   // true when updating in background is enabled.
>   get backgroundUpdateEnabled() {
>     return this.updateEnabled &&
>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>...
>@@ -637,27 +638,50 @@ XPCOMUtils.defineLazyGetter(this, "gCanS
>         "browser is already handling updates for this installation.");
>     return false;
>   }
> 
>   LOG("gCanStageUpdates - able to stage updates");
>   return true;
> });
> 
>+XPCOMUtils.defineLazyGetter(this, "gCanCheckForUpdatesIfMetro", function aus_gCanCheckForMetroUpdates() {
So, gCanCheckForUpdates is for whether it is possible to check for updates and this is for whether Metro updates are enabled.

Please name this
gMetroUpdatesEnabled

Please mirror that name to aus_gMetroUpdatesEnabled

>+#ifdef XP_WIN
>+#ifdef MOZ_METRO
>+  let metroUtils = Cc["@mozilla.org/windows-metroutils;1"].
>+                   createInstance(Ci.nsIWinMetroUtils);
>+  if (metroUtils && metroUtils.immersive) {
>+    let metroUpdate getPref("getBoolPref", PREF_APP_UPDATE_METRO_ENABLED, true);
>+    if (!metroUpdate) {
>+      LOG("gCanCheckForUpdates - unable to automatically check for metro " +
>+          "updates, disabled by pref");
Please use the actual function name in the call to log... now gMetroUpdatesEnabled

>+      return false;
>+    }
>+  }
>+#endif
>+#endif

r=me with that

Comment 40

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Use metro enabled pref

Review comments implemented

Comment 41

[Brian R. Bondy [:bbondy]]

Comment on attachment 754874 [details] [diff] [review]
Patch v3 - Use metro enabled pref

Meant to mark this as +, carrying forward r+.

Comment 42

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Hide Metro prefs pre win8

Comment 43

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Hide Metro prefs pre win8

Also cover not touching addonIncompatible based on metro pref if the metro pref is not displayed.

Comment 44

[Brian R. Bondy [:bbondy]]

Robert I checked what happens with updatePrompt._showUI, nothing shows and an error gets logged on the console about MetroWidget.cpp window doesn't exist.  I'm going to go ahead with that patch I discussed with you about the default handling of app.update.silent w/ immersive mode.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Don't skip updates that want showPrompt for metro

I think we eventually want to implement our own update prompt code that works on Metro, but I think that is well beyond the scope of this bug.  If you agree I'll post for this, but it probably won't be in v1 unless you're completely against it.

Comment 46

[Robert Strong (they/them - no direct email)]

Comment on attachment 755348 [details] [diff] [review]
Patch v2 - Hide Metro prefs pre win8

You should be able to use document.getElementById(id).hidden = true or document.getElementById(id).collapsed = true

r=me with that

Comment 47

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Hide Metro prefs pre win8

Review comments implemented

Comment 48

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Use metro enabled pref

Fixed bug with app.update.metro.enabled = false not being respected in metro front end code. Carrying forward r+.

Comment 49

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/fa1e4a308527
https://hg.mozilla.org/mozilla-central/rev/241aedb647cf
https://hg.mozilla.org/mozilla-central/rev/afd8afc7a840
https://hg.mozilla.org/mozilla-central/rev/0f4d868fd43e
https://hg.mozilla.org/mozilla-central/rev/920669704476

Comment 50

[juan becerra [:juanb]]

Under what conditions should we see the flyout text described in comment #12 if at all? I haven't been able to see that after going through the variations in the options/advanced/update preferences in the desktop version.

Still testing variations for verification.

Comment 51

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to juan becerra [:juanb] from comment #50)
> Under what conditions should we see the flyout text described in comment #12
> if at all? I haven't been able to see that after going through the
> variations in the options/advanced/update preferences in the desktop version.
> 
> Still testing variations for verification.

This flyout should be visible when the users have updates on desktop only. The goal of this message is to inform the users that Metro FX is not update-to-date, and provide the path to change their preferences if they need to. 


Brian, I think we need to replace the string "Nightly is being updated by another instance" here with the strings in that mockup.

Comment 52

[Brian R. Bondy [:bbondy]]

Nighlty is being updated by another instance is something different, it only allows one of the 2 browsers that are open to take responsibility for doing the update, and the other gives that message.  For the string in the image (About flyout_updates disabled) please post a followup bug.

Comment 53

[Samvedana (:Samvedana)]

Attachment: Settings page in Firefox on Windows 8.1

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130708 Firefox/25.0
Build ID: 20130708031114

Tested for iteration-9 on Windows 8.1 preview using latest nightly build from ftp://ftp.mozilla.org/pub/firefox/nightly/2013/07/2013-07-08-03-11-14-mozilla-central/

I opened Firefox in Desktop. Options>Advanced>Update
I don't see settings attached here. Advanced settings are different here. I can not understand if it is enough or not.

Comment 54

[Brian R. Bondy [:bbondy]]

why are all of your update settings disabled?

Comment 55

[Samvedana (:Samvedana)]

(In reply to Brian R. Bondy [:bbondy] from comment #54)
> why are all of your update settings disabled?

Brian, I didn't disabled them. When I open Options>Advanced>Update, I see disabled settings, even I am not able to change any settings in Update.

Comment 56

[Brian R. Bondy [:bbondy]]

Well I don't think you can properly test this task without being able to change the update settings, and I don't think the update settings being disabled are a regression caused here. 

I think it would be good to figure out why they are disabled and post a bug for the text under the radio button to also be disabled.  Please also confirm that old builds unrelated to this work have the same result.

It would be good to get in a state where the options are not disbled to test this.

Comment 57

[Samvedana (:Samvedana)]

User Agent : Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130701 Firefox/25.0
Build ID : 20130701031115

User Agent : Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130702 Firefox/25.0
Build ID :  20130702031131

User Agent  Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130708 Firefox/25.0
Build ID : 20130708031114

User Agent : Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130704 Firefox/25.0
Build ID : 20130704031323

User Agent : Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130708 Firefox/25.0
Build ID : 20130708031114

User Agent  Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130710 Firefox/25.0
Build ID : 20130710030205

These builds are working fine for Options>Advanced>Update.
I don't see disabled settings.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Samvedana, please see the following document which describes how to find the right regression window. What we need are two changesets for one build that works and the next one which is broken.

https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/finding-a-regression-window/

Comment 59

[Samvedana (:Samvedana)]

(In reply to Henrik Skupin (:whimboo) from comment #58)
> Samvedana, please see the following document which describes how to find the
> right regression window. What we need are two changesets for one build that
> works and the next one which is broken.
> 
> https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/
> finding-a-regression-window/

Hi Henrik,
There is no regression here. I took wrong build to test.
Thanks for the document!

Comment 60

[Mihaela Velimiroviciu (:mihaelav)]

I (In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #51)
> (In reply to juan becerra [:juanb] from comment #50)
> > Under what conditions should we see the flyout text described in comment #12
> > if at all? I haven't been able to see that after going through the
> > variations in the options/advanced/update preferences in the desktop version.
> > 
> > Still testing variations for verification.
> 
> This flyout should be visible when the users have updates on desktop only.
> The goal of this message is to inform the users that Metro FX is not
> update-to-date, and provide the path to change their preferences if they
> need to. 
> 
> 
> Brian, I think we need to replace the string "Nightly is being updated by
> another instance" here with the strings in that mockup.

I am always getting the "Nightly is being updated by another instance" message in the Metro about flyout, no matter what option I select in the desktop Options>Advanced>Update, even after restarts both in metro and desktop mode.
Tested with July the 9th (Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130709 Firefox/25.0) and the 10th (Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130710 Firefox/25.0) Nightlies (fresh profile)

Comment 61

[Brian R. Bondy [:bbondy]]

I think that's because the desktop browser is open, close both the desktop and metro browser, then open the metro browser, does that get you past the error?

Comment 62

[Mihaela Velimiroviciu (:mihaelav)]

With desktop Nightly closed, i get the "Check for Updates" button in Metro about flyout for each update option I select. Steps:
1. Start desktop Nightly
2. Select update option
3. Close desktop Nightly
4. Launch Metro Nightly
5. Go to Settings>About.

If I let the desktop Nightly open - Steps:
1. Start desktop Nightly
2. Select update option
3. Restart desktop Nightly
4. Launch Metro Nightly
5. Go to Settings>About.
- I get the following cases:
1. For "Automatically update from desktop and Windows 8 style Nightly" - in metro mode it automatically checks for updates
2. For "Automatically install updates from desktop Nightly" - in metro mode i get the message "Nightly is being updated by another instance"
3. For the other 2 options I get the "Check for updates" button

Also, I noticed that the update options are grayed out (like in the attachment from comment #53") when metro Nightly is running and you open the panel in desktop Nightly.

Are all these intended?

Comment 63

[Brian R. Bondy [:bbondy]]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #62)
> With desktop Nightly closed, i get the "Check for Updates" button in Metro
> about flyout for each update option I select. Steps:
> 1. Start desktop Nightly
> 2. Select update option
> 3. Close desktop Nightly
> 4. Launch Metro Nightly
> 5. Go to Settings>About.

Are you sure that the metro browser was closed at step 3.5? The preferences are only synced to metro on each metro app startup.

> If I let the desktop Nightly open - Steps:
> 1. Start desktop Nightly
> 2. Select update option
> 3. Restart desktop Nightly
> 4. Launch Metro Nightly
> 5. Go to Settings>About.
> - I get the following cases:
> 1. For "Automatically update from desktop and Windows 8 style Nightly" - in
> metro mode it automatically checks for updates
> 2. For "Automatically install updates from desktop Nightly" - in metro mode
> i get the message "Nightly is being updated by another instance"

Because I think you left the desktop browser open

> 3. For the other 2 options I get the "Check for updates" button
> 

That is expected.

> Also, I noticed that the update options are grayed out (like in the
> attachment from comment #53") when metro Nightly is running and you open the
> panel in desktop Nightly.
> 
> Are all these intended?

No please post a bug for that with steps.

Comment 64

[Brian R. Bondy [:bbondy]]

To close the metro app by the way go into the metro mode app and drag from the top of the window down to the bottom of the screen.

Comment 65

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Brian R. Bondy [:bbondy] from comment #63)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #62)
> > With desktop Nightly closed, i get the "Check for Updates" button in Metro
> > about flyout for each update option I select. Steps:
> > 1. Start desktop Nightly
> > 2. Select update option
> > 3. Close desktop Nightly
> > 4. Launch Metro Nightly
> > 5. Go to Settings>About.
> 
> Are you sure that the metro browser was closed at step 3.5? The preferences
> are only synced to metro on each metro app startup.
> 

I retested and, indeed, the results are different:
For update option:
1. "Automatically update from desktop and Windows 8 style Nightly" - it checks for updates and downloads the updates if available; if not, it displays the message "Nightly is up to date". After downloading the update, it displays the "Restart to Update" button which, when pressed, restarts the browser (in metro mode) and the update is applied.
2. all other - it displays the "Check for Updates" button

> > Also, I noticed that the update options are grayed out (like in the
> > attachment from comment #53") when metro Nightly is running and you open the
> > panel in desktop Nightly.
> > 
> > Are all these intended?
> 
> No please post a bug for that with steps.

It's weird, I cannot reproduce anymore :-/ I'll try again tomorrow

Another question here: If I update Firefox in desktop mode, having the metro one also running,  and restart, only the desktop version is restarted and updated. Is this the designed behaviour?

Comment 66

[Brian R. Bondy [:bbondy]]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #65)
> Another question here: If I update Firefox in desktop mode, having the metro
> one also running,  and restart, only the desktop version is restarted and
> updated. Is this the designed behaviour?

Yes that is correct, the user may be in the middle of something important in the browser they didn't initiate the upgrade from. Next time they restart it the new version should be in effect though.