Closed Bug 861322 Opened 9 years ago Closed 9 years ago

Make Metro read Desktop's app.update.enabled and app.update.auto settings

Categories

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

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently some users have manual updates specified in Desktop Firefox options.
But we use a different profile for Metro Firefox so that option is not respected.

Adding an option in Metro preferences will not fix this because both Metro and Desktop share the same installation directory and files.

That means that some users who are expecting manual updates will get auto updated when bug 794937 lands and when the Metro browser starts to be enabled past Nightly.

I think we should take the manual update setting out of the prefs and move it into HKCU.  The HKCU would be initialized with whatever value the pref currently has, and then the pref wouldn't be used after that original initialization of the HKCU value.

I don't think this should hold up the story in bug 833182 but I think that we should create a new story or just use this as a defect on bug 833182.
Thoughts on Comment 0? It would avoid any new UX decisions.
Flags: needinfo?(robert.bugzilla)
As it stands now i a user has manual updates, the Metro browser will get auto updated but the Desktop browser will be manual.
(In reply to Brian R. Bondy [:bbondy] from comment #2)
> As it stands now i a user has manual updates, the Metro browser will get
> auto updated but the Desktop browser will be manual.

I'm unclear what the results of the proposed change would be.  Are we splitting the desktop pref out so that desktop and metro have different settings or are we making the desktop setting propagate to metro?
Currently we have a Desktop pref that applies to Desktop (Accessible through options).

And we have a Metro pref that applies to Metro (Only accessible through about:config).

I'm suggesting that we make the option in Firefox Desktop Options modify the registry instead of the profile so that both Metro an Desktop would share this same value.  We can add an option into Firefox Metro Options if we want to or just let the user change it in the desktop browser.  I think just letting desktop handle it is sufficient. 

The problem is that unlike other prefs, this one controls the updates of the installation directory which is shared by both browsers.  So if a user thinks they disabled updates, they'd still get the update of Desktop Firefox by running Metro.
We could also perhaps introduce a shared prefs location and attempt to migrate peoples existing settings to that shared prefs file.  Registry seems like the easiest option to me though.  This is a unique problem to Windows + Metro.
(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Currently we have a Desktop pref that applies to Desktop (Accessible through
> options).
> 
> And we have a Metro pref that applies to Metro (Only accessible through
> about:config).
> 
> I'm suggesting that we make the option in Firefox Desktop Options modify the
> registry instead of the profile so that both Metro an Desktop would share
> this same value.  We can add an option into Firefox Metro Options if we want
> to or just let the user change it in the desktop browser.  I think just
> letting desktop handle it is sufficient. 
> 
> The problem is that unlike other prefs, this one controls the updates of the
> installation directory which is shared by both browsers.  So if a user
> thinks they disabled updates, they'd still get the update of Desktop Firefox
> by running Metro.

OK. Thanks for the clarification. This sounds good to me. Let's make desktop the only interface and respect its setting in both browsers through the registry.
p=2 if rstrong agrees with the approach.
The pref has to be used to allow administrators to lock down the preference.
Flags: needinfo?(robert.bugzilla)
I think a solution based on our pref system is going to be needed because of comment #8 and that I don't think we want to go down the path of having a one-off just for this.
rstrong: I'm wondering where we'd store it to apply to both (or all?) profiles. Would you be against just using the pref on desktop and then also replicate it into the registry for read only access from Metro?  I can build it in such a way that we can use this same method for any read-only prefs on the Metro side.
I was thinking that might be the simplest way though there may be things I haven't thought about yet that might make that problematic.
OK thanks for the input, I'll likely go with a solution like that but will take care to make sure we're safe about not reaching a situation where registry != pref.

I'll just make this block the story in bug 833182 since that is carrying into it6 anyway.
Blocks: 833182
No longer blocks: 794937, metrov1triage
Component: General → Install/Update
OS: Windows 8 → Windows 8 Metro
Summary: Make Desktop's settings for updates per installation and stored in the registry instead of a pref → Make Metro read Desktop's app.update.mode setting
Attached patch Patch v1 (obsolete) — Splinter Review
So here's the deal with the patch:
1. Desktop Firefox, after startup is done, will copy an array of prefs to the registry (overwriting old values if they are already there).
2. Metro Firefox, after startup is done, will apply the prefs in the array to the current profile.
3. Desktop Firefox will sync up prefs to the registry as soon as they are changed.
4. There is no observer for registry changes, we have some support for detecting changed values in nsIWindowsRegKey, but the problem with the current nsIWindowsRegKey implementation is we'd have to have a timer in our front end code that checks every X minutes if changes exist.  That would be kind of ugly. 
5. All of this is extensible just by adding a single pref name to the array described in point #1.
Attachment #750536 - Flags: review?(jmathies)
Looks like all desktop installs would write to these keys. Maybe we should only write these out in desktop if the desktop browser is set as the default?
That seems logical. will do.
Comment on attachment 750536 [details] [diff] [review]
Patch v1

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

::: browser/base/content/browser.js
@@ +1279,5 @@
>      setTimeout(function () { BrowserChromeTest.markAsReady(); }, 0);
>      TelemetryTimestamps.add("delayedStartupFinished");
> +
> +#ifdef XP_WIN
> +#ifdef MOZ_METRO

lets add a nice comment here explaining what this is.

@@ +1557,5 @@
> +      }
> +    }
> +
> +    pushDesktopControlledPrefToMetro(prefName);
> +    let self = this;

You aren't using 'self'.

::: browser/metro/base/content/browser-ui.js
@@ +245,5 @@
>  
>      return uri.spec;
>    },
>  
> +  _pullDesktopControlledPrefs: function() {

nit - comment header
Attachment #750536 - Flags: review?(jmathies) → review+
Attachment #750536 - Attachment is obsolete: true
Summary: Make Metro read Desktop's app.update.mode setting → Make Metro read Desktop's app.update.enabled and app.update.auto settings
Attachment #750632 - Flags: review+
Attached patch Patch v3Splinter Review
Fix for Bc test failure due to not removing the pref observer, carrying forward r+.
Attachment #752507 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6066b9d92032
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.