Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -1 line) Patch
M lib/prefs.js View 1 2 3 3 chunks +11 lines, -1 line 0 comments Download

Messages

Total messages: 14
Felix Dahlke
Somewhat WIP - we don't exactly agree that this is the correct approach so far. ...
March 19, 2015, 10:18 p.m. (2015-03-19 22:18:49 UTC) #1
Sebastian Noack
I would love to enable users and admins to externally override default prefs that way. ...
March 20, 2015, 9:55 a.m. (2015-03-20 09:55:03 UTC) #2
Felix Dahlke
On 2015/03/20 09:55:03, Sebastian Noack wrote: > I would love to enable users and admins ...
March 20, 2015, 9:58 a.m. (2015-03-20 09:58:15 UTC) #3
Sebastian Noack
So I see following options to handle this: 1. Ignore it. We rarely change default ...
March 20, 2015, 10:26 a.m. (2015-03-20 10:26:34 UTC) #4
Felix Dahlke
On 2015/03/20 10:26:34, Sebastian Noack wrote: > So I see following options to handle this: ...
March 20, 2015, 10:28 a.m. (2015-03-20 10:28:42 UTC) #5
Felix Dahlke
New patch set is up, implemented Sebastian's suggestion. One thing that's not nice with this ...
March 21, 2015, 7:50 p.m. (2015-03-21 19:50:25 UTC) #6
Felix Dahlke
Uploaded a new patch set, as discussed in the issue.
April 3, 2015, 11:28 p.m. (2015-04-03 23:28:47 UTC) #7
Sebastian Noack
Maybe I just miss something, but the argument introduced here seems to be unused.
April 4, 2015, 12:42 a.m. (2015-04-04 00:42:15 UTC) #8
Felix Dahlke
On 2015/04/04 00:42:15, Sebastian Noack wrote: > Maybe I just miss something, but the argument ...
April 4, 2015, 11:05 p.m. (2015-04-04 23:05:32 UTC) #9
Sebastian Noack
LGTM. But this change should be reviewed by Wladimir as well.
April 5, 2015, 11:01 a.m. (2015-04-05 11:01:36 UTC) #10
Wladimir Palant
http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/prefs.js#newcode36 lib/prefs.js:36: catch (e) {} Nit: Please put brackets around this ...
April 28, 2015, 9:25 p.m. (2015-04-28 21:25:19 UTC) #11
Felix Dahlke
New patch set up. http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/6647895159734272/diff/5766466041282560/lib/prefs.js#newcode36 lib/prefs.js:36: catch (e) {} On 2015/04/28 ...
April 28, 2015, 9:28 p.m. (2015-04-28 21:28:32 UTC) #12
Wladimir Palant
LGTM
April 28, 2015, 9:33 p.m. (2015-04-28 21:33:41 UTC) #13
Sebastian Noack
April 29, 2015, 6:56 p.m. (2015-04-29 18:56:08 UTC) #14
LGTM

Powered by Google App Engine
This is Rietveld