Issue 3453 - Adapt indistinguishable request types for changes in Chrome 49

Issue 3453 - Adapt indistinguishable request types for changes in Chrome 49

[Sebastian Noack, 2015-12-23 12:11:54]

I think we should get that into the 1.10 release.

[Wladimir Palant, 2015-12-23 12:50:51]

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
chrome/ext/background.js:380: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
This logic is very hard to follow, and I'm not really sure it is correct either.
I suggest you structure it like this:

  var types = [];
  var version = (match ? parseInt(match[1], 10) : Infinity);

  // bla-bla objects
  if (version >= 38 || version <= 48)
    types.push(["OTHER", "OBJECT", "OBJECT_SUBREQUEST"]);
  else
    types.push(["OBJECT", "OBJECT_SUBREQUEST"]);

  // bla-bla font and ping
  ...

  return types;

[Sebastian Noack, 2015-12-23 13:08:43]

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
chrome/ext/background.js:380: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2015/12/23 12:50:51, Wladimir Palant wrote:
> This logic is very hard to follow, and I'm not really sure it is correct
either.
> I suggest you structure it like this:
> 
>   var types = [];
>   var version = (match ? parseInt(match[1], 10) : Infinity);
> 
>   // bla-bla objects
>   if (version >= 38 || version <= 48)
>     types.push(["OTHER", "OBJECT", "OBJECT_SUBREQUEST"]);
>   else
>     types.push(["OBJECT", "OBJECT_SUBREQUEST"]);
> 
>   // bla-bla font and ping
>   ...
> 
>   return types;

This won't work. If the workaround for Chrome 38-48 is applied the
indistinguishable types should simply be [["OTHER", "OBJECT",
"OBJECT_SUBREQUEST"]]. Yes, that means that other types that cannot be
distinguished (i.e. media and font) are ignored rather than mapped. However,
this has been the behavior since we used that workaround. And changing that
behavior would bear the risk of introducing false positives due to overly greedy
matching.

Also to be honest, I think that my implementation makes it more obvious that we
never apply the workaround if we cannot detect the Chromium version. That is
important for Kitt.

[Wladimir Palant, 2015-12-23 14:39:18]

I cannot say that I like the logic there but - LGTM.

[kzar, 2015-12-23 14:43:20]

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
chrome/ext/background.js:363: var match =
navigator.userAgent.match(/\bChrome\/(\d+)/);
We already parse the application name and version in the info module,  how come
we're doing it again here?

[Sebastian Noack, 2015-12-23 14:46:56]

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
chrome/ext/background.js:363: var match =
navigator.userAgent.match(/\bChrome\/(\d+)/);
On 2015/12/23 14:43:20, kzar wrote:
> We already parse the application name and version in the info module,  how
come
> we're doing it again here?

adblockplus.js (including the info module) depends on ext, and as ext is loaded
first, the adblockplus.js might not be avialable yet. Moreover, circular
dependencies are generally bad practice.

[kzar, 2015-12-23 14:56:42]

LGTM

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29333035/diff/29333036/chrome/ext/backgrou...
chrome/ext/background.js:363: var match =
navigator.userAgent.match(/\bChrome\/(\d+)/);
On 2015/12/23 14:46:56, Sebastian Noack wrote:
> On 2015/12/23 14:43:20, kzar wrote:
> > We already parse the application name and version in the info module,  how
> come
> > we're doing it again here?
> 
> adblockplus.js (including the info module) depends on ext, and as ext is
loaded
> first, the adblockplus.js might not be avialable yet. Moreover, circular
> dependencies are generally bad practice.

Acknowledged.