Issue 3247 - Closing the browser shortly after initial launch(s) causes no filters lists to be sele…

Issue 3247 - Closing the browser shortly after initial launch(s) causes no filters lists to be sele…

[anton, 2016-09-30 06:39:04]

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm
File adblockplus/Api.jsm (right):

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm...
adblockplus/Api.jsm:82: return null;
LGTM in general if hiding exception with just `null` is ok for js (in java it's
not good usually)

[Felix Dahlke, 2016-09-30 07:44:12]

Looks good, but see my comment.

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm
File adblockplus/Api.jsm (right):

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm...
adblockplus/Api.jsm:61: UI.addSubscription(UI.currentWindow,
Prefs.currentVersion);
This'll solve the subscription issue I guess, but shouldn't we run all the first
run actions (i.e. `UI.firstRunActions`) again if they weren't previously
completed, not just the `addSubscription` step? (I might be missing something,
it's been a little while since I was working on this code.)

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm...
adblockplus/Api.jsm:82: return null;
On 2016/09/30 06:39:04, anton wrote:
> LGTM in general if hiding exception with just `null` is ok for js (in java
it's
> not good usually)

That's alright, but because of type coercion, this is going to be treated the
same as `false` in most cases (e.g. in the condition above). Just something to
be aware of.

[diegocarloslima, 2016-10-25 16:16:20]

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm
File adblockplus/Api.jsm (right):

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm...
adblockplus/Api.jsm:61: UI.addSubscription(UI.currentWindow,
Prefs.currentVersion);
On 2016/09/30 07:44:12, Felix Dahlke wrote:
> This'll solve the subscription issue I guess, but shouldn't we run all the
first
> run actions (i.e. `UI.firstRunActions`) again if they weren't previously
> completed, not just the `addSubscription` step? (I might be missing something,
> it's been a little while since I was working on this code.)

Actually firstRunActions is always performed. The issue is on this snipped from
ui.js:
    let prevVersion = Prefs.currentVersion;
    if (prevVersion != addonVersion)
    {
      Prefs.currentVersion = addonVersion;
      this.addSubscription(window, prevVersion);

Here, Prefs.currentVersion is stored before addSubscription completes. So if we
kill the app before addSubscription is stored on disk, next time this is run, it
will fail for the prevVersion != addonVersion check, because it was already
stored and it will never complete the addSubscription call

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm...
adblockplus/Api.jsm:82: return null;
On 2016/09/30 07:44:12, Felix Dahlke wrote:
> On 2016/09/30 06:39:04, anton wrote:
> > LGTM in general if hiding exception with just `null` is ok for js (in java
> it's
> > not good usually)
> 
> That's alright, but because of type coercion, this is going to be treated the
> same as `false` in most cases (e.g. in the condition above). Just something to
> be aware of.

 I did that way to have a more flexible way of handling what is returned by
getBoolPref, it could be useful for checking if any value has ever being defined
for that preference

[Felix Dahlke, 2016-10-27 17:12:50]

LGTM

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm
File adblockplus/Api.jsm (right):

https://codereview.adblockplus.org/29350137/diff/29350138/adblockplus/Api.jsm...
adblockplus/Api.jsm:61: UI.addSubscription(UI.currentWindow,
Prefs.currentVersion);
On 2016/10/25 16:16:20, diegocarloslima wrote:
> On 2016/09/30 07:44:12, Felix Dahlke wrote:
> > This'll solve the subscription issue I guess, but shouldn't we run all the
> first
> > run actions (i.e. `UI.firstRunActions`) again if they weren't previously
> > completed, not just the `addSubscription` step? (I might be missing
something,
> > it's been a little while since I was working on this code.)
> 
> Actually firstRunActions is always performed. The issue is on this snipped
from
> ui.js:
>     let prevVersion = Prefs.currentVersion;
>     if (prevVersion != addonVersion)
>     {
>       Prefs.currentVersion = addonVersion;
>       this.addSubscription(window, prevVersion);
> 
> Here, Prefs.currentVersion is stored before addSubscription completes. So if
we
> kill the app before addSubscription is stored on disk, next time this is run,
it
> will fail for the prevVersion != addonVersion check, because it was already
> stored and it will never complete the addSubscription call

Acknowledged.