Issue 29356283: Issue 4500 - Fix element hiding integration tests (Closed)
|
Created:
Oct. 7, 2016, 8:29 a.m. by Wladimir Palant Modified:
Oct. 7, 2016, 10:53 a.m. Reviewers:
kzar Base URL:
https://hg.adblockplus.org/adblockplustests Visibility:
Public. |
DescriptionWith these changes only six tests are failing. These six are failing reliably however which indicates an actual issue, I am looking into it.
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed nit #Patch Set 3 : Addressed one more nit #MessagesTotal messages: 7
For clarity, here is what `hg diff -b` shows:
diff --git a/chrome/content/tests/elemhide.js b/chrome/content/tests/elemhide.js
--- a/chrome/content/tests/elemhide.js
+++ b/chrome/content/tests/elemhide.js
@@ -113,38 +113,36 @@
[["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)
+ FilterStorage.addFilter(Filter.fromText(filter_text));
if (stage == 2)
- defaultMatcher.add(Filter.fromText("@@||localhost^$document"));
+ FilterStorage.addFilter(Filter.fromText("@@||localhost^$document"));
else if (stage == 3)
- defaultMatcher.add(Filter.fromText("@@||localhost^$~document"));
+ FilterStorage.addFilter(Filter.fromText("@@||localhost^$~document"));
else if (stage == 4)
- defaultMatcher.add(Filter.fromText("@@||localhost^$elemhide"));
+ FilterStorage.addFilter(Filter.fromText("@@||localhost^$elemhide"));
if (stage == 2 || stage == 4)
expected = ["visible", "visible"]; // Second and forth runs are
whitelisted, nothing should be hidden
frame.addEventListener("abp:frameready", function()
{
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.
+ // 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.
addMessageListener("pong", function()
{
let visibility = [
content.document.getElementById("test1").offsetHeight > 0 ?
"visible" : "hidden",
content.document.getElementById("test2").offsetHeight > 0 ?
"visible" : "hidden"
];
sendAsyncMessage("visibility", visibility);
});
@@ -157,30 +155,16 @@
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();
}
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",
};
https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test...
File chrome/content/tests/elemhide.js (right):
https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test...
chrome/content/tests/elemhide.js:122:
FilterStorage.addFilter(Filter.fromText(filter_text));
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.
https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test...
chrome/content/tests/elemhide.js:140: // checking visibility to make sure timing
is right.
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.
https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:121: for (let filter_text of filters) Nit: We usually use camelCase for variable names in JS code. https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:122: FilterStorage.addFilter(Filter.fromText(filter_text)); On 2016/10/07 08:35:50, Wladimir Palant wrote: > 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. Sounds good to me, in fact I wonder why didn't do that to start with. https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:132: expected = ["visible", "visible"]; // Second and forth runs are whitelisted, nothing should be hidden Nit: Mind moving the comment above the if statement, on its own line? https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:140: // checking visibility to make sure timing is right. On 2016/10/07 08:35:50, Wladimir Palant wrote: > 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. Acknowledged.
https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:122: FilterStorage.addFilter(Filter.fromText(filter_text)); On 2016/10/07 09:42:08, kzar wrote: > Sounds good to me, in fact I wonder why didn't do that to start with. Because element hiding was applying in an async fashion before #521 - applying being a lengthy operation we didn't want to block everything. https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:132: expected = ["visible", "visible"]; // Second and forth runs are whitelisted, nothing should be hidden On 2016/10/07 09:42:08, kzar wrote: > Nit: Mind moving the comment above the if statement, on its own line? Done.
LGTM https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:122: FilterStorage.addFilter(Filter.fromText(filter_text)); On 2016/10/07 10:16:05, Wladimir Palant wrote: > On 2016/10/07 09:42:08, kzar wrote: > > Sounds good to me, in fact I wonder why didn't do that to start with. > > Because element hiding was applying in an async fashion before #521 - applying > being a lengthy operation we didn't want to block everything. Acknowledged.
I overlooked one more nit, addressed this one as well now. https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/29356283/diff/29356284/chrome/content/test... chrome/content/tests/elemhide.js:121: for (let filter_text of filters) On 2016/10/07 09:42:08, kzar wrote: > Nit: We usually use camelCase for variable names in JS code. Done.
LGTM++ 
|
