Issue 5264 - Return page objects instead of page IDs

After the changeset 2cebd252b232 (#5161) in adblockpluschrome, there's an issue on the options page: when you add a filter subscription, it throws an exception.

There are two problems: (1) keys() returns an iterable, not an array; (2) the keys themselves are page IDs, not page objects as the caller expects.

This patch modifies ext.PageMap.prototype.keys so that it returns an array of page objects instead of an iterable of page IDs.

[Manish Jethani, 2017-05-22 15:55:10]

Patch Set 1

See issue description.

[kzar, 2017-05-22 15:57:30]

On 2017/05/22 15:55:10, Manish Jethani wrote:
> Patch Set 1
> 
> See issue description.

Please could you open an issue for this change?

[Manish Jethani, 2017-05-22 16:19:32]

On 2017/05/22 15:57:30, kzar wrote:
> On 2017/05/22 15:55:10, Manish Jethani wrote:
> > Patch Set 1
> > 
> > See issue description.
> 
> Please could you open an issue for this change?

This would be filed as a bug, right?

[kzar, 2017-05-22 16:43:52]

On 2017/05/22 16:19:32, Manish Jethani wrote:
> On 2017/05/22 15:57:30, kzar wrote:
> > On 2017/05/22 15:55:10, Manish Jethani wrote:
> > > Patch Set 1
> > > 
> > > See issue description.
> > 
> > Please could you open an issue for this change?
> 
> This would be filed as a bug, right?

Yea I think so since it's a regression. I'd link to the commit and explain what
broke, what needs to be changed and also how our testers can verify the change
worked.

[Manish Jethani, 2017-05-22 17:04:14]

On 2017/05/22 15:57:30, kzar wrote:
> On 2017/05/22 15:55:10, Manish Jethani wrote:
> > Patch Set 1
> > 
> > See issue description.
> 
> Please could you open an issue for this change?

https://issues.adblockplus.org/ticket/5264

[Manish Jethani, 2017-05-22 17:09:48]

Patch Set 2: Return page objects instead of page IDs

A much more straightforward fix.

[kzar, 2017-05-23 11:47:59]

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js#n...
ext/background.js:38: return Array.from(this._map.keys()).map(ext.getPage);
I wonder if you could do `return Array.prototype.map.call(this._map.keys(),
ext.getPage)`?

[Sebastian Noack, 2017-05-23 12:16:54]

LGTM

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js#n...
ext/background.js:38: return Array.from(this._map.keys()).map(ext.getPage);
On 2017/05/23 11:47:59, kzar wrote:
> I wonder if you could do `return Array.prototype.map.call(this._map.keys(),
> ext.getPage)`?

Please don't. Array generics are non-standard and deprecated:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[Manish Jethani, 2017-05-23 12:32:27]

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js#n...
ext/background.js:38: return Array.from(this._map.keys()).map(ext.getPage);
On 2017/05/23 12:16:54, Sebastian Noack wrote:
> On 2017/05/23 11:47:59, kzar wrote:
> > I wonder if you could do `return Array.prototype.map.call(this._map.keys(),
> > ext.getPage)`?
> 
> Please don't. Array generics are non-standard and deprecated:
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

I think Dave meant Array.prototype.map rather than Array.map. The former isn't
going anywhere, though for better or worse it won't work with a non-array-like
object.

To map an iterable regardless of its type you have to use a function like this: 

https://codereview.adblockplus.org/29417597/patch/29417598/29417600

If you have generators support, then this: 

function* mapIterable(iterable, callback)
{
  for (let entry of iterable)
    yield callback(entry);
}

Here we don't have too many keys though, it's OK to create an intermediate
array.

[Sebastian Noack, 2017-05-23 15:09:18]

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js#n...
ext/background.js:38: return Array.from(this._map.keys()).map(ext.getPage);
On 2017/05/23 12:32:27, Manish Jethani wrote:
> On 2017/05/23 12:16:54, Sebastian Noack wrote:
> > On 2017/05/23 11:47:59, kzar wrote:
> > > I wonder if you could do `return
Array.prototype.map.call(this._map.keys(),
> > > ext.getPage)`?
> > 
> > Please don't. Array generics are non-standard and deprecated:
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> 
> I think Dave meant Array.prototype.map rather than Array.map. The former isn't
> going anywhere, though for better or worse it won't work with a non-array-like
> object.
> 
> To map an iterable regardless of its type you have to use a function like
this: 
> 
> https://codereview.adblockplus.org/29417597/patch/29417598/29417600
> 
> If you have generators support, then this: 
> 
> function* mapIterable(iterable, callback)
> {
>   for (let entry of iterable)
>     yield callback(entry);
> }
> 
> Here we don't have too many keys though, it's OK to create an intermediate
> array.

Sorry, you are right, I mixed things up, calling array method's through the
prototype isn't generally an issue, but as you pointed out it only works with
array-like objects.

I wouldn't be opposed to make keys() a generator function. All browsers this
code is running on support generators, and it doesn't seem to make the code any
more complicated. However, this would imply that there is no usage of keys()
which access the result like an array, I didn't check this.

Alternatively, we could create an array like below:

  let pages = [];
  for (let key of this._map.keys())
    pages.push(ext.getPage(key));
  return pages;

In the end it doesn't matter much, so as far as I'm concerned, just pick the
approach that seems best to you.

[kzar, 2017-05-23 16:15:04]

LGTM

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29445648/diff/29445659/ext/background.js#n...
ext/background.js:38: return Array.from(this._map.keys()).map(ext.getPage);
On 2017/05/23 15:09:18, Sebastian Noack wrote:
> On 2017/05/23 12:32:27, Manish Jethani wrote:
> > On 2017/05/23 12:16:54, Sebastian Noack wrote:
> > > On 2017/05/23 11:47:59, kzar wrote:
> > > > I wonder if you could do `return
> Array.prototype.map.call(this._map.keys(),
> > > > ext.getPage)`?
> > > 
> > > Please don't. Array generics are non-standard and deprecated:
> > >
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> > 
> > I think Dave meant Array.prototype.map rather than Array.map. The former
isn't
> > going anywhere, though for better or worse it won't work with a
non-array-like
> > object.
> > 
> > To map an iterable regardless of its type you have to use a function like
> this: 
> > 
> > https://codereview.adblockplus.org/29417597/patch/29417598/29417600
> > 
> > If you have generators support, then this: 
> > 
> > function* mapIterable(iterable, callback)
> > {
> >   for (let entry of iterable)
> >     yield callback(entry);
> > }
> > 
> > Here we don't have too many keys though, it's OK to create an intermediate
> > array.
> 
> Sorry, you are right, I mixed things up, calling array method's through the
> prototype isn't generally an issue, but as you pointed out it only works with
> array-like objects.
> 
> I wouldn't be opposed to make keys() a generator function. All browsers this
> code is running on support generators, and it doesn't seem to make the code
any
> more complicated. However, this would imply that there is no usage of keys()
> which access the result like an array, I didn't check this.
> 
> Alternatively, we could create an array like below:
> 
>   let pages = [];
>   for (let key of this._map.keys())
>     pages.push(ext.getPage(key));
>   return pages;
> 
> In the end it doesn't matter much, so as far as I'm concerned, just pick the
> approach that seems best to you.

Acknowledged.