Remove more CPOWs from tests

RESOLVED FIXED in Firefox 52

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
Firefox 53
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments)

59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Felipe
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
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).

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Updated

2 years ago
Attachment #8826397 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8826397 [details]
Bug 1330822 - Remove CPOWs from browser_usercontext.js.

https://reviewboard.mozilla.org/r/104340/#review105228
Attachment #8826397 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

2 years ago
Attachment #8826398 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8826398 [details]
Bug 1330822 - Remove CPOWs from customizableui/head.js.

https://reviewboard.mozilla.org/r/104342/#review105234
Attachment #8826398 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

2 years ago
Attachment #8826399 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)

Comment 16

2 years ago
mozreview-review
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?
Attachment #8826399 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

2 years ago
Attachment #8826400 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8826402 [details]
Bug 1330822 - Remove CPOWs from browser_favicon_change.js.

https://reviewboard.mozilla.org/r/104350/#review105252
Attachment #8826402 - Flags: review?(felipc) → review+

Comment 18

2 years ago
mozreview-review
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...
Attachment #8826400 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8826403 [details]
Bug 1330822 - Remove CPOWs from browser_identity_UI.js.

https://reviewboard.mozilla.org/r/104352/#review105254
Attachment #8826403 - Flags: review?(felipc) → review+

Updated

2 years ago
Attachment #8826407 - Flags: review?(felipc) → review?(gijskruitbosch+bugs)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8826406 [details]
Bug 1330822 - Remove CPOWs from browser_loadURI.js.

https://reviewboard.mozilla.org/r/104358/#review105260
Attachment #8826406 - Flags: review?(felipc) → review+

Comment 21

2 years ago
mozreview-review
Comment on attachment 8826404 [details]
Bug 1330822 - Remove CPOWs from browser_mixedcontent_securityflags.js.

https://reviewboard.mozilla.org/r/104354/#review105262
Attachment #8826404 - Flags: review?(felipc) → review+

Comment 22

2 years ago
mozreview-review
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.
Attachment #8826407 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8826405 [details]
Bug 1330822 - Remove CPOWs from browser_popupUI.js.

https://reviewboard.mozilla.org/r/104356/#review105264
Attachment #8826405 - Flags: review?(felipc) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8826401 [details]
Bug 1330822 - Remove CPOWs from browser_no_mcb_on_http_site.js.

https://reviewboard.mozilla.org/r/104348/#review105268
Attachment #8826401 - Flags: review?(felipc) → review+
(Assignee)

Comment 25

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 48

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by mrbkap@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.