Issue 704 - Generate protocol-agnostic request blocking filters with "Block element"

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -279,11 +279,6 @@
   else if (elt.src)
     url = elt.src;
 
-  // Only normalize when the element contains a URL (issue 328.)
-  // The URL is not always normalized, so do it here
-  if (url)
-    url = normalizeURL(relativeToAbsoluteUrl(url));
-
   // Construct filters. The popup will retrieve these.
   // Only one ID
   var elementId = elt.id ? elt.id.split(' ').join('') : null;
@@ -316,7 +311,9 @@
 
   if (url)
   {
-    clickHideFilters.push(relativeToAbsoluteUrl(url));
+    url = relativeToAbsoluteUrl(url);
+
+    clickHideFilters.push(normalizeURL(url));

[Wladimir Palant, 2014/06/27 07:30:53]

I wonder what will happen to src="about:blank" or src="data:..."

[Sebastian Noack, 2014/06/28 09:42:50]

See http://codereview.adblockplus.org/5945877571043328/

     selectorList.push(elt.localName + '[src="' + url + '"]');

[Wladimir Palant, 2014/06/27 07:30:53]

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?

[Sebastian Noack, 2014/06/28 09:42:50]

Done.

   }
 
@@ -397,15 +394,13 @@
 // Does some degree of URL normalization
 function normalizeURL(url)
 {
-  var components = url.match(/(.+:\/\/.+?)\/(.*)/);
+  var components = url.match(/(.+:\/\/)(.+?)\/(.*)/);

[Wladimir Palant, 2014/06/27 07:30:53]

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.

[Sebastian Noack, 2014/06/28 09:42:50]

Any reasons why we "normalize" the URL in the first place?

[Sebastian Noack, 2014/09/25 08:36:25]

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.

   if(!components)
     return url;
-  var newPath = removeDotSegments(components[2]);
-  if(newPath.length == 0)
-    return components[1];
-  if(newPath[0] != '/')
+  var newPath = removeDotSegments(components[3]);
+  if(newPath != '' && newPath[0] != '/')

[Wladimir Palant, 2014/06/27 07:30:53]

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.

[Sebastian Noack, 2014/06/28 09:42:50]

Then you can just check for newPath[0] != "/" with the same result.
Done.

     newPath = '/' + newPath;
-  return components[1] + newPath;
+  return '||' + components[2] + newPath;
 }
 
 // Content scripts are apparently invoked on non-HTML documents, so we have to