Issue 4500 - Fix element hiding integration tests

chrome/content/tests/elemhide.js

 (function()
 {
   let server = null;
   let frame = null;
 
   module("Element hiding", {
     setup: function()
     {
       prepareFilterComponents.call(this);
       preparePrefs.call(this);
@@ skipping 100 matching lines @@
     [["##div#test1", "@@localhost$generichide"], ["visible", "visible"]],
     [["##div#test1", "@@localhost$genericblock"], ["hidden", "visible"]],
     [["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)
   {
-    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.

+    for (let filterText of filters)
+      FilterStorage.addFilter(Filter.fromText(filterText));
 
     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"));
 
+    // 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.

+      expected = ["visible", "visible"];
 
     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");
       `;
@@ skipping 18 matching lines @@
     4: "running with element hiding exception",
   };
 
   for (let test = 0; test < tests.length; test++)
   {
     let [filters, expected] = tests[test];
     for (let stage = 1; stage in stageDescriptions; stage++)
       asyncTest(filters.join(", ") + " (" + stageDescriptions[stage] + ")", runTest.bind(null, tests[test], stage));
   }
 })();