This is important for enterprise. Setting browser.pocket.enabled to false doesn't appear to work anymore in distribution.ini or the CCK2. Going into about:config shoes the preferences as false, but pocket is still there and working. To recreate, create a distribution/distribution.ini file: [Global] id=test version=1.0 about=test [Preferences] # Don't use pocket browser.pocket.enabled=false Start Firefox with a new profile. Pocket is enabled.

What version of Firefox? As of 46, pocket is a system addon. The addon should be upgrading that pref value to the new pref name extensions.pocket.enabled if the user previously set it. I would have assumed that distributions.js beats addons so the pref should be upgraded. In any case, it is probably useful to also set extensions.pocket.enabled=false

It was Firefox 45 actually. What's odd is that it's not enabled, but the icon still shows up. When you click on the icon, it doesn't work. I'll do some more testing.

Not terribly surprised. The question becomes, do we fix for 45? Not sure it could even be fixed for 45 at this stage.

The pref still works fine, just not in distribution.ini and other places. I'd like to at least understand the why on this. Hopefully something simple. Especially since FF45 is the ESR.

I think we might have a bigger problem here. Previously, when this code was executed: http://mxr.mozilla.org/mozilla-beta/source/browser/components/customizableui/CustomizableWidgets.jsm#1222 distribution.ini had been read in. For some reason now it is not. This could mean differences in partner distribution loading. I'm researching.

This regressed between: 2015-11-30-03-02-28-mozilla-central and 2015-12-01-03-02-26-mozilla-central 2015-11-30-03-02-28-mozilla-central was built from: https://hg.mozilla.org/mozilla-central/rev/47b49b0d32360fab04b11ff9120970979c426911 2015-12-01-03-02-26-mozilla-central was built from: https://hg.mozilla.org/mozilla-central/rev/66a6d7ec9534b9d7847b665142fef0dd87623768

The first bad revision is: changeset: 274524:722d3a01dcfc user: Mark Banner <standard8@mozilla.com> date: Sun Nov 29 17:08:35 2015 +0000 summary: Bug 1223573 - Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again. r=mikedeboer

So specifically, this call: http://mxr.mozilla.org/mozilla-beta/source/browser/extensions/loop/bootstrap.js#815 Is causing CustomizableUI to get initialized much earlier than it used to.

(In reply to Mike Kaply [:mkaply] from comment #8) > So specifically, this call: > > http://mxr.mozilla.org/mozilla-beta/source/browser/extensions/loop/bootstrap. > js#815 > > Is causing CustomizableUI to get initialized much earlier than it used to. It might make sense to have CUI dispatch an observer notification add-ons can listen to to add their widget? Not sure how that deals with installing restartless add-ons post-startup. I guess they could detect whether CUI is loaded using Cu.isModuleLoaded?

Conversing with Mike in irc. seems to be multiple issues related to this. - CUI initialized prior to distribution customization modifying prefs - In 46, Pocket addon doesn't respect a previously set default pref - Loop addon doesn't appear to respect a previously set default pref - Loop creates the widget even if loop.enabled=false - Loop problem exists in 45, which is next ESR, unknown if enterprises are disabling loop via autoconf

This particular patch would need uplift to beta to fix ESR/autoconf/distribution.ini

Regression for Loop and Pocket, looks like a regression from last November/December in 45. Tracking and marking affected.

(In reply to :Gijs Kruitbosch from comment #9) > (In reply to Mike Kaply [:mkaply] from comment #8) > > So specifically, this call: > > > > http://mxr.mozilla.org/mozilla-beta/source/browser/extensions/loop/bootstrap. > > js#815 > > > > Is causing CustomizableUI to get initialized much earlier than it used to. > > It might make sense to have CUI dispatch an observer notification add-ons > can listen to to add their widget? Not sure how that deals with installing > restartless add-ons post-startup. I guess they could detect whether CUI is > loaded using Cu.isModuleLoaded? I think it would be good to ensure the distribution pref customization prior to addonmanager loading addons. Seems that would address one part of the problem. The second part is that an addon can set a default pref overwriting what distribution wants. Maybe the distribution prefs could be locked until after addon startup is complete.

(In reply to Shane Caraveo (:mixedpuppy) from comment #14) > I think it would be good to ensure the distribution pref customization prior > to addonmanager loading addons. Seems that would address one part of the > problem. The second part is that an addon can set a default pref > overwriting what distribution wants. Maybe the distribution prefs could be > locked until after addon startup is complete. Even if they were locked, an add-on could unlock and set, so I don't know that would buy us much. Unfortunately we really never built a good default prefs story for restartless add-ons, but we're stuck with what we have. The key for us is when we switch built in stuff to be add-ons, we need to be smart about our default prefs (and encourage add-on others to do the same). And we should make sure we have a good default pref story in WebExtensions.

(In reply to Mike Kaply [:mkaply] from comment #15) > (In reply to Shane Caraveo (:mixedpuppy) from comment #14) > > > I think it would be good to ensure the distribution pref customization prior > > to addonmanager loading addons. Seems that would address one part of the > > problem. The second part is that an addon can set a default pref > > overwriting what distribution wants. Maybe the distribution prefs could be > > locked until after addon startup is complete. > > Even if they were locked, an add-on could unlock and set, so I don't know > that would buy us much. I think that's ok. It becomes an explicit decision to do that, rather than an oversight.

Comment on attachment 8723219 [details] [diff] [review] fix loop pref use Review of attachment 8723219 [details] [diff] [review]: ----------------------------------------------------------------- I'm moving back to desktop - deferring to Mark, if you don't mind, Shane!

Comment on attachment 8723219 [details] [diff] [review] fix loop pref use Review of attachment 8723219 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/loop/bootstrap.js @@ +955,5 @@ > function loadDefaultPrefs() { > var branch = Services.prefs.getDefaultBranch(""); > Services.scriptloader.loadSubScript("chrome://loop/content/preferences/prefs.js", { > pref: (key, val) => { > + // if a previously set default exists, don't overwrite (e.g. distribution.ini) nit: please make the comments into proper sentences (capital & full stop). @@ +957,5 @@ > Services.scriptloader.loadSubScript("chrome://loop/content/preferences/prefs.js", { > pref: (key, val) => { > + // if a previously set default exists, don't overwrite (e.g. distribution.ini) > + if (branch.getPrefType(key) != branch.PREF_INVALID) > + return; nit: please include braces for all of the if statements you've added as per the prevailing style. @@ +1037,5 @@ > + > + loadDefaultPrefs(); > + > + // watch pref change and enable/disable if necessary > + Services.prefs.addObserver("loop.enabled", prefObserver, false); We need a way to remove the observer on real shutdown of the add-on. Otherwise we might have references left hanging around that'll confuse things on upgrade. @@ +1038,5 @@ > + loadDefaultPrefs(); > + > + // watch pref change and enable/disable if necessary > + Services.prefs.addObserver("loop.enabled", prefObserver, false); > + if (Services.prefs.getPrefType("loop.enabled") == Services.prefs.PREF_INVALID || nit: only one space before ==

Comment on attachment 8723218 [details] [diff] [review] fix pocket pref use Review of attachment 8723218 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/pocket/bootstrap.js @@ +522,5 @@ > Services.prefs.clearUserPref("browser.pocket.enabled"); > } > // watch pref change and enable/disable if necessary > Services.prefs.addObserver("extensions.pocket.enabled", prefObserver, false); > + if (Services.prefs.getPrefType("extensions.pocket.enabled") == Services.prefs.PREF_INVALID || This could only happen if something removed the pref from the default branch in between us adding it and us now checking it, but all that code is sync, right? So presumably we could just remove this check? If there's some reason to keep it, then nit: one space before "==".

This is a minimal patch to support autoconfig and distribution.ini prefs. Changing loop.enabled requires a restart unlike the larger patch.

address earlier review comments.

Comment on attachment 8723268 [details] [diff] [review] fix loop pref use Review of attachment 8723268 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/loop/bootstrap.js @@ +1041,5 @@ > + > + // Watch pref changes and enable/disable if necessary. > + Services.prefs.addObserver("loop.enabled", prefObserver, false); > + if (Services.prefs.getPrefType("loop.enabled") == Services.prefs.PREF_INVALID || > + !Services.prefs.getBoolPref("loop.enabled")) nit: missing braces for the if statement. @@ +1079,5 @@ > // it fade away... > if (reason == APP_SHUTDOWN) { > return; > } > + Services.prefs.removeObserver("loop.enabled", prefObserver); We probably want to make this conditional on if shutdown is being called from the prefObserver or not - otherwise the loop.enabled would toggle off in restartless fashion, but not toggle on again.

per irc, landing the smaller loop patch in case it is accepted for uplift. we'll work out the larger patch later.

https://hg.mozilla.org/integration/fx-team/rev/53b10ff5a66259400a440e03a135395a397f9962 Bug 1245277 pocket addon needs to respect prior set default prefs, r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/f184bfcbbc460851820579855b40d64ac621384c Bug 1245277 loop addon needs to respect prior set default prefs, not restartless, r=Standard8

Comment on attachment 8723267 [details] [diff] [review] fix loop pref use, smaller patch for uplift uplift for aurora, beta and ESR45 which is already branched but no flag here to request it. Approval Request Comment [Feature/regressing bug #]:loop system addon [User impact if declined]:autoconfig, distriubtion.ini, ESR etc. are unable to pref off our system addons [Describe test coverage new/current, TreeHerder]: [Risks and why]: late beta is always high. low for aurora. [String/UUID change made/needed]:none

This is the patch that landed with prior review comments addressed.

Comment on attachment 8723282 [details] [diff] [review] fix pocket pref use Approval Request Comment [Feature/regressing bug #]:pocket system addon [User impact if declined]:autoconfig, distriubtion.ini, ESR etc. are unable to pref off our system addons [Describe test coverage new/current, TreeHerder]: [Risks and why]: low for aurora. [String/UUID change made/needed]:none

https://hg.mozilla.org/mozilla-central/rev/53b10ff5a662 https://hg.mozilla.org/mozilla-central/rev/f184bfcbbc46

I pushed the Loop patch to the loop repo as well. Shane, sorry I forgot to attribute you on there: https://github.com/mozilla/loop/commit/07fac0925ec62b54c8d9e900f20df07f734be4eb

Comment on attachment 8723267 [details] [diff] [review] fix loop pref use, smaller patch for uplift We indeed want this in 45, taking it. Should be in 45 beta 10

https://hg.mozilla.org/releases/mozilla-aurora/rev/f29d0f7b017e https://hg.mozilla.org/releases/mozilla-beta/rev/30883d70b037

Does ESR 45 still use the beta branch or has it branched separately?

(In reply to Mike Kaply [:mkaply] from comment #33) > Does ESR 45 still use the beta branch or has it branched separately? It hasn't branched yet. I think it only branches when mozilla-beta gets merged into mozilla-release. In any case, for questions like this, ask on IRC, mailing lists, or needinfo someone. Just commenting in bugs isn't an effective way of asking a question like that.

Standard8, I think the larger patch I initially proposed should still be considered for loop, though the shutdown aspect needs a bit more thought. Can that just pass on to someone one the loop team?

(In reply to Shane Caraveo (:mixedpuppy) from comment #35) > Standard8, I think the larger patch I initially proposed should still be > considered for loop, though the shutdown aspect needs a bit more thought. > Can that just pass on to someone one the loop team? That would be fine, can I talk you into filing a bug in the Loop product with an outline, we can then schedule it in.

I used distribution.ini file based on comment 0 (with loop.enabled pref added as false) but I can see that both old Nightly (2015-12-01 as reported in comment 6) and Firefox 45 beta 10 behave the same: Pocket does not go away from Toolbar but Hello does, even though both prefs are successfully modified to 'false'. Am I missing something?

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #37) > I used distribution.ini file based on comment 0 (with loop.enabled pref > added as false) but I can see that both old Nightly (2015-12-01 as reported > in comment 6) and Firefox 45 beta 10 behave the same: Pocket does not go > away from Toolbar but Hello does, even though both prefs are successfully > modified to 'false'. > Am I missing something? a) I dont see that the pocket patch got uplifted, so only verify pocket on nightly b) the pref for pocket is extensions.pocket.enabled

> the pref for pocket is extensions.pocket.enabled To clarify, this is starting with Firefox 46

Comment on attachment 8723282 [details] [diff] [review] fix pocket pref use OK for uplift

Sorry, looks like we missed the loop patch. Did that make it into esr? Esr did already branch by the way.

Oh, not the loop one. Your 2nd pocket patch. approving it now, anyway, for aurora.

I beleive the loop patch made it, just the pocket patch needs to get into 46

Based on comment 29, this is fixed in Fx 47 as well (when it was on Nightly channel).

Is it fixed in the new release 46? A user reported the same issue with extensions.pocket.enabled ignored in /usr/lib/firefox/browser/defaults/preferences/prefs.js

It looks like what happened was that the Loop patches landed in comment 32, and set the status to fixed. The pocket patch was later approved in comment 40, but hasn't been landed (and was missed due to the previous "fixed" status). I'm guessing the leave-open was also forgotten about, which is why this bug doesn't have status "fixed". Liz/Shane, we probably need some discussion/decision here.

Marking this as affected for 46/47, then. Is this still an issue in ESR too? Shane, can you fix this outside of a point release using the system addon?

This does not affect fx45 [pocket was not a system addon in that], iirc 45 is esr, so this does not affect esr. The primary issue here is with distributions, this is not a user-facing problem. ie. the pref still works if a user goes into about:config and turns off pocket. This also makes the fix more difficult, distributions need the fix "up front" not as a later upgrade to the addon. @mkpaly do you know who/if any distributions are turning off pocket and are affected by this with fx46? If so, can they live with it for a version? In the meantime, this should get landed on beta.

> do you know who/if any distributions are turning off pocket and are affected by this with fx46? If so, can they live with it for a version? Not distributions specifically, but enterprise customizations. The bug that found this (bug 1267946) seems to indicate that the preference doesn't work in any of the normal locations except about:config. For 46, I would probably just recommend folks remove the XPI from their distribution. That's the easiest way to disable it... I don't think it's worth trying to get Pocket updated for this one thing...

Ok, so wontfix for 46, but lets uplift through beta?

whatever patch you need to uplift, please nominate it for the appropriate branch.

hrm. it's in mozilla-beta (47) already, we'll just leave it at that.

I used distribution.ini file with loop.enabled and extensions.pocket.enabled/browser.pocket.enabled for ESR prefs added as false, and I can confirm that both Pocket and Loop are disabled. I used Firefox 47 beta 9 and latest pre ESR 45.1.2 build on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.10.5.

Removing leave-open keyword from resolved bugs, per :sylvestre.