Issue 4722 - Drop support for Chrome 41 - 48

Issue 4722 - Drop support for Chrome 41 - 48

[kzar, 2016-12-13 10:55:48]

Patch Set 1

[Sebastian Noack, 2016-12-13 11:09:12]

Generally, we also strip all obvious legacy code when advancing the minimum
Chrome version. I've seen some legacy code in chrome/ext/background.js and
lib/csp.js which is referring to Chrome versions that aren't supported anymore.
There might be more. Mind stripping that code while on it?

[kzar, 2016-12-13 13:59:44]

Patch Set 2 : Removed some legacy code

> Mind stripping that code while on it?

Good idea, I've had a go. (I drew the line at replacing `var` with `let` and
functions with arrow functions everywhere!)

I've tested that icon animations still work, the CSP stuff still works, the
subscription link at ​https://testpages.adblockplus.org still works (except the
bug to be fixed with #4680). I've tested browsing a few sites adblocking seems
to work as before with no exceptions in the background / content consoles etc. I
did that testing with Chrome 49.

I wasn't sure how to test that I didn't break something with the
indistinguishable types, mutation observer and chrome.runtime.onMessage
listener.

[Wladimir Palant, 2016-12-13 14:15:02]

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
OBJECT and OBJECT_SUBREQUEST are still indistinguishable as of Chrome 55 and
this is unlikely to change. Looks like you removed a bit too much code.

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js
File lib/csp.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js#oldcode43
lib/csp.js:43: // https://www.chromestatus.com/feature/6653486812889088
This comment still adds some value, http: also including https: is non-obvious.
Only the version number is no longer important.

https://codereview.adblockplus.org/29367316/diff/29367322/lib/requestBlocker.js
File lib/requestBlocker.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/lib/requestBlocker....
lib/requestBlocker.js:36: });
This code is still needed to deal with OBJECT and OBJECT_SUBREQUEST.

[Sebastian Noack, 2016-12-13 14:35:39]

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/13 14:15:02, Wladimir Palant wrote:
> OBJECT and OBJECT_SUBREQUEST are still indistinguishable as of Chrome 55 and
> this is unlikely to change. Looks like you removed a bit too much code.

Yep. But let's hard-code that like we initially did, and still get rid of
getIndistinguishableTypes() which mainly existed to abstract the differences
between Chrome and Safari here.

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js
File lib/csp.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js#oldcode44
lib/csp.js:44: value: "connect-src http: https:; frame-src http: https:; " +
It seems we don't support any Chrome version anymore that doesn't support
child-src. Hence frame-src is redundant at least as far as Chrome is concerned,
since if both are given frame-src is ignored.

Not sure about Edge though. On the other hand, from the comment above, it seems
adding frame-src and child-src was only neccessary to workaround a Chrome bug in
the first place.

[Wladimir Palant, 2016-12-13 14:46:35]

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/13 14:35:39, Sebastian Noack wrote:
> Yep. But let's hard-code that like we initially did, and still get rid of
> getIndistinguishableTypes() which mainly existed to abstract the differences
> between Chrome and Safari here.

Not sure whether that's a good idea - with Firefox and Edge we might get more
complicated logic here again soon.

[kzar, 2016-12-13 15:05:40]

Patch Set 3 : Brought back http CSP comment + indistinguishableTypes

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/13 14:46:35, Wladimir Palant wrote:
> On 2016/12/13 14:35:39, Sebastian Noack wrote:
> > Yep. But let's hard-code that like we initially did, and still get rid of
> > getIndistinguishableTypes() which mainly existed to abstract the differences
> > between Chrome and Safari here.
> 
> Not sure whether that's a good idea - with Firefox and Edge we might get more
> complicated logic here again soon.

Done.

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js
File lib/csp.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js#oldcode43
lib/csp.js:43: // https://www.chromestatus.com/feature/6653486812889088
On 2016/12/13 14:15:02, Wladimir Palant wrote:
> This comment still adds some value, http: also including https: is
non-obvious.
> Only the version number is no longer important.

Done.

https://codereview.adblockplus.org/29367316/diff/29367322/lib/csp.js#oldcode44
lib/csp.js:44: value: "connect-src http: https:; frame-src http: https:; " +
On 2016/12/13 14:35:39, Sebastian Noack wrote:
> It seems we don't support any Chrome version anymore that doesn't support
> child-src. Hence frame-src is redundant at least as far as Chrome is
concerned,
> since if both are given frame-src is ignored.
> 
> Not sure about Edge though. On the other hand, from the comment above, it
seems
> adding frame-src and child-src was only neccessary to workaround a Chrome bug
in
> the first place.

Done.

https://codereview.adblockplus.org/29367316/diff/29367322/lib/requestBlocker.js
File lib/requestBlocker.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/lib/requestBlocker....
lib/requestBlocker.js:36: });
On 2016/12/13 14:15:02, Wladimir Palant wrote:
> This code is still needed to deal with OBJECT and OBJECT_SUBREQUEST.

Done.

[Sebastian Noack, 2016-12-13 15:06:58]

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/13 14:46:35, Wladimir Palant wrote:
> On 2016/12/13 14:35:39, Sebastian Noack wrote:
> > Yep. But let's hard-code that like we initially did, and still get rid of
> > getIndistinguishableTypes() which mainly existed to abstract the differences
> > between Chrome and Safari here.
> 
> Not sure whether that's a good idea - with Firefox and Edge we might get more
> complicated logic here again soon.

Well, ext is matter to go away, not that we need any platform-level abstraction
for that. That mapping could/should be performed at runtime (outside of ext)
based on the values you'll find in webRequest.ResourceType:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/Reso...

[kzar, 2016-12-13 15:12:54]

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/13 15:06:58, Sebastian Noack wrote:
> On 2016/12/13 14:46:35, Wladimir Palant wrote:
> > On 2016/12/13 14:35:39, Sebastian Noack wrote:
> > > Yep. But let's hard-code that like we initially did, and still get rid of
> > > getIndistinguishableTypes() which mainly existed to abstract the
differences
> > > between Chrome and Safari here.
> > 
> > Not sure whether that's a good idea - with Firefox and Edge we might get
more
> > complicated logic here again soon.
> 
> Well, ext is matter to go away, not that we need any platform-level
abstraction
> for that. That mapping could/should be performed at runtime (outside of ext)
> based on the values you'll find in webRequest.ResourceType:
>
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/Reso...

I don't mind too much either way, if it's hard-coded or not, but perhaps
Wladimir has a point since we're likely adding the Edge and Firefox platforms in
the near future. Do you insist I remove the function?

[Sebastian Noack, 2016-12-14 12:39:53]

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/13 15:12:54, kzar wrote:
> On 2016/12/13 15:06:58, Sebastian Noack wrote:
> > On 2016/12/13 14:46:35, Wladimir Palant wrote:
> > > On 2016/12/13 14:35:39, Sebastian Noack wrote:
> > > > Yep. But let's hard-code that like we initially did, and still get rid
of
> > > > getIndistinguishableTypes() which mainly existed to abstract the
> differences
> > > > between Chrome and Safari here.
> > > 
> > > Not sure whether that's a good idea - with Firefox and Edge we might get
> more
> > > complicated logic here again soon.
> > 
> > Well, ext is matter to go away, not that we need any platform-level
> abstraction
> > for that. That mapping could/should be performed at runtime (outside of ext)
> > based on the values you'll find in webRequest.ResourceType:
> >
>
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/Reso...
> 
> I don't mind too much either way, if it's hard-coded or not, but perhaps
> Wladimir has a point since we're likely adding the Edge and Firefox platforms
in
> the near future. Do you insist I remove the function?

Again, the differences in Firefox and Edge aren't meant to be abstracted by yet
another ext backend, as we agreed to get rid of ext altogether eventually. In
the particular case here there is no need for platform specific code anyway.
Yes, Firefox uses a different (more granular) set of request types. But there is
no need for a special-case for Firefox. Instead, you can and should do feature
detection and map the values given in webRequest.ResourceType accordingly. This
can be done in lib/requestBlocker.js.

[kzar, 2016-12-16 09:38:35]

Patch Set 4 : Removed getIndistinguishableTypes function

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
File chrome/ext/background.js (left):

https://codereview.adblockplus.org/29367316/diff/29367322/chrome/ext/backgrou...
chrome/ext/background.js:467: return [["OBJECT", "OBJECT_SUBREQUEST"],
otherTypes];
On 2016/12/14 12:39:53, Sebastian Noack wrote:
> On 2016/12/13 15:12:54, kzar wrote:
> > On 2016/12/13 15:06:58, Sebastian Noack wrote:
> > > On 2016/12/13 14:46:35, Wladimir Palant wrote:
> > > > On 2016/12/13 14:35:39, Sebastian Noack wrote:
> > > > > Yep. But let's hard-code that like we initially did, and still get rid
> of
> > > > > getIndistinguishableTypes() which mainly existed to abstract the
> > differences
> > > > > between Chrome and Safari here.
> > > > 
> > > > Not sure whether that's a good idea - with Firefox and Edge we might get
> > more
> > > > complicated logic here again soon.
> > > 
> > > Well, ext is matter to go away, not that we need any platform-level
> > abstraction
> > > for that. That mapping could/should be performed at runtime (outside of
ext)
> > > based on the values you'll find in webRequest.ResourceType:
> > >
> >
>
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/Reso...
> > 
> > I don't mind too much either way, if it's hard-coded or not, but perhaps
> > Wladimir has a point since we're likely adding the Edge and Firefox
platforms
> in
> > the near future. Do you insist I remove the function?
> 
> Again, the differences in Firefox and Edge aren't meant to be abstracted by
yet
> another ext backend, as we agreed to get rid of ext altogether eventually. In
> the particular case here there is no need for platform specific code anyway.
> Yes, Firefox uses a different (more granular) set of request types. But there
is
> no need for a special-case for Firefox. Instead, you can and should do feature
> detection and map the values given in webRequest.ResourceType accordingly.
This
> can be done in lib/requestBlocker.js.

Done.

[Wladimir Palant, 2016-12-16 10:06:15]

https://codereview.adblockplus.org/29367316/diff/29368658/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29367316/diff/29368658/lib/requestBlocker....
lib/requestBlocker.js:32: // Chrome can not distinguish between
OBJECT_SUBREQUEST and OBJECT requests.
Nit: I learned that it is "cannot" in English ;)

[kzar, 2016-12-16 10:27:35]

Patch Set 5 : Addressed nit

https://codereview.adblockplus.org/29367316/diff/29368658/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29367316/diff/29368658/lib/requestBlocker....
lib/requestBlocker.js:32: // Chrome can not distinguish between
OBJECT_SUBREQUEST and OBJECT requests.
On 2016/12/16 10:06:15, Wladimir Palant wrote:
> Nit: I learned that it is "cannot" in English ;)

Heh good point, well even better I went for "can't".

[Wladimir Palant, 2016-12-16 10:34:09]

No issues as far as I am concerned but this is really up to Sebastian.

[Sebastian Noack, 2016-12-16 11:08:45]

LGTM