Issue 6648246944399360: Noissue - Document the prefs module (Closed)
|
Created:
April 15, 2015, 4 p.m. by Sebastian Noack Modified:
April 21, 2015, 11:32 a.m. Reviewers:
kzar CC:
Wladimir Palant Visibility:
Public. |
DescriptionNoissue - Document the prefs module
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fixed mistakenly negated description of suppress_first_run_page #Patch Set 3 : Fixed typo, misspelling "initialization" #Patch Set 4 : Added link to devbuild announcement explaining the suppress_first_run_page preference #
Total comments: 1
MessagesTotal messages: 11
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. Nit: initialization http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:39: * Only for compatibility with core code. Please do not change! It would be nice to add a little detail for all these ones that we ask not to be changed. It might help me to grok the core code later for example. http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:160: * this event before using preferences during extension intilization. Nit: initialization
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:39: * Only for compatibility with core code. Please do not change! On 2015/04/16 12:11:14, kzar wrote: > It would be nice to add a little detail for all these ones that we ask not to be > changed. It might help me to grok the core code later for example. Those are already documented under https://adblockplus.org/en/preferences. Duplicating the documentation or even linking to it here, would be misleading, since those preferences don't have the same effect here, but changing them would merely break the extension.
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 12:11:14, kzar wrote: > Nit: initialization What about these comments? http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:39: * Only for compatibility with core code. Please do not change! On 2015/04/16 12:52:00, Sebastian Noack wrote: > On 2015/04/16 12:11:14, kzar wrote: > > It would be nice to add a little detail for all these ones that we ask not to > be > > changed. It might help me to grok the core code later for example. > > Those are already documented under https://adblockplus.org/en/preferences. > Duplicating the documentation or even linking to it here, would be misleading, > since those preferences don't have the same effect here, but changing them would > merely break the extension. Fair enough but in that case how about adding a "@see https://adblockplus.org/en/preferences" to them?
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 13:32:34, kzar wrote: > On 2015/04/16 12:11:14, kzar wrote: > > Nit: initialization > > What about these comments? I didn't addressed them yet, or do you read "Done"? ;) http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:39: * Only for compatibility with core code. Please do not change! On 2015/04/16 13:32:34, kzar wrote: > On 2015/04/16 12:52:00, Sebastian Noack wrote: > > On 2015/04/16 12:11:14, kzar wrote: > > > It would be nice to add a little detail for all these ones that we ask not > to > > be > > > changed. It might help me to grok the core code later for example. > > > > Those are already documented under https://adblockplus.org/en/preferences. > > Duplicating the documentation or even linking to it here, would be misleading, > > since those preferences don't have the same effect here, but changing them > would > > merely break the extension. > > Fair enough but in that case how about adding a "@see > https://adblockplus.org/en/preferences%22 to them? I do for preferences that have an effect here. But as I said, those preferences don't have the documented effect on Chrome, Opera and Safari, but would merely break the extension. So there is no point in linking to the Firefox documentation here. All that somebody working on this code needs to know is that he must not touch those preferences.
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 13:47:06, Sebastian Noack wrote: > On 2015/04/16 13:32:34, kzar wrote: > > On 2015/04/16 12:11:14, kzar wrote: > > > Nit: initialization > > > > What about these comments? > > I didn't addressed them yet, or do you read "Done"? ;) You quite commonly say "What about this comment" on my reviews should I not reply to one. I assumed you would expect the same.
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 12:11:14, kzar wrote: > Nit: initialization Done. http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 13:48:55, kzar wrote: > On 2015/04/16 13:47:06, Sebastian Noack wrote: > > On 2015/04/16 13:32:34, kzar wrote: > > > On 2015/04/16 12:11:14, kzar wrote: > > > > Nit: initialization > > > > > > What about these comments? > > > > I didn't addressed them yet, or do you read "Done"? ;) > > You quite commonly say "What about this comment" on my reviews should I not > reply to one. I assumed you would expect the same. Only if you uploaded a new patch set, and I have reason to assume that you overlooked those comments or try to cheat around addressing them. ;P Done. http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:160: * this event before using preferences during extension intilization. On 2015/04/16 12:11:14, kzar wrote: > Nit: initialization Done.
LGTM http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 13:51:43, Sebastian Noack wrote: > On 2015/04/16 13:48:55, kzar wrote: > > On 2015/04/16 13:47:06, Sebastian Noack wrote: > > > On 2015/04/16 13:32:34, kzar wrote: > > > > On 2015/04/16 12:11:14, kzar wrote: > > > > > Nit: initialization > > > > > > > > What about these comments? > > > > > > I didn't addressed them yet, or do you read "Done"? ;) > > > > You quite commonly say "What about this comment" on my reviews should I not > > reply to one. I assumed you would expect the same. > > Only if you uploaded a new patch set, and I have reason to assume that you > overlooked those comments or try to cheat around addressing them. ;P > > Done. You had uploaded a new patch set though...
http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5629499534213120/lib/... lib/prefs.js:33: * The application version as set during intilization. Used to detect updates. On 2015/04/16 13:54:50, kzar wrote: > On 2015/04/16 13:51:43, Sebastian Noack wrote: > > On 2015/04/16 13:48:55, kzar wrote: > > > On 2015/04/16 13:47:06, Sebastian Noack wrote: > > > > On 2015/04/16 13:32:34, kzar wrote: > > > > > On 2015/04/16 12:11:14, kzar wrote: > > > > > > Nit: initialization > > > > > > > > > > What about these comments? > > > > > > > > I didn't addressed them yet, or do you read "Done"? ;) > > > > > > You quite commonly say "What about this comment" on my reviews should I not > > > reply to one. I assumed you would expect the same. > > > > Only if you uploaded a new patch set, and I have reason to assume that you > > overlooked those comments or try to cheat around addressing them. ;P > > > > Done. > > You had uploaded a new patch set though... ... before you published your comments. ;)
http://codereview.adblockplus.org/6648246944399360/diff/5634387206995968/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/6648246944399360/diff/5634387206995968/lib/... lib/prefs.js:141: * @see https://adblockplus.org/development-builds/suppressing-the-first-run-page-on-... Note that this page isn't up yet, but will be at the time this change is going to be pushed.
LGTM 
|
