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

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

[Wladimir Palant, 2017-08-24 09:08:09]

https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataClea...
File lib/firefoxDataCleanup.js (right):

https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:73: backups.push(`file:patterns-backup${i}.ini`);
This is using implementation details of io.js which is awkward. However, using
io.js here would require probing whether the files are actually there - which
means reading in the data, very inefficient.

https://codereview.adblockplus.org/29526555/diff/29526556/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29526555/diff/29526556/lib/prefs.js#newcode54
lib/prefs.js:54: defaults.patternsbackups = 0;
I don't know why this feature was enabled initially, it's useless right now. On
Firefox it also degrades performance considerably because both patterns.ini and
all the backups are stored in a single JSON file.

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
This should really be added in metadata.gecko-webext but that doesn't work
correctly. IMHO, the format has been misdesigned here, spaces as separator would
allow extending the value in configurations inheriting from this one.

[Sebastian Noack, 2017-08-24 09:42:06]

https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataClea...
File lib/firefoxDataCleanup.js (right):

https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:74: return
browser.storage.local.remove(backups).then(() =>
Returning this promise here seems weird. Did you mean to return the promise
returned by browser.storage.local.remove(backups), and then chain the callback
to the outer promise?

https://codereview.adblockplus.org/29526555/diff/29526556/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29526555/diff/29526556/lib/prefs.js#newcod...
lib/prefs.js:156: defaults.data_cleanup_done = false;
This way the data migration will also be performed on fresh installation. I
wonder whether it would be worth to prevent that.

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 09:08:09, Wladimir Palant wrote:
> This should really be added in metadata.gecko-webext but that doesn't work
> correctly. IMHO, the format has been misdesigned here, spaces as separator
would
> allow extending the value in configurations inheriting from this one.

I agree. But probably not worth changing if we move to webpack soon anyway?

[Wladimir Palant, 2017-08-24 09:57:43]

https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataClea...
File lib/firefoxDataCleanup.js (right):

https://codereview.adblockplus.org/29526555/diff/29526556/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:74: return
browser.storage.local.remove(backups).then(() =>
On 2017/08/24 09:42:05, Sebastian Noack wrote:
> Returning this promise here seems weird.

Right, this is merely a left-over from a previous iteration. Fixed.

https://codereview.adblockplus.org/29526555/diff/29526556/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29526555/diff/29526556/lib/prefs.js#newcod...
lib/prefs.js:156: defaults.data_cleanup_done = false;
On 2017/08/24 09:42:05, Sebastian Noack wrote:
> This way the data migration will also be performed on fresh installation. I
> wonder whether it would be worth to prevent that.

On a fresh install this will be a fast operation. Then again, we can detect
first run easily.

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 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.

[Sebastian Noack, 2017-08-24 10:19:45]

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 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.

https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataClea...
File lib/firefoxDataCleanup.js (right):

https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:20: let {Filter, ActiveFilter} =
require("filterClasses");
It seems we use const (instead let) for module imports everywhere else.

https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:58: if (position < 0)
You might want to use includes() instead of indexOf() < 0.

[Wladimir Palant, 2017-08-24 10:44:24]

https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataClea...
File lib/firefoxDataCleanup.js (right):

https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:20: let {Filter, ActiveFilter} =
require("filterClasses");
On 2017/08/24 10:19:44, Sebastian Noack wrote:
> It seems we use const (instead let) for module imports everywhere else.

Done.

https://codereview.adblockplus.org/29526555/diff/29526566/lib/firefoxDataClea...
lib/firefoxDataCleanup.js:58: if (position < 0)
On 2017/08/24 10:19:44, Sebastian Noack wrote:
> You might want to use includes() instead of indexOf() < 0.

No, we are using the position later on.

[Sebastian Noack, 2017-08-24 10:45:58]

LGTM

[kzar, 2017-08-29 11:33:48]

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.