Issue 5059 - Simplify I/O API and FilterStorage implementation

Issue 5059 - Simplify I/O API and FilterStorage implementation

[Wladimir Palant, 2017-04-10 14:59:15]

At this point, these changes have been validated by unit tests only. I'll test
integration in Firefox before pushing.

https://codereview.adblockplus.org/29408742/diff/29408743/lib/filterStorage.js
File lib/filterStorage.js (left):

https://codereview.adblockplus.org/29408742/diff/29408743/lib/filterStorage.j...
lib/filterStorage.js:627: catch (e) {}
This code block is removed, now the IO module is responsible for creating the
directory (which in practice no implementations need to do any more).

https://codereview.adblockplus.org/29408742/diff/29408743/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29408742/diff/29408743/lib/filterStorage.j...
lib/filterStorage.js:66: return "patterns.ini";
We are removing configurability here, there is no point any more.

https://codereview.adblockplus.org/29408742/diff/29408743/lib/filterStorage.j...
lib/filterStorage.js:465: FilterNotifier.triggerListeners("load");
There is a change here: load notification is fired and initialized flag is set
only after we tried backups. This restores the logic we had before
https://issues.adblockplus.org/ticket/5052.

https://codereview.adblockplus.org/29408742/diff/29408743/lib/filterStorage.j...
lib/filterStorage.js:619: .catch(ignoreErrors);
The logic is slightly simplified by not removing the last backup explicitly any
more. The assumption is that IO.renameFile() will always overwrite existing
files (which it does for the remaining implementations).

https://codereview.adblockplus.org/29408742/diff/29408743/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29408742/diff/29408743/test/filterStorage_...
test/filterStorage_readwrite.js:299:
FilterStorage.addFilter(Filter.fromText("barfoo"));
Removing subscription here doesn't work because FilterStorage will restore
backup automatically then (no data in patterns.ini). Previously we got away with
this because load notification fired before the backup was restored, this is no
longer the case.

[Wladimir Palant, 2017-04-11 15:12:36]

Patch set 2 fixes an issue that only came up when integrating in Firefox - an
inner function doesn't preserve this value, unlike an arrow function.

[Wladimir Palant, 2017-04-11 15:20:27]

I uploaded https://codereview.adblockplus.org/29409617/ to show how this will be
integrated in Firefox. The io.js implementation there has proper JSDoc comments
and the fallbacks make most differences between old and new API obvious.

[kzar, 2017-04-20 06:49:25]

https://codereview.adblockplus.org/29408742/diff/29409613/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29408742/diff/29409613/lib/filterStorage.j...
lib/filterStorage.js:461: return tryBackup(1);
So we return a promise here, which might recursively return more promises while
we attempt to restore from our backups. But if tryBackup gives up it will catch
the error and I think the promise here will resolve anyway. Is that what you
intended? It seems kind of counter-intuitive that the promise isn't rejected in
that case.

https://codereview.adblockplus.org/29408742/diff/29409613/test/stub-modules/i...
File test/stub-modules/io.js (left):

https://codereview.adblockplus.org/29408742/diff/29409613/test/stub-modules/i...
test/stub-modules/io.js:6: // Fake nsIFile implementation for our I/O
We have similar crap in adblockpluschrome, I wonder if we can remove it there
too? I guess we have to adjust how the io module works there too? If so perhaps
add some more details to the integration notes in the issue?

[Wladimir Palant, 2017-04-20 07:17:07]

https://codereview.adblockplus.org/29408742/diff/29409613/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29408742/diff/29409613/lib/filterStorage.j...
lib/filterStorage.js:461: return tryBackup(1);
On 2017/04/20 06:49:25, kzar wrote:
> So we return a promise here, which might recursively return more promises
while
> we attempt to restore from our backups. But if tryBackup gives up it will
catch
> the error and I think the promise here will resolve anyway. Is that what you
> intended? It seems kind of counter-intuitive that the promise isn't rejected
in
> that case.

Yes, this is the intended behavior - we already reported an error, and we are
attempting a fallback now. There is little point reporting another error because
the fallback failed as well.

https://codereview.adblockplus.org/29408742/diff/29409613/test/stub-modules/i...
File test/stub-modules/io.js (left):

https://codereview.adblockplus.org/29408742/diff/29409613/test/stub-modules/i...
test/stub-modules/io.js:6: // Fake nsIFile implementation for our I/O
On 2017/04/20 06:49:25, kzar wrote:
> We have similar crap in adblockpluschrome, I wonder if we can remove it there
> too? I guess we have to adjust how the io module works there too? If so
perhaps
> add some more details to the integration notes in the issue?

Yes, that was the point - fake files should no longer be necessary with these
changes. I made the integration notes more explicit on the topic of API changes.

[kzar, 2017-04-20 07:32:16]

LGTM

[Sebastian Noack, 2017-04-20 08:08:08]

LGTM