Issue 2064 - Configure indistinguishable request types in the abstraction layer

Issue 2064 - Configure indistinguishable request types in the abstraction layer

[kzar, 2015-03-02 16:56:53]

LGTM

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
safari/ext/content.js:84: type = "MEDIA";
One thought I had was that you could add most of these as indistinguishable type
arrays and therefore simplify this logic. Not sure if it's a good idea though.

[Sebastian Noack, 2015-03-02 16:59:26]

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
safari/ext/content.js:84: type = "MEDIA";
On 2015/03/02 16:56:53, kzar wrote:
> One thought I had was that you could add most of these as indistinguishable
type
> arrays and therefore simplify this logic. Not sure if it's a good idea though.

All types handled here are actually distinguishable. So this doesn't make any
sense to me. And regardless of the indistinguishable types, I need to map the
types that are actually distinguishable.

[kzar, 2015-03-02 17:04:29]

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
safari/ext/content.js:84: type = "MEDIA";
On 2015/03/02 16:59:26, Sebastian Noack wrote:
> On 2015/03/02 16:56:53, kzar wrote:
> > One thought I had was that you could add most of these as indistinguishable
> type
> > arrays and therefore simplify this logic. Not sure if it's a good idea
though.
> 
> All types handled here are actually distinguishable. So this doesn't make any
> sense to me. And regardless of the indistinguishable types, I need to map the
> types that are actually distinguishable.

So for example I thought you could add ["MEDIA", "video", "audio", "source"] to
the indistinguishable types array and therefore not need the logic here that you
added, you would just need to set type to event.target.localName and have a bit
of logic for the special cases like "iframe" where something else happens too.

Again I'm not sure that it's actually a good idea, just a thought.

[Sebastian Noack, 2015-03-02 17:08:49]

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/safa...
safari/ext/content.js:84: type = "MEDIA";
On 2015/03/02 17:04:29, kzar wrote:
> On 2015/03/02 16:59:26, Sebastian Noack wrote:
> > On 2015/03/02 16:56:53, kzar wrote:
> > > One thought I had was that you could add most of these as
indistinguishable
> > type
> > > arrays and therefore simplify this logic. Not sure if it's a good idea
> though.
> > 
> > All types handled here are actually distinguishable. So this doesn't make
any
> > sense to me. And regardless of the indistinguishable types, I need to map
the
> > types that are actually distinguishable.
> 
> So for example I thought you could add ["MEDIA", "video", "audio", "source"]
to
> the indistinguishable types array and therefore not need the logic here that
you
> added, you would just need to set type to event.target.localName and have a
bit
> of logic for the special cases like "iframe" where something else happens too.
> 
> Again I'm not sure that it's actually a good idea, just a thought.

This won't work, since "video", "audio" and "source" aren't valid types, but
merely tag names.

[Wladimir Palant, 2015-03-02 19:12:18]

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/chro...
chrome/ext/background.js:340: ["OTHER", "OBJECT", "OBJECT_SUBREQUEST", "MEDIA",
"FONT"]
This will overshoot. With the previous approach, a filter with $media option
wouldn't apply to objects. Indeed, this filter wouldn't apply to anything due to
our hack - but that was ok given that there aren't so many rules applying to
audio/video but many for objects. Now this filter will apply to object
subrequests and even objects that have been correctly reports as such by Chrome.

[Sebastian Noack, 2015-03-02 19:23:11]

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/6228201961422848/diff/5629499534213120/chro...
chrome/ext/background.js:340: ["OTHER", "OBJECT", "OBJECT_SUBREQUEST", "MEDIA",
"FONT"]
On 2015/03/02 19:12:18, Wladimir Palant wrote:
> This will overshoot. With the previous approach, a filter with $media option
> wouldn't apply to objects. Indeed, this filter wouldn't apply to anything due
to
> our hack - but that was ok given that there aren't so many rules applying to
> audio/video but many for objects. Now this filter will apply to object
> subrequests and even objects that have been correctly reports as such by
Chrome.

I felt that it's more correct this way. But fair enough. I restored the previous
behavior, ignoring MEDIA and FONT filters again.

[Wladimir Palant, 2015-03-02 20:29:49]

LGTM and I hope that issue will finally resolved soon :-(