Isssue 3459 - Map the content type IMAGESET to IMAGE and FETCH to XMLHTTPREQUEST

Isssue 3459 - Map the content type IMAGESET to IMAGE and FETCH to XMLHTTPREQUEST

[Erik, 2015-12-28 17:04:36]

LGTM

[Wladimir Palant, 2015-12-29 23:19:48]

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.j...
lib/contentPolicy.js:48: ]),
This member becomes unnecessary with your change, it should be removed - the map
is all that's needed. IMHO, contentTypesMap should be renamed into contentTypes.
Initialization could go like this:

contentTypes: new Map([
  ["BEACON", "PING"],
  ...
].concat([
  // Everything else is mapped to itself
  "OTHER", "SCRIPT", ...
].map(x => [x, x])));

[Sebastian Noack, 2015-12-30 15:54:58]

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.j...
lib/contentPolicy.js:48: ]),
On 2015/12/29 23:19:48, Wladimir Palant wrote:
> This member becomes unnecessary with your change, it should be removed - the
map
> is all that's needed. IMHO, contentTypesMap should be renamed into
contentTypes.
> Initialization could go like this:
> 
> contentTypes: new Map([
>   ["BEACON", "PING"],
>   ...
> ].concat([
>   // Everything else is mapped to itself
>   "OTHER", "SCRIPT", ...
> ].map(x => [x, x])));

Note that chrome/content/ui/sidebar.js and chrome/content/ui/composer.js use the
contentTypes property as well and expect a unique list of these content types.

[Wladimir Palant, 2016-01-06 12:46:01]

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.j...
lib/contentPolicy.js:48: ]),
On 2015/12/30 15:54:58, Sebastian Noack wrote:
> Note that chrome/content/ui/sidebar.js and chrome/content/ui/composer.js use
the
> contentTypes property as well and expect a unique list of these content types.

You are right but that's not a reason to duplicate the data structure. They can
rather use new Set(Policy.contentTypes.values()).

[Sebastian Noack, 2016-01-06 13:29:50]

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29333097/diff/29333100/lib/contentPolicy.j...
lib/contentPolicy.js:48: ]),
On 2016/01/06 12:46:00, Wladimir Palant wrote:
> On 2015/12/30 15:54:58, Sebastian Noack wrote:
> > Note that chrome/content/ui/sidebar.js and chrome/content/ui/composer.js use
> the
> > contentTypes property as well and expect a unique list of these content
types.
> 
> You are right but that's not a reason to duplicate the data structure. They
can
> rather use new Set(Policy.contentTypes.values()).

There you go: https://codereview.adblockplus.org/29333254