Issue 4364 - Drop support for Chrome 29-40 and remove legacy code

Issue 4364 - Drop support for Chrome 29-40 and remove legacy code

[kzar, 2016-08-30 13:14:09]

I really like how you've refactored the element hiding logic in
include.preload.js. Mostly Looks good to Davey, just a few nits.

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

https://codereview.adblockplus.org/29350213/diff/29350230/chrome/ext/backgrou...
chrome/ext/background.js:478: // we are looking for the frame that contains the
element that
Mind making this comment a bit clearer (or removing it) while at it? I find it
quite hard to understand.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:338: function runInDocument(fn, arg)
Since this no longer takes a document argument perhaps it should be called
runInPage again? (Or something else?)

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:435: this.shadow = this.createShadowTree();
I don't mind too much but perhaps this.shadow should instead be a getter that
caches the result?

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:456: // Use Shadow DOM if available to don't mess with web
pages that rely
Nit: Should be "...if available as to not mess with..."

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:468: var shadow =
document.documentElement.createShadowRoot();
If we don't turn shadow into a getter perhaps we should just assign this.shadow
here instead of having the shadow variable?

[Sebastian Noack, 2016-09-09 14:39:51]

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

https://codereview.adblockplus.org/29350213/diff/29350230/chrome/ext/backgrou...
chrome/ext/background.js:478: // we are looking for the frame that contains the
element that
On 2016/08/30 13:14:09, kzar wrote:
> Mind making this comment a bit clearer (or removing it) while at it? I find it
> quite hard to understand.

Well, this comment isn't new and it makes sense to me. We are looking for the
frame that contains the element this request belongs to. If that element is
loading a subdocument (e.g. if the element is an <iframe>), the frame that
contains it is indicated by the parentFrameId property. However, if for example
it's an <img> element, the frame that contains it is indicated by the frameId
property. Feel free to make a suggestion how to better phrase it.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:338: function runInDocument(fn, arg)
On 2016/08/30 13:14:09, kzar wrote:
> Since this no longer takes a document argument perhaps it should be called
> runInPage again? (Or something else?)

How about runInPageContext()? No preference though, I don't think there is a
good term for that.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:435: this.shadow = this.createShadowTree();
On 2016/08/30 13:14:09, kzar wrote:
> I don't mind too much but perhaps this.shadow should instead be a getter that
> caches the result?

Why adding more code just for the matter of using a getter? Note the comment
explaining why we have to create the shadow root no matter what.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:456: // Use Shadow DOM if available to don't mess with web
pages that rely
On 2016/08/30 13:14:09, kzar wrote:
> Nit: Should be "...if available as to not mess with..."

Done.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:468: var shadow =
document.documentElement.createShadowRoot();
On 2016/08/30 13:14:09, kzar wrote:
> If we don't turn shadow into a getter perhaps we should just assign
this.shadow
> here instead of having the shadow variable?

We still need to indicate whether we are using shadow DOM, in order to convert
the selectors accordingly. So while this would slightly simplify the code
injecting the stylesheet, it would require adding another property.

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

https://codereview.adblockplus.org/29350213/diff/29352571/chrome/ext/backgrou...
chrome/ext/background.js:113: function createFrame(tabId, frameId)
The changes here are due to rebasing.

[kzar, 2016-09-13 12:30:02]

Up to you about that confusing comment, otherwise LGTM.

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

https://codereview.adblockplus.org/29350213/diff/29350230/chrome/ext/backgrou...
chrome/ext/background.js:478: // we are looking for the frame that contains the
element that
On 2016/09/09 14:39:50, Sebastian Noack wrote:
> On 2016/08/30 13:14:09, kzar wrote:
> > Mind making this comment a bit clearer (or removing it) while at it? I find
it
> > quite hard to understand.
> 
> Well, this comment isn't new and it makes sense to me. We are looking for the
> frame that contains the element this request belongs to. If that element is
> loading a subdocument (e.g. if the element is an <iframe>), the frame that
> contains it is indicated by the parentFrameId property. However, if for
example
> it's an <img> element, the frame that contains it is indicated by the frameId
> property. Feel free to make a suggestion how to better phrase it.

Fair enough, well how about this?

"We are looking for the frame that contains the element which has triggered this
request. For most requests (e.g. images) we can just use the request's frame ID,
but for subdocument requests (e.g. iframes) we must instead use the request's
parent frame ID."

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:338: function runInDocument(fn, arg)
On 2016/09/09 14:39:50, Sebastian Noack wrote:
> On 2016/08/30 13:14:09, kzar wrote:
> > Since this no longer takes a document argument perhaps it should be called
> > runInPage again? (Or something else?)
> 
> How about runInPageContext()? No preference though, I don't think there is a
> good term for that.

Sounds good to me.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:435: this.shadow = this.createShadowTree();
On 2016/09/09 14:39:51, Sebastian Noack wrote:
> On 2016/08/30 13:14:09, kzar wrote:
> > I don't mind too much but perhaps this.shadow should instead be a getter
that
> > caches the result?
> 
> Why adding more code just for the matter of using a getter? Note the comment
> explaining why we have to create the shadow root no matter what.

Acknowledged.

https://codereview.adblockplus.org/29350213/diff/29350230/include.preload.js#...
include.preload.js:468: var shadow =
document.documentElement.createShadowRoot();
On 2016/09/09 14:39:50, Sebastian Noack wrote:
> On 2016/08/30 13:14:09, kzar wrote:
> > If we don't turn shadow into a getter perhaps we should just assign
> this.shadow
> > here instead of having the shadow variable?
> 
> We still need to indicate whether we are using shadow DOM, in order to convert
> the selectors accordingly. So while this would slightly simplify the code
> injecting the stylesheet, it would require adding another property.

Acknowledged.

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

https://codereview.adblockplus.org/29350213/diff/29352571/chrome/ext/backgrou...
chrome/ext/background.js:113: function createFrame(tabId, frameId)
On 2016/09/09 14:39:51, Sebastian Noack wrote:
> The changes here are due to rebasing.

Acknowledged.

[Sebastian Noack, 2016-09-13 15:21:30]

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

https://codereview.adblockplus.org/29350213/diff/29350230/chrome/ext/backgrou...
chrome/ext/background.js:478: // we are looking for the frame that contains the
element that
On 2016/09/13 12:30:01, kzar wrote:
> On 2016/09/09 14:39:50, Sebastian Noack wrote:
> > On 2016/08/30 13:14:09, kzar wrote:
> > > Mind making this comment a bit clearer (or removing it) while at it? I
find
> it
> > > quite hard to understand.
> > 
> > Well, this comment isn't new and it makes sense to me. We are looking for
the
> > frame that contains the element this request belongs to. If that element is
> > loading a subdocument (e.g. if the element is an <iframe>), the frame that
> > contains it is indicated by the parentFrameId property. However, if for
> example
> > it's an <img> element, the frame that contains it is indicated by the
frameId
> > property. Feel free to make a suggestion how to better phrase it.
> 
> Fair enough, well how about this?
> 
> "We are looking for the frame that contains the element which has triggered
this
> request. For most requests (e.g. images) we can just use the request's frame
ID,
> but for subdocument requests (e.g. iframes) we must instead use the request's
> parent frame ID."

Done.

[kzar, 2016-09-13 17:08:58]

LGTM

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

https://codereview.adblockplus.org/29350213/diff/29350230/chrome/ext/backgrou...
chrome/ext/background.js:478: // we are looking for the frame that contains the
element that
On 2016/09/13 15:21:30, Sebastian Noack wrote:
> On 2016/09/13 12:30:01, kzar wrote:
> > On 2016/09/09 14:39:50, Sebastian Noack wrote:
> > > On 2016/08/30 13:14:09, kzar wrote:
> > > > Mind making this comment a bit clearer (or removing it) while at it? I
> find
> > it
> > > > quite hard to understand.
> > > 
> > > Well, this comment isn't new and it makes sense to me. We are looking for
> the
> > > frame that contains the element this request belongs to. If that element
is
> > > loading a subdocument (e.g. if the element is an <iframe>), the frame that
> > > contains it is indicated by the parentFrameId property. However, if for
> > example
> > > it's an <img> element, the frame that contains it is indicated by the
> frameId
> > > property. Feel free to make a suggestion how to better phrase it.
> > 
> > Fair enough, well how about this?
> > 
> > "We are looking for the frame that contains the element which has triggered
> this
> > request. For most requests (e.g. images) we can just use the request's frame
> ID,
> > but for subdocument requests (e.g. iframes) we must instead use the
request's
> > parent frame ID."
> 
> Done.

Looks great, thanks.