Issue 5899 - Use CSS attribute selectors for collapsing media elements

include.preload.js

Index: include.preload.js
===================================================================
--- a/include.preload.js
+++ b/include.preload.js
@@ -30,16 +30,18 @@
   ["audio", "MEDIA"],
   ["video", "MEDIA"],
   ["frame", "SUBDOCUMENT"],
   ["iframe", "SUBDOCUMENT"],
   ["object", "OBJECT"],
   ["embed", "OBJECT"]
 ]);
 
+let collapsingSelectors = new Set();

[kzar, 2018/02/26 17:18:58]

I guess we'll slowly leak memory here, since we never remove the selectors
again. I wonder if it matters and if there's something we can do to avoid that?

[Manish Jethani, 2018/02/27 13:11:49]

I wouldn't call that "leaking memory" since we do need the selectors to stay in
the set (i.e. we don't have a case for removing any selector, and if we did we
would do it).

+
 function getURLsFromObjectElement(element)
 {
   let url = element.getAttribute("data");
   if (url)
     return [url];
 
   for (let child of element.children)
   {
@@ -123,16 +125,51 @@
   {
     if (/^(?!https?:)[\w-]+:/i.test(urls[i]))
       urls.splice(i--, 1);
   }
 
   return urls;
 }
 
+function isCollapsibleMediaElement(element, mediatype)
+{
+  if (mediatype != "MEDIA")
+    return false;
+
+  if (!element.getAttribute("src"))
+    return false;
+
+  for (let child of element.children)
+  {
+    // If the <video> or <audio> element contains any <source> or <track>
+    // children, we cannot address it in CSS by the source URL; in that case we
+    // don't "collapse" it using a CSS selector but rather hide it directly by
+    // setting the style="..." attribute.
+    if (["source", "track"].includes(child.localName))
+      return false;
+  }
+
+  return true;
+}
+
+function collapseMediaElement(element, srcValue)
+{
+  let selector = srcValue ?

[kzar, 2018/02/26 17:18:58]

Nit: It would be clearer to do this IMO:

if (!srcValue)
  return;

let selector = element.localName + "[src=" + CSS.escape(srcValue) + "]";

...

if (!collapsingSelectors.has(selector))

[Manish Jethani, 2018/02/27 13:11:49]

Ah, you're right.

Done.

+                   element.localName + "[src=" + CSS.escape(srcValue) + "]" :

[kzar, 2018/02/26 17:18:58]

Maybe a dumb question but could this end up matching other elements too?

[Manish Jethani, 2018/02/27 13:11:49]

No, that's a good question.

In isCollapsibleMediaElement we're checking if the element is a media element
(<video> or <audio>), if it has an src property, and if it has no child elements
<source> or <track>. If all of these are true, then it's collapsible. Why?
Because getURLsFromMediaElement will return only the value of the src property 
in this case, so we can be certain that whatever element we're collapsing is
based solely on the value of that property.

Since the src property in JS reflects the value of the src attribute in HTML, we
can hide the element based on the value of src attribute using a CSS attribute
selector.

So basically if there are multiple media elements on the page with the same src
attribute value, then they will all be hidden (even if their media hasn't
started loading yet!), and this is a good thing. We do want to hide those.

+                   null;
+  // Adding selectors is expensive so do it only if we really have a new
+  // selector.
+  if (selector && !collapsingSelectors.has(selector))
+  {
+    collapsingSelectors.add(selector);
+    elemhide.addSelectors(Array.from(collapsingSelectors), null, "collapsing");

[Manish Jethani, 2018/02/22 06:55:06]

There's a new "collapsing" group for these selectors.

Ideally we would have an append-only mode for the collapsing selectors since
they are never removed. This would make it more efficient.

[kzar, 2018/02/27 14:18:38]

Good idea, let's do that now?

[Manish Jethani, 2018/02/27 16:00:22]

Done.

+  }
+}
+
 function hideElement(element)
 {
   function doHide()
   {
     let propertyName = "display";
     let propertyValue = "none";
     if (element.localName == "frame")
     {
@@ -160,29 +197,37 @@
   let mediatype = typeMap.get(element.localName);
   if (!mediatype)
     return;
 
   let urls = getURLsFromElement(element);
   if (urls.length == 0)
     return;
 
+  let collapsible = isCollapsibleMediaElement(element, mediatype);

[kzar, 2018/02/26 17:18:58]

IMO this variable name is misleading since non-media elements are collapsible
too right?

[Manish Jethani, 2018/02/27 13:11:49]

Yeah, I suppose you're right.

Updated.

+
+  // Save the value of the src attribute because it can change between now and
+  // when we get the response from the background page.
+  let srcValue = collapsible ? element.getAttribute("src") : null;

[Manish Jethani, 2018/02/22 06:55:06]

If the pre-roll ad is blocked then pretty quickly the page continues on to the
actual video content, so we need to make sure we get the original value for the
pre-roll before it is changed.

[kzar, 2018/02/26 17:18:58]

Acknowledged.

+
   browser.runtime.sendMessage(
     {
       type: "filters.collapse",
       urls,
       mediatype,
       baseURL: document.location.href
     },
-
     collapse =>
     {
       if (collapse)
       {
-        hideElement(element);
+        if (collapsible)
+          collapseMediaElement(element, srcValue);
+        else
+          hideElement(element);
       }
     }
   );
 }
 
 function checkSitekey()
 {
   let attr = document.documentElement.getAttribute("data-adblockkey");
@@ -447,37 +492,37 @@
       let selector = preparedSelectors.slice(
         i, i + this.selectorGroupSize
       ).join(", ");
       style.sheet.insertRule(selector + "{display: none !important;}",
                              style.sheet.cssRules.length);
     }
   },
 
-  addSelectors(selectors, filters)
+  addSelectors(selectors, filters, groupName = "emulated")

[Manish Jethani, 2018/02/22 06:55:06]

Using a default value for groupName here (instead of setting it in the body or
repeating it).

[kzar, 2018/02/26 17:18:59]

Acknowledged.

   {
     if (this.inline || this.inlineEmulated)
     {
       // Insert the style rules inline if we have been instructed by the
       // background page to do so. This is usually the case, except on platforms
       // that do support user stylesheets via the browser.tabs.insertCSS API
       // (Firefox 53 onwards for now and possibly Chrome in the near future).
       // Once all supported platforms have implemented this API, we can remove
       // the code below. See issue #5090.
       // Related Chrome and Firefox issues:
       // https://bugs.chromium.org/p/chromium/issues/detail?id=632009
       // https://bugzilla.mozilla.org/show_bug.cgi?id=1310026
-      this.addSelectorsInline(selectors, "emulated");
+      this.addSelectorsInline(selectors, groupName);
     }
     else
     {
       browser.runtime.sendMessage({
         type: "elemhide.injectSelectors",
         selectors,
-        groupName: "emulated"
+        groupName
       });
     }
 
     if (this.tracer)
       this.tracer.addSelectors(selectors, filters);
   },
 
   hideElements(elements, filters)