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

Issue 29526555: Issue 5554 - [webextensions] Adjust data to account for UI limitations (Closed)

Created:
Aug. 24, 2017, 9:02 a.m. by Wladimir Palant
Modified:
Aug. 29, 2017, 11:33 a.m.
Reviewers:
Sebastian Noack, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome
Visibility:
Public.

Description

Issue 5554 - [webextensions] Adjust data to account for UI limitations

Patch Set 1 #

Total comments: 11

Patch Set 2 : Return early on fresh installs #

Total comments: 4

Patch Set 3 : Made imports constant #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -2 lines) Patch
A lib/firefoxDataCleanup.js View 1 2 1 chunk +84 lines, -0 lines 0 comments Download
M lib/prefs.js View 2 chunks +9 lines, -1 line 0 comments Download
M metadata.chrome View 1 chunk +1 line, -1 line 0 comments Download
M metadata.gecko-webext View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7
Wladimir Palant
https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataCleanup.js File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataCleanup.js#newcode73 lib/firefoxDataCleanup.js:73: backups.push(`file:patterns-backup${i}.ini`); This is using implementation details of io.js which ...
Aug. 24, 2017, 9:08 a.m. (2017-08-24 09:08:09 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataCleanup.js File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataCleanup.js#newcode74 lib/firefoxDataCleanup.js:74: return browser.storage.local.remove(backups).then(() => Returning this promise here seems weird. ...
Aug. 24, 2017, 9:42 a.m. (2017-08-24 09:42:06 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataCleanup.js File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataCleanup.js#newcode74 lib/firefoxDataCleanup.js:74: return browser.storage.local.remove(backups).then(() => On 2017/08/24 09:42:05, Sebastian Noack wrote: ...
Aug. 24, 2017, 9:57 a.m. (2017-08-24 09:57:43 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29526555/diff/29526556/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29526555/diff/29526556/metadata.chrome#newcode115 metadata.chrome:115: lib/adblockplus.js[autoload] = filterListener,synchronizer,requestBlocker,popupBlocker,subscriptionInit,filterComposer,stats,uninstall,csp,cssInjection,firefoxDataCleanup On 2017/08/24 09:57:43, Wladimir Palant wrote: ...
Aug. 24, 2017, 10:19 a.m. (2017-08-24 10:19:45 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataCleanup.js File lib/firefoxDataCleanup.js (right): https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataCleanup.js#newcode20 lib/firefoxDataCleanup.js:20: let {Filter, ActiveFilter} = require("filterClasses"); On 2017/08/24 10:19:44, Sebastian ...
Aug. 24, 2017, 10:44 a.m. (2017-08-24 10:44:24 UTC) #5
Sebastian Noack
LGTM
Aug. 24, 2017, 10:45 a.m. (2017-08-24 10:45:58 UTC) #6
kzar
Aug. 29, 2017, 11:33 a.m. (2017-08-29 11:33:48 UTC) #7
Message was sent while issue was closed.
https://codereview.adblockplus.org/29526555/diff/29526556/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29526555/diff/29526556/metadata.chrome#new...
metadata.chrome:115: lib/adblockplus.js[autoload] =
filterListener,synchronizer,requestBlocker,popupBlocker,subscriptionInit,filterComposer,stats,uninstall,csp,cssInjection,firefoxDataCleanup
On 2017/08/24 10:19:44, Sebastian Noack wrote:
> On 2017/08/24 09:57:43, Wladimir Palant wrote:
> > On 2017/08/24 09:42:05, Sebastian Noack wrote:
> > > I agree. But probably not worth changing if we move to webpack soon
anyway?
> > 
> > Rather something to be changed in the same go. We'll need to specify entry
> > points for webpack as well, we should make sure that these are separated by
> > spaces.
> 
> Acknowledged.

Noted, I'll do that.

Powered by Google App Engine
This is Rietveld