Issue 4580 - Removed ext.storage

Issue 4580 - Removed ext.storage

[kzar, 2018-04-24 20:38:56]

Sorry I hoped to finish this review tonight, but I'm feeling pretty tired. This
change looks really good so far, removing an unnecessary abstraction and
reducing LOC, but I want to look through it one last time tomorrow when I'm more
awake!

https://codereview.adblockplus.org/29760680/diff/29760681/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29760680/diff/29760681/lib/io.js#newcode35
lib/io.js:35: throw {type: "NoSuchFile"};
Don't we normally throw an Error instance? (I get the feeling you know what
you're doing here, so I'm not saying this is wrong. Just kinda curious, since
I've not seen throw used like this before.)

https://codereview.adblockplus.org/29760680/diff/29760681/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29760680/diff/29760681/lib/prefs.js#newcod...
lib/prefs.js:336: (error) =>
Nit: Superfluous parenthesis, also could we just pass in console.error directly?

[Sebastian Noack, 2018-04-24 21:12:55]

https://codereview.adblockplus.org/29760680/diff/29760681/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29760680/diff/29760681/lib/io.js#newcode35
lib/io.js:35: throw {type: "NoSuchFile"};
On 2018/04/24 20:38:56, kzar wrote:
> Don't we normally throw an Error instance? (I get the feeling you know what
> you're doing here, so I'm not saying this is wrong. Just kinda curious, since
> I've not seen throw used like this before.)

Yeah, I had a similar WTF moment when I converted this code, but didn't feel
like doing an unrelated change here.

FWIW you can throw any kind of value (or reject a promise with any kind of
value). It will just be passed through to any catch statement (or error callback
attached to the promise). There is nothing special about Error objects in
JavaScript (other than that they have a traceback attached).

Its of course good practice to use Error objects to indicate an error, but then
I have to also change the code that detects non existing files, and adding a
custom property to the error object might not be much better. If we want to do
that properly we would just resolve the promise with null if the file does not
exist, but then I have to change even more.

https://codereview.adblockplus.org/29760680/diff/29760681/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29760680/diff/29760681/lib/prefs.js#newcod...
lib/prefs.js:336: (error) =>
On 2018/04/24 20:38:56, kzar wrote:
> Nit: Superfluous parenthesis

Acknowledged.

> also could we just pass in console.error directly?

That is not garuanteed to work, and at least in older Chrome versions it didn't
work. That is because its an implementation detail of the JavaScript environment
whether the console methods depend on the console object (i.e. rely on the
correct value of "this") or not.

FWIW, I only added the error callback here to not change the behavior yet, i.e.
make sure that the Prefs.untilLoaded promise is never rejected. This callback is
removed with the next patch anyway.

[kzar, 2018-04-25 10:33:39]

LGTM

https://codereview.adblockplus.org/29760680/diff/29760681/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29760680/diff/29760681/lib/io.js#newcode35
lib/io.js:35: throw {type: "NoSuchFile"};
On 2018/04/24 21:12:55, Sebastian Noack wrote:
> On 2018/04/24 20:38:56, kzar wrote:
> > Don't we normally throw an Error instance? (I get the feeling you know what
> > you're doing here, so I'm not saying this is wrong. Just kinda curious,
since
> > I've not seen throw used like this before.)
> 
> Yeah, I had a similar WTF moment when I converted this code, but didn't feel
> like doing an unrelated change here.
> 
> FWIW you can throw any kind of value (or reject a promise with any kind of
> value). It will just be passed through to any catch statement (or error
callback
> attached to the promise). There is nothing special about Error objects in
> JavaScript (other than that they have a traceback attached).
> 
> Its of course good practice to use Error objects to indicate an error, but
then
> I have to also change the code that detects non existing files, and adding a
> custom property to the error object might not be much better. If we want to do
> that properly we would just resolve the promise with null if the file does not
> exist, but then I have to change even more. 

Acknowledged.

https://codereview.adblockplus.org/29760680/diff/29760681/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29760680/diff/29760681/lib/prefs.js#newcod...
lib/prefs.js:336: (error) =>
On 2018/04/24 21:12:55, Sebastian Noack wrote:
> This callback is removed with the next patch anyway.

Ah right, well it's not a big deal then.