Issue 6647895159734272: Issue 1489 - Support preconfigured defaults (Closed)
|
Created:
March 19, 2015, 10:14 p.m. by Felix Dahlke Modified:
April 30, 2015, 3:30 a.m. Visibility:
Public. |
DescriptionIssue 1489 - Support preconfigured defaults
Patch Set 1 #Patch Set 2 : Make the defaults to leave alone configurable #Patch Set 3 : Read preconfigured defaults from a dedicated branch #
Total comments: 2
Patch Set 4 : Added braces around the if body #MessagesTotal messages: 14
Somewhat WIP - we don't exactly agree that this is the correct approach so far. Still, it seems to work, also solving a problem in Flux. Might as well put it up for review.
I would love to enable users and admins to externally override default prefs that way. But doesn't prevent it us from changing default prefs in future versions, because we won't know whether existing default prefs were set by a previous version of Adblock Plus or by the user?
On 2015/03/20 09:55:03, Sebastian Noack wrote: > I would love to enable users and admins to externally override default prefs > that way. But doesn't prevent it us from changing default prefs in future > versions, because we won't know whether existing default prefs were set by a > previous version of Adblock Plus or by the user? That's what I meant by "we don't exactly agree" - there's an (unfortunately internal for no good reason) discussion on this: https://intraforum.adblockplus.org/t/pre-configuring-extensions-on-chrome-ope... We should settle that one first.
So I see following options to handle this: 1. Ignore it. We rarely change default pref and when we need to, we can still just override that one particular default pref for a while until the majority of users have updated, like we do in other migration scenarios. 2. Don't use Gecko APIs to configure default prefs, but handle them manually like we do on Chrome. So if there is a default pref set we know it was set by the user and we use it. If there isn't a default value for a pref, we fallback to our defaults stored in a JavaScript object or something. 3. Introduce a new pref called "pre_configured_prefs" or something like that, which is never set by us, but can be set externally by the user to indicate which prefs has been pre-configured. We then read that pref and only apply our defaults for prefs not listed there.
On 2015/03/20 10:26:34, Sebastian Noack wrote: > So I see following options to handle this: > > 1. Ignore it. We rarely change default pref and when we need to, we can still > just override that one particular default pref for a while until the majority of > users have updated, like we do in other migration scenarios. > > 2. Don't use Gecko APIs to configure default prefs, but handle them manually > like we do on Chrome. So if there is a default pref set we know it was set by > the user and we use it. If there isn't a default value for a pref, we fallback > to our defaults stored in a JavaScript object or something. > > 3. Introduce a new pref called "pre_configured_prefs" or something like that, > which is never set by us, but can be set externally by the user to indicate > which prefs has been pre-configured. We then read that pref and only apply our > defaults for prefs not listed there. Options 1 and 2 are under discussion, yeah. Option 3 was new to me, and I actually kinda like it. Best of both worlds.
New patch set is up, implemented Sebastian's suggestion. One thing that's not nice with this approach is that specifying preconfigured_defaults via JSON in autoconfig.js is quite error prone - needs to be a string, passing in an array (with or without JSON.stringify) doesn't work.
Uploaded a new patch set, as discussed in the issue.
Maybe I just miss something, but the argument introduced here seems to be unused.
On 2015/04/04 00:42:15, Sebastian Noack wrote: > Maybe I just miss something, but the argument introduced here seems to be > unused. Yes, this is a change to buildtools. The prefs() function defined here is used in defaults/prefs.js in ABP for Firefox. If this lands, I'll put up a tiny patch for that that passes `true` as the third parameter for the suppress_first_run_page pref.
LGTM. But this change should be reviewed by Wladimir as well.
http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/... lib/prefs.js:36: catch (e) {} Nit: Please put brackets around this block.
New patch set up. http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/... lib/prefs.js:36: catch (e) {} On 2015/04/28 21:25:19, Wladimir Palant wrote: > Nit: Please put brackets around this block. Done.
LGTM
LGTM 
|
