1330822 - Remove more CPOWs from tests

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I have a bunch of patches to remove CPOWs from tests. A lot of them are pretty simple, except for a few which have longer explanations in their commit messages.

The one to look at the closest, I believe, is the one to fix browser_context_menu_iframe.js as I had to fix the synthesizeMouse implementation to take iframes into account.

Also of note was the fix for browser_bug839103.js where the first test (`initialStylesheetAdded`) simply wasn't testing what it claimed to and instead was firing for the first dynamic stylesheet, duplicating the test for that stylesheet. It isn't easily possible to listen for the initial stylesheet being added in a browser-chrome test and I didn't feel that it was worth the effort to add it here.

Patches coming up.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_usercontext.js.

Review commit: https://reviewboard.mozilla.org/r/104340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104340/

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from customizableui/head.js.

Review commit: https://reviewboard.mozilla.org/r/104342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104342/

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_context_menu_iframe.js.

This was tricky because synthesizeMouse would compute the incorrect
coordinates if the requested event target was in a sub-frame. With this patch,
we deal correctly with sub-frames.

Review commit: https://reviewboard.mozilla.org/r/104344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104344/

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_bug839103.js.

In converting this test, I found that `initialStylesheetAdded` was testing the
wrong thing. It was firing instead for the dynamic case. Fixing it is hard
without converting the test to be a mochitest-plain test. I simply removed it.
If we want to add it back, we can at another date.

Review commit: https://reviewboard.mozilla.org/r/104346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104346/

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_no_mcb_on_http_site.js.

Review commit: https://reviewboard.mozilla.org/r/104348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104348/

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_favicon_change.js.

Review commit: https://reviewboard.mozilla.org/r/104350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104350/

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_identity_UI.js.

Review commit: https://reviewboard.mozilla.org/r/104352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104352/

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_mixedcontent_securityflags.js.

Review commit: https://reviewboard.mozilla.org/r/104354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104354/

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_popupUI.js.

Finding the popup window after the fact was going to be messy (maintaining
window identity across the message manager is a tricky task). Instead, it's
much easier to wait for the new window to open and grab a reference to it
then.

Review commit: https://reviewboard.mozilla.org/r/104356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104356/

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_loadURI.js.

Review commit: https://reviewboard.mozilla.org/r/104358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104358/

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1330822 - Remove CPOWs from browser_bug633691.js

Review commit: https://reviewboard.mozilla.org/r/104360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104360/

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I've asked a ton of reviews from Felipe here. Others are very welcome to steal them and/or give comments. Hopefully these patches should be fast/easy to review (I'm also always looking for more people to spread the review load to, I don't really know who the best targets are, though).

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=558669879f910e1ba0d1113eb93b0366c336b0b3

Comment 14

[:Gijs (he/him)]

Comment on attachment 8826397 [details]
Bug 1330822 - Remove CPOWs from browser_usercontext.js.

https://reviewboard.mozilla.org/r/104340/#review105228

Comment 15

[:Gijs (he/him)]

Comment on attachment 8826398 [details]
Bug 1330822 - Remove CPOWs from customizableui/head.js.

https://reviewboard.mozilla.org/r/104342/#review105234

Comment 16

[:Gijs (he/him)]

Comment on attachment 8826399 [details]
Bug 1330822 - Remove CPOWs from browser_context_menu_iframe.js.

https://reviewboard.mozilla.org/r/104344/#review105238

::: testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js:55
(Diff revision 1)
> +        cur = frame;
> +      } while (cur && cur.ownerDocument !== content.document);
> +
> +      // If node wasn't in a document that was parented to us somehow, bail.
> +      if (!cur) {
> +        throw new Error("unrelated node found");

Can we Cu.import() Assert.jsm and ensure this case fails tests?

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8826402 [details]
Bug 1330822 - Remove CPOWs from browser_favicon_change.js.

https://reviewboard.mozilla.org/r/104350/#review105252

Comment 18

[:Gijs (he/him)]

Comment on attachment 8826400 [details]
Bug 1330822 - Remove CPOWs from browser_bug839103.js.

https://reviewboard.mozilla.org/r/104346/#review105248

Not sure why this isn't a plain test, and this should *really* not be in browser/base/content/test/general, but hey...

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8826403 [details]
Bug 1330822 - Remove CPOWs from browser_identity_UI.js.

https://reviewboard.mozilla.org/r/104352/#review105254

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8826406 [details]
Bug 1330822 - Remove CPOWs from browser_loadURI.js.

https://reviewboard.mozilla.org/r/104358/#review105260

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8826404 [details]
Bug 1330822 - Remove CPOWs from browser_mixedcontent_securityflags.js.

https://reviewboard.mozilla.org/r/104354/#review105262

Comment 22

[:Gijs (he/him)]

Comment on attachment 8826407 [details]
Bug 1330822 - Remove CPOWs from browser_bug633691.js

https://reviewboard.mozilla.org/r/104360/#review105256

::: browser/base/content/test/general/browser_bug633691.js:7
(Diff revision 1)
>   * http://creativecommons.org/publicdomain/zero/1.0/
>   */
>  
> -function test() {
> -  waitForExplicitFinish();
> -  gBrowser.selectedTab = gBrowser.addTab("data:text/html,<iframe width='700' height='700'></iframe>");
> +add_task(function* test() {
> +  const URL = "data:text/html,<iframe width='700' height='700'></iframe>";
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: URL }, function* (browser) {

Nit: can just pass URL instead of the object.

::: browser/base/content/test/general/browser_bug633691.js:19
(Diff revision 1)
> -  });
> -}
>  
> -function testIframeCert(e) {
> -  // Confirm that the expert section is hidden
> -  var doc = gBrowser.contentDocument.getElementsByTagName('iframe')[0].contentDocument;
> +      yield loadError;
> +
> +      let is_hidden = eval(`(() => ${is_hidden_})()`);

eslint points out this is unused, which means you can also not pass it in from the parent.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8826405 [details]
Bug 1330822 - Remove CPOWs from browser_popupUI.js.

https://reviewboard.mozilla.org/r/104356/#review105264

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8826401 [details]
Bug 1330822 - Remove CPOWs from browser_no_mcb_on_http_site.js.

https://reviewboard.mozilla.org/r/104348/#review105268

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826407 [details]
Bug 1330822 - Remove CPOWs from browser_bug633691.js

https://reviewboard.mozilla.org/r/104360/#review105256

> eslint points out this is unused, which means you can also not pass it in from the parent.

`is_element_hidden` actually calls `is_hidden` in the body of the function. eslint can't see that through the `eval` so it complains incorrctly (I've fixed the eslint error by adding `void is_hidden;` to the function).

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826397 [details]
Bug 1330822 - Remove CPOWs from browser_usercontext.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104340/diff/1-2/

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826398 [details]
Bug 1330822 - Remove CPOWs from customizableui/head.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104342/diff/1-2/

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826399 [details]
Bug 1330822 - Remove CPOWs from browser_context_menu_iframe.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104344/diff/1-2/

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826400 [details]
Bug 1330822 - Remove CPOWs from browser_bug839103.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104346/diff/1-2/

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826401 [details]
Bug 1330822 - Remove CPOWs from browser_no_mcb_on_http_site.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104348/diff/1-2/

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826402 [details]
Bug 1330822 - Remove CPOWs from browser_favicon_change.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104350/diff/1-2/

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826403 [details]
Bug 1330822 - Remove CPOWs from browser_identity_UI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104352/diff/1-2/

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826404 [details]
Bug 1330822 - Remove CPOWs from browser_mixedcontent_securityflags.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104354/diff/1-2/

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826405 [details]
Bug 1330822 - Remove CPOWs from browser_popupUI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104356/diff/1-2/

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826406 [details]
Bug 1330822 - Remove CPOWs from browser_loadURI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104358/diff/1-2/

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826407 [details]
Bug 1330822 - Remove CPOWs from browser_bug633691.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104360/diff/1-2/

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826397 [details]
Bug 1330822 - Remove CPOWs from browser_usercontext.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104340/diff/2-3/

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826398 [details]
Bug 1330822 - Remove CPOWs from customizableui/head.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104342/diff/2-3/

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826399 [details]
Bug 1330822 - Remove CPOWs from browser_context_menu_iframe.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104344/diff/2-3/

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826400 [details]
Bug 1330822 - Remove CPOWs from browser_bug839103.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104346/diff/2-3/

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826401 [details]
Bug 1330822 - Remove CPOWs from browser_no_mcb_on_http_site.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104348/diff/2-3/

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826402 [details]
Bug 1330822 - Remove CPOWs from browser_favicon_change.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104350/diff/2-3/

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826403 [details]
Bug 1330822 - Remove CPOWs from browser_identity_UI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104352/diff/2-3/

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826404 [details]
Bug 1330822 - Remove CPOWs from browser_mixedcontent_securityflags.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104354/diff/2-3/

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826405 [details]
Bug 1330822 - Remove CPOWs from browser_popupUI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104356/diff/2-3/

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826406 [details]
Bug 1330822 - Remove CPOWs from browser_loadURI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104358/diff/2-3/

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826407 [details]
Bug 1330822 - Remove CPOWs from browser_bug633691.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104360/diff/2-3/

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8826399 [details]
Bug 1330822 - Remove CPOWs from browser_context_menu_iframe.js.

https://reviewboard.mozilla.org/r/104344/#review105238

> Can we Cu.import() Assert.jsm and ensure this case fails tests?

Assert.jsm was too generic for this. Instead, I send a message back to the parent so we at least get an uncaught exception there. I don't know if it'll turn tests orange, but I also don't believe it's possible to hit this currently.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46b7136b8790
Remove CPOWs from browser_usercontext.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/79079769f4c9
Remove CPOWs from customizableui/head.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/21fd0eb4fa62
Remove CPOWs from browser_context_menu_iframe.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2e3c8dc20f2c
Remove CPOWs from browser_bug839103.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2b01eae261ba
Remove CPOWs from browser_no_mcb_on_http_site.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/32713b1df8ae
Remove CPOWs from browser_favicon_change.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/538aec2ed89e
Remove CPOWs from browser_identity_UI.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/a5cc4b891022
Remove CPOWs from browser_mixedcontent_securityflags.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/a66860c81eaf
Remove CPOWs from browser_popupUI.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/ef6ee3788c6b
Remove CPOWs from browser_loadURI.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/e4a95dce8bb8
Remove CPOWs from browser_bug633691.js r=Gijs

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46b7136b8790
https://hg.mozilla.org/mozilla-central/rev/79079769f4c9
https://hg.mozilla.org/mozilla-central/rev/21fd0eb4fa62
https://hg.mozilla.org/mozilla-central/rev/2e3c8dc20f2c
https://hg.mozilla.org/mozilla-central/rev/2b01eae261ba
https://hg.mozilla.org/mozilla-central/rev/32713b1df8ae
https://hg.mozilla.org/mozilla-central/rev/538aec2ed89e
https://hg.mozilla.org/mozilla-central/rev/a5cc4b891022
https://hg.mozilla.org/mozilla-central/rev/a66860c81eaf
https://hg.mozilla.org/mozilla-central/rev/ef6ee3788c6b
https://hg.mozilla.org/mozilla-central/rev/e4a95dce8bb8

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eebc55e20cc9
https://hg.mozilla.org/releases/mozilla-beta/rev/9d8bd6eaf525
https://hg.mozilla.org/releases/mozilla-beta/rev/2c9226752040
https://hg.mozilla.org/releases/mozilla-beta/rev/bace9af707bf
https://hg.mozilla.org/releases/mozilla-beta/rev/56ce2dca89a6
https://hg.mozilla.org/releases/mozilla-beta/rev/d006ddf8eaa3
https://hg.mozilla.org/releases/mozilla-beta/rev/47e7220e1a9e
https://hg.mozilla.org/releases/mozilla-beta/rev/a4dd0e9734d2
https://hg.mozilla.org/releases/mozilla-beta/rev/711fc611dbd8
https://hg.mozilla.org/releases/mozilla-beta/rev/848bf93b1a96
https://hg.mozilla.org/releases/mozilla-beta/rev/48718f7f527f