Issue 3244 - Pop-up blocker unit tests broken in E10S mode

Issue 3244 - Pop-up blocker unit tests broken in E10S mode

[Erik, 2015-12-04 20:58:34]

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
File chrome/content/tests/popupBlocker.js (right):

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:4: let modelFor =
SDK.require("sdk/model/core").modelFor;
oh I should remove this.

[Wladimir Palant, 2015-12-14 11:28:53]

Sorry, it took me rather long to wrap my head around this. Would be a lot
simpler if this kept relying on compatibility shims rather than switching to SDK
for proper E10S compatibility.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/comm...
File chrome/content/common.js (right):

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/comm...
chrome/content/common.js:14: const SDK =
Cu.import("resource://gre/modules/commonjs/toolkit/require.js", {});
I added dev-doc-needed keyword to
https://bugzilla.mozilla.org/show_bug.cgi?id=1070927 - this functionality isn't
currently documented. So I would have a problem with using it in production,
good enough for tests however.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/comm...
chrome/content/common.js:15: SDK.require("sdk/tabs");
This line seems unnecessary?

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
File chrome/content/tests/popupBlocker.js (right):

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:58: });
I don't think that clicking the link should be part of the setup. How about
calling start() when the tab is ready and having the test click the link?

**Edit**: It seems that you click the link there as well...

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:63: })
Nit: No need to wrap the callback here, this will do:

  worker.port.once("done", start);

Also, you are missing a semicolon.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:88: // expect no opened tab
Frankly, I think that this is overdocumenting. I would be ok with the general
comment explaining what we are storing in that tests array, but documenting each
line seems wrong - and it makes adding/modifying tests more complicated
unnecessarily.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:89: //["||127.0.0.1:1234/target$popup",
false],
Why is this test commented out, isn't it working correctly?

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:112: var testCount = 0;
This variable seems unused.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:116: tabs.off("ready", onTabOpen);
This seems pointless, why would there be an existing listener at this point?
Note that onTabOpen is a different function for each invocation of runTest.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:121: function onTabOpen(tab) {
Style nit: Bracket on the next line please.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:128: contentScript: "self.port.emit('done',
document.body.innerHTML.toString());"
No point calling toString() here - innerHTML is always a string. Also, if you
want text then you should send textContent rather than innerHTML.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:152: keys.push(key);
I have no idea why this code is here, it isn't being used. Could you remove it?

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:163: }, 1000);
Nit: no point wrapping the callback here either. This should do:

  let timeout = window.setTimeout(onTabClose, 1000, null);

Also, we don't use var in new code (unless it's top-level code where we need the
variable to be global).

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:170: },
The content script doesn't need any options...

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:175: //contentScript:
"document.getElementById('link').click();"
Please remove this line.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:179: // create async qunit tests
This is stating the obvious again.

[Erik, 2015-12-29 22:49:41]

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
File chrome/content/tests/popupBlocker.js (right):

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:88: // expect no opened tab
On 2015/12/14 11:28:52, Wladimir Palant wrote:
> Frankly, I think that this is overdocumenting. I would be ok with the general
> comment explaining what we are storing in that tests array, but documenting
each
> line seems wrong - and it makes adding/modifying tests more complicated
> unnecessarily.

We can leave this for another issue, I'll take it out for this one.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:89: //["||127.0.0.1:1234/target$popup",
false],
On 2015/12/14 11:28:53, Wladimir Palant wrote:
> Why is this test commented out, isn't it working correctly?

oh I didn't notice that I did that, iirc I commented it out because it wasn't
working, and I must've forgotten to uncomment it.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:116: tabs.off("ready", onTabOpen);
On 2015/12/14 11:28:53, Wladimir Palant wrote:
> This seems pointless, why would there be an existing listener at this point?
> Note that onTabOpen is a different function for each invocation of runTest.

This should be in `onTabOpen`.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:128: contentScript: "self.port.emit('done',
document.body.innerHTML.toString());"
On 2015/12/14 11:28:52, Wladimir Palant wrote:
> No point calling toString() here - innerHTML is always a string. Also, if you
> want text then you should send textContent rather than innerHTML.

good point.

https://codereview.adblockplus.org/29331996/diff/29331997/chrome/content/test...
chrome/content/tests/popupBlocker.js:152: keys.push(key);
On 2015/12/14 11:28:52, Wladimir Palant wrote:
> I have no idea why this code is here, it isn't being used. Could you remove
it?

alright it doesn't seem to be used to me either.

[Wladimir Palant, 2016-01-06 12:54:37]

https://codereview.adblockplus.org/29331996/diff/29333120/chrome/content/test...
File chrome/content/tests/popupBlocker.js (right):

https://codereview.adblockplus.org/29331996/diff/29333120/chrome/content/test...
chrome/content/tests/popupBlocker.js:4: let modelFor =
SDK.require("sdk/model/core").modelFor;
So, do you want to remove this? ;)

https://codereview.adblockplus.org/29331996/diff/29333120/chrome/content/test...
chrome/content/tests/popupBlocker.js:58: });
As mentioned before, I don't think that clicking the link should be part of the
setup - nor does this code appear necessary as you are clicking the link at the
bottom of runTest().

https://codereview.adblockplus.org/29331996/diff/29333120/chrome/content/test...
chrome/content/tests/popupBlocker.js:60: worker.port.once("done", start)
Nit: Missing semicolon here.

[Erik, 2016-01-22 17:29:41]

Hey Wladimir, when you get a chance can you take another look at this?

[Wladimir Palant, 2016-01-27 16:19:33]

LGTM

[Erik, 2016-02-04 23:30:38]

On 2016/01/27 16:19:33, Wladimir Palant wrote:
> LGTM

hey I had to make one little change, check out the delta #3, it still looks good
right?

[Wladimir Palant, 2016-02-05 13:03:15]

LGTM