Issue 4131 - Simplify UI initialization logic

Issue 4131 - Simplify UI initialization logic

Repository: hg.adblockplus.org/adblockplus

[Thomas Greiner, 2016-06-08 15:47:33]

https://codereview.adblockplus.org/29345630/diff/29345631/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29345630/diff/29345631/lib/ui.js#newcode281
lib/ui.js:281: let promises = [];
Detail: That variable name is not very descriptive because it's like calling an
array of numbers "numbers". For instance, I could imagine naming it
"startupActions" based on your comment.

https://codereview.adblockplus.org/29345630/diff/29345631/lib/ui.js#newcode293
lib/ui.js:293: return;
Instead of returning I'd suggest to resolve or reject the promise instead to
avoid it being in an unresolved state forever. Note that I couldn't find hints
in the standard as to what the browser is supposed to do with promises that
remain unresolved. So seems like it's up to the GC to decide what to do with
those.

Same also applies to the missing handling of the "error" event since the "load"
event is not being dispatched if an error occurs.

[Wladimir Palant, 2016-06-08 20:51:30]

Note: Changes from https://issues.adblockplus.org/ticket/4133 show up in the
interdiff due to rebasing.

https://codereview.adblockplus.org/29345630/diff/29345631/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/29345630/diff/29345631/lib/ui.js#newcode281
lib/ui.js:281: let promises = [];
On 2016/06/08 15:47:32, Thomas Greiner wrote:
> Detail: That variable name is not very descriptive because it's like calling
an
> array of numbers "numbers". For instance, I could imagine naming it
> "startupActions" based on your comment.

Done.

https://codereview.adblockplus.org/29345630/diff/29345631/lib/ui.js#newcode293
lib/ui.js:293: return;
On 2016/06/08 15:47:32, Thomas Greiner wrote:
> Instead of returning I'd suggest to resolve or reject the promise instead to
> avoid it being in an unresolved state forever.

We don't really want to do anything with that promise. If we resolve it, the
follow-up actions will run. If we reject, this will generate an error message.
Neither is desirable at this point, the extension has been shut down. Not doing
anything and allowing the promise to be garbage collected seems to be the best
course of action.

> Same also applies to the missing handling of the "error" event since the
"load"
> event is not being dispatched if an error occurs.

The original code didn't handle the "error" event either, simply because there
is no way this download would error out. In fact, chrome:// is a synchronous
protocol and there are no asynchronous error events. Still, fair enough - let's
fix this and reject the promise on error (had to replace protocol by http:// in
order to test this).

[Thomas Greiner, 2016-06-13 13:22:33]

LGTM