Issue 4500 - Fix element hiding integration tests

chrome/content/tests/elemhide.js

Index: chrome/content/tests/elemhide.js
===================================================================
--- a/chrome/content/tests/elemhide.js
+++ b/chrome/content/tests/elemhide.js
@@ -113,74 +113,58 @@
     [["localhost##div#test1", "@@localhost$generichide"], ["hidden", "visible"]],
     [["~example.com##div#test1", "@@localhost$generichide"], ["visible", "visible"]],
     [["~example.com##div#test1", "@@localhost$genericblock"], ["hidden", "visible"]],
     [["~example.com,localhost##div#test1", "@@localhost$generichide"], ["hidden", "visible"]],
   ];
 
   function runTest([filters, expected], stage)
   {
-    let listener = function(action)
-    {
-      if (action != "elemhideupdate")
-        return;
-      FilterNotifier.removeListener(listener);
+    for (let filter_text of filters)

[kzar, 2016/10/07 09:42:08]

Nit: We usually use camelCase for variable names in JS code.

[Wladimir Palant, 2016/10/07 10:41:21]

Done.

+      FilterStorage.addFilter(Filter.fromText(filter_text));

[Wladimir Palant, 2016/10/07 08:35:50]

Rietveld doesn't show it because of whitespace changes but this is mostly merely
reordering - this block used to be at the end of the function and everything
that follows used to be within the listener. The real change is replacing
messing with ElemHide and defaultMatcher directly by FilterStorage.addFilter()
here and in the block below.

[kzar, 2016/10/07 09:42:08]

Sounds good to me, in fact I wonder why didn't do that to start with.

[Wladimir Palant, 2016/10/07 10:16:05]

Because element hiding was applying in an async fashion before #521 - applying
being a lengthy operation we didn't want to block everything.

[kzar, 2016/10/07 10:19:54]

Acknowledged.

 
-      if (stage == 2)
-        defaultMatcher.add(Filter.fromText("@@||localhost^$document"));
-      else if (stage == 3)
-        defaultMatcher.add(Filter.fromText("@@||localhost^$~document"));
-      else if (stage == 4)
-        defaultMatcher.add(Filter.fromText("@@||localhost^$elemhide"));
+    if (stage == 2)
+      FilterStorage.addFilter(Filter.fromText("@@||localhost^$document"));
+    else if (stage == 3)
+      FilterStorage.addFilter(Filter.fromText("@@||localhost^$~document"));
+    else if (stage == 4)
+      FilterStorage.addFilter(Filter.fromText("@@||localhost^$elemhide"));
 
-      if (stage == 2 || stage == 4)
-        expected = ["visible", "visible"];   // Second and forth runs are whitelisted, nothing should be hidden
+    if (stage == 2 || stage == 4)
+      expected = ["visible", "visible"];   // Second and forth runs are whitelisted, nothing should be hidden

[kzar, 2016/10/07 09:42:08]

Nit: Mind moving the comment above the if statement, on its own line?

[Wladimir Palant, 2016/10/07 10:16:05]

Done.

 
-      frame.addEventListener("abp:frameready", function()
+    frame.addEventListener("abp:frameready", function()
+    {
+      let frameScript = `
+        // The "load" event doesn't mean that our styles are applied - these
+        // are only applied after a message roundtrip to parent determining
+        // whether element hiding is enabled. Do the same roundtrip here before
+        // checking visibility to make sure timing is right.

[Wladimir Palant, 2016/10/07 08:35:50]

This comment changed - the reason why we need to play ping pong with the parent
is a different one now. The code is unchanged however.

[kzar, 2016/10/07 09:42:08]

Acknowledged.

+        addMessageListener("pong", function()
+        {
+          let visibility = [
+            content.document.getElementById("test1").offsetHeight > 0 ? "visible" : "hidden",
+            content.document.getElementById("test2").offsetHeight > 0 ? "visible" : "hidden"
+          ];
+          sendAsyncMessage("visibility", visibility);
+        });
+        sendAsyncMessage("ping");
+      `;
+      frame.messageManager.addMessageListener("ping", () => frame.messageManager.sendAsyncMessage("pong"));
+      frame.messageManager.addMessageListener("visibility", (message) =>
       {
-        let frameScript = `
-          // The "load" event doesn't mean XBL bindings are done, these will
-          // take longer to load (async messaging). Only check visibility after
-          // sending a message to parent and receiving response.
-          addMessageListener("pong", function()
-          {
-            let visibility = [
-              content.document.getElementById("test1").offsetHeight > 0 ? "visible" : "hidden",
-              content.document.getElementById("test2").offsetHeight > 0 ? "visible" : "hidden"
-            ];
-            sendAsyncMessage("visibility", visibility);
-          });
-          sendAsyncMessage("ping");
-        `;
-        frame.messageManager.addMessageListener("ping", () => frame.messageManager.sendAsyncMessage("pong"));
-        frame.messageManager.addMessageListener("visibility", (message) =>
-        {
-          let visibility = message.data;
-          equal(visibility[0], expected[0], "First element visible");
-          equal(visibility[1], expected[1], "Second element visible");
+        let visibility = message.data;
+        equal(visibility[0], expected[0], "First element visible");
+        equal(visibility[1], expected[1], "Second element visible");
 
-          start();
-        });
-        frame.messageManager.loadFrameScript("data:text/javascript," + encodeURIComponent(frameScript), false);
-      }, false, true);
-      frame.setAttribute("src", "http://localhost:1234/test");
-    };
-    FilterNotifier.addListener(listener);
-
-    for (let filter_text of filters)
-    {
-      let filter = Filter.fromText(filter_text);
-      if (filter instanceof WhitelistFilter)
-        defaultMatcher.add(filter);
-      else
-        ElemHide.add(filter);
-    }
-
-    ElemHide.isDirty = true;
-    ElemHide.apply();
+        start();
+      });
+      frame.messageManager.loadFrameScript("data:text/javascript," + encodeURIComponent(frameScript), false);
+    }, false, true);
+    frame.setAttribute("src", "http://localhost:1234/test");
   }
 
   let stageDescriptions = {
     1: "running without exceptions",
     2: "running with whitelisted document",
     3: "running with exception not applying to documents",
     4: "running with element hiding exception",
   };