Issue 5132310882025472: Issue 704 - Generate protocol-agnostic request blocking filters with "Block element" (Closed)
|
Created:
June 24, 2014, 1:38 p.m. by Sebastian Noack Modified:
Oct. 10, 2014, 6:31 p.m. Visibility:
Public. |
DescriptionIssue 704 - Generate protocol-agnostic request blocking filters with "Block element"
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Rebased and simplified changes #
Total comments: 8
Patch Set 4 : Addressed comments #MessagesTotal messages: 13
Most of the comments below apply to issues that were already there - the code you touched here really isn't great. http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:316: clickHideFilters.push(normalizeURL(url)); I wonder what will happen to src="about:blank" or src="data:..." http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:317: selectorList.push(elt.localName + '[src="' + url + '"]'); Just noticed - this one won't work correctly, it needs the original value of the attribute rather than the absolute URL. Not that this filter makes sense - blocking via webRequest makes a lot more sense than hiding by src attribute. Remove that line? http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:397: var components = url.match(/(.+:\/\/)(.+?)\/(.*)/); We should really start using the URI class from basedomain.js in the content scripts :-( Maybe extract that class into a separate file and load it as a content script? Not really something you need to do here but maybe later. http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:401: if(newPath != '' && newPath[0] != '/') It should actually be: newPath == "" || newPath[0] != "/" (yes, this isn't what the original code did). There should always be a slash after the host name. Style nit: please use double quotation marks in JavaScript.
http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:316: clickHideFilters.push(normalizeURL(url)); On 2014/06/27 07:30:53, Wladimir Palant wrote: > I wonder what will happen to src="about:blank" or src="data:..." See http://codereview.adblockplus.org/5945877571043328/ http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:317: selectorList.push(elt.localName + '[src="' + url + '"]'); On 2014/06/27 07:30:53, Wladimir Palant wrote: > Just noticed - this one won't work correctly, it needs the original value of the > attribute rather than the absolute URL. Not that this filter makes sense - > blocking via webRequest makes a lot more sense than hiding by src attribute. > Remove that line? Done. http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:397: var components = url.match(/(.+:\/\/)(.+?)\/(.*)/); On 2014/06/27 07:30:53, Wladimir Palant wrote: > We should really start using the URI class from basedomain.js in the content > scripts :-( > > Maybe extract that class into a separate file and load it as a content script? > Not really something you need to do here but maybe later. Any reasons why we "normalize" the URL in the first place? http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:401: if(newPath != '' && newPath[0] != '/') On 2014/06/27 07:30:53, Wladimir Palant wrote: > It should actually be: newPath == "" || newPath[0] != "/" Then you can just check for newPath[0] != "/" with the same result. > Style nit: please use double quotation marks in JavaScript. Done.
http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5144752345317376/incl... include.postload.js:397: var components = url.match(/(.+:\/\/)(.+?)\/(.*)/); On 2014/06/27 07:30:53, Wladimir Palant wrote: > We should really start using the URI class from basedomain.js in the content > scripts :-( > > Maybe extract that class into a separate file and load it as a content script? > Not really something you need to do here but maybe later. As discussed in http://codereview.adblockplus.org/4567408790470656/diff/5634387206995968/incl... this code will be gone by https://issues.adblockplus.org/ticket/1441 and a temporary <a> element will be used to resolve URLs with its href attribute. I have uploaded a new patch set that just replaces the leading protocol scheme with "||" in the already normalized URL.
http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:394: clickHideFilters.push(url.replace(/.*?:\/\//, '||')); Nit: Use double instead of single quotation marks for strings.
Note: the comments below apply to patch set 2, patch set 3 doesn't seem to make much sense. http://codereview.adblockplus.org/5132310882025472/diff/5685265389584384/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5685265389584384/incl... include.postload.js:390: function normalizeURL(url) The function name and comment no longer make sense. It's really urlToFilter() now. http://codereview.adblockplus.org/5132310882025472/diff/5685265389584384/incl... include.postload.js:392: var components = url.match(/(.+:\/\/)(.+?)\/(.*)/); This regexp won't work correctly for something like http://foo/?http://bar/", the protocol part shouldn't be greedy. Besides, I would prefer an explicit ^ at the beginning of the regexp. http://codereview.adblockplus.org/5132310882025472/diff/5685265389584384/incl... include.postload.js:394: return url; I guess this would make more sense: return "|" + url; http://codereview.adblockplus.org/5132310882025472/diff/5685265389584384/incl... include.postload.js:395: var newPath = removeDotSegments(components[3]); Do we still need the "dot removal"? This should be taken care of the relativeToAbsoluteURL() function.
On 2014/09/30 21:27:42, Wladimir Palant wrote: > Note: the comments below apply to patch set 2, patch set 3 doesn't seem to make > much sense. How do you mean? Note that the function normalizeURL() no longer exist, since we let the browser normalize the URL via the element's src attribute. So patch set 1 and 2 don't make sense anymore.
Ok, it seems that only one change was dropped in Patch Set 3. http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:394: clickHideFilters.push(url.replace(/.*?:\/\//, '||')); It makes sense to use the same regular expression here as in composer.js in Firefox (which in turn is based on the regular expression actually used by RegExpFilter): /^[\w\-]+:\/+(?:www\.)?/ http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:395: selectorList.push(elt.localName + '[src="' + elt.getAttribute("src") + '"]'); Please remove this line, your previous patch did this.
http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:394: clickHideFilters.push(url.replace(/.*?:\/\//, '||')); On 2014/09/30 11:10:53, Thomas Greiner wrote: > Nit: Use double instead of single quotation marks for strings. Done. http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:394: clickHideFilters.push(url.replace(/.*?:\/\//, '||')); On 2014/10/01 18:00:45, Wladimir Palant wrote: > It makes sense to use the same regular expression here as in composer.js in > Firefox (which in turn is based on the regular expression actually used by > RegExpFilter): > > /^[\w\-]+:\/+(?:www\.)?/ Done. http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:395: selectorList.push(elt.localName + '[src="' + elt.getAttribute("src") + '"]'); On 2014/10/01 18:00:45, Wladimir Palant wrote: > Please remove this line, your previous patch did this. How do you mean? I didn't add this line, I just fixed it, to use the actual "src" attribute instead the normalized URL, which won't match.
http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:395: selectorList.push(elt.localName + '[src="' + elt.getAttribute("src") + '"]'); As mentioned before, a hiding filter based on the src attribute makes exactly zero sense - we shouldn't suggest it. And, yes - I know that you didn't introduce that code.
http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... File include.postload.js (right): http://codereview.adblockplus.org/5132310882025472/diff/5676830073815040/incl... include.postload.js:395: selectorList.push(elt.localName + '[src="' + elt.getAttribute("src") + '"]'); On 2014/10/01 18:26:51, Wladimir Palant wrote: > As mentioned before, a hiding filter based on the src attribute makes exactly > zero sense - we shouldn't suggest it. And, yes - I know that you didn't > introduce that code. We don't generate a filer here. This list is only used to highlight matching elements, while the "Block Element" shows.
LGTM
LGTM 
|
