Issue 3515 - Use fetch() API instead XMLHttpRequest (Platform)

Issue 3515 - Use fetch() API instead XMLHttpRequest (Platform)

[kzar, 2016-01-17 17:14:06]

https://codereview.adblockplus.org/29333528/diff/29333608/background.js
File background.js (right):

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:235: var node =
Utils.chooseFilterSubscription(doc.getElementsByTagName("subscription"));
Nit: While changing mind fixing some of these long lines?

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:247: ).then(function()
Nit: The indentation of this `.then(...` call seems inconsistent with the
previous one.

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:249: if (!Prefs.suppress_first_run_page)
I think previously we always called `notifyUser()` but now we only execute it's
code if `addSubscription` is truthy and everything succeeds. Deliberate?

https://codereview.adblockplus.org/29333528/diff/29333608/options.js
File options.js (right):

https://codereview.adblockplus.org/29333528/diff/29333608/options.js#newcode209
options.js:209: option.text = element.getAttribute("title") + " (" +
element.getAttribute("specialization") + ")";
Nit: Mind fixing the long lines here as well while at it?

[Sebastian Noack, 2016-01-19 14:58:10]

https://codereview.adblockplus.org/29333528/diff/29333608/background.js
File background.js (right):

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:235: var node =
Utils.chooseFilterSubscription(doc.getElementsByTagName("subscription"));
On 2016/01/17 17:14:06, kzar wrote:
> Nit: While changing mind fixing some of these long lines?

Done.

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:247: ).then(function()
On 2016/01/17 17:14:06, kzar wrote:
> Nit: The indentation of this `.then(...` call seems inconsistent with the
> previous one.

Intending the code above like that:

  Promise.resolve(addSubscription &&
fetch("subscriptions.xml").then(function(response)
  {
    ..
  }).then(text)
  {
    ..
  });

would make it harder to identify the then() calls. And the first line would be
too long. On the other hand:

  Promise.resolve(
   ..
  )
    .then(function()
    {
      ..
    })

would look quite weird.

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:249: if (!Prefs.suppress_first_run_page)
On 2016/01/17 17:14:06, kzar wrote:
> I think previously we always called `notifyUser()` but now we only execute
it's
> code if `addSubscription` is truthy and everything succeeds. Deliberate?

That's not correct. Promise.resolve(addSubscription && fetch(..).then(..))
returns a promise that is immediately fulfilled with the value of
addSubscription if it is false, or when the request was sent and its response
was processed successfully.

https://codereview.adblockplus.org/29333528/diff/29333608/options.js
File options.js (right):

https://codereview.adblockplus.org/29333528/diff/29333608/options.js#newcode209
options.js:209: option.text = element.getAttribute("title") + " (" +
element.getAttribute("specialization") + ")";
On 2016/01/17 17:14:06, kzar wrote:
> Nit: Mind fixing the long lines here as well while at it?

Done.

[kzar, 2016-01-19 15:39:19]

https://codereview.adblockplus.org/29333528/diff/29333608/background.js
File background.js (right):

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:247: ).then(function()
On 2016/01/19 14:58:10, Sebastian Noack wrote:
> On 2016/01/17 17:14:06, kzar wrote:
> > Nit: The indentation of this `.then(...` call seems inconsistent with the
> > previous one.
> 
> Intending the code above like that:
> 
>   Promise.resolve(addSubscription &&
> fetch("subscriptions.xml").then(function(response)
>   {
>     ..
>   }).then(text)
>   {
>     ..
>   });
> 
> would make it harder to identify the then() calls. And the first line would be
> too long. On the other hand:
> 
>   Promise.resolve(
>    ..
>   )
>     .then(function()
>     {
>       ..
>     })
> 
> would look quite weird.

What about doing this here? That way they'd all be lined up.

  }))
  .then(function()
    ...

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:249: if (!Prefs.suppress_first_run_page)
On 2016/01/19 14:58:10, Sebastian Noack wrote:
> On 2016/01/17 17:14:06, kzar wrote:
> > I think previously we always called `notifyUser()` but now we only execute
> it's
> > code if `addSubscription` is truthy and everything succeeds. Deliberate?
> 
> That's not correct. Promise.resolve(addSubscription && fetch(..).then(..))
> returns a promise that is immediately fulfilled with the value of
> addSubscription if it is false, or when the request was sent and its response
> was processed successfully.

Ah, I see.

[Sebastian Noack, 2016-01-19 16:13:25]

https://codereview.adblockplus.org/29333528/diff/29333608/background.js
File background.js (right):

https://codereview.adblockplus.org/29333528/diff/29333608/background.js#newco...
background.js:247: ).then(function()
On 2016/01/19 15:39:19, kzar wrote:
> On 2016/01/19 14:58:10, Sebastian Noack wrote:
> > On 2016/01/17 17:14:06, kzar wrote:
> > > Nit: The indentation of this `.then(...` call seems inconsistent with the
> > > previous one.
> > 
> > Intending the code above like that:
> > 
> >   Promise.resolve(addSubscription &&
> > fetch("subscriptions.xml").then(function(response)
> >   {
> >     ..
> >   }).then(text)
> >   {
> >     ..
> >   });
> > 
> > would make it harder to identify the then() calls. And the first line would
be
> > too long. On the other hand:
> > 
> >   Promise.resolve(
> >    ..
> >   )
> >     .then(function()
> >     {
> >       ..
> >     })
> > 
> > would look quite weird.
> 
> What about doing this here? That way they'd all be lined up.
> 
>   }))
>   .then(function()
>     ...

Note sure if I like it any better, but done.

[kzar, 2016-01-19 16:24:15]

LGTM