1146566 - [e10s] browser_markupview_tag_edit_03.js causes unsafe CPOW usage warnings

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Ian Moody [:Kwan] (UTC+1)]

+++ This bug was initially created as a clone of Bug #1133577 +++

Mined from test logs

In browser/devtools/markupview/test/browser_markupview_tag_edit_03.js:

add_task(function*() {
  let {toolbox, inspector} = yield addTab(TEST_URL).then(openInspector);

  yield inspector.markup.expandAll();

  info("Selecting the test node");
  let node = content.document.querySelector("#retag-me"); <- Causes CPOW warning
  let child = content.document.querySelector("#retag-me-2"); <- Causes CPOW warning
  yield selectNode("#retag-me", inspector);

  let container = yield getContainerForSelector("#retag-me", inspector);
  is(node.tagName, "DIV", "We've got #retag-me element, it's a DIV"); <- Causes CPOW warning
  ok(container.expanded, "It is expanded");
  is(child.parentNode, node, "Child #retag-me-2 is inside #retag-me"); <- Causes CPOW warning

  info("Changing the tagname");
  let mutated = inspector.once("markupmutation");
  let tagEditor = container.editor.tag;
  setEditableFieldValue(tagEditor, "p", inspector);
  yield mutated;

  info("Checking that the tagname change was done");
  node = content.document.querySelector("#retag-me"); <- Causes CPOW warning
  container = yield getContainerForSelector("#retag-me", inspector);
  is(node.tagName, "P", "We've got #retag-me, it should now be a P"); <- Causes CPOW warning
  ok(container.expanded, "It is still expanded");
  ok(container.selected, "It is still selected");
  is(child.parentNode, node, "Child #retag-me-2 is still inside #retag-me"); <- Causes CPOW warning
});

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1146566/pbrosset

/r/5933 - Bug 1146566 - 1 - Use devtools common frame-script in markupview tests and add helper; r=bgrins
/r/5935 - Bug 1146566 - 2 - Avoid using CPOWs in test browser_markupview_tag_edit_03.js; r=bgrins

Pull down these commits:

hg pull review -r 550d6fdf0baa4557be024e42de1e9b7f03652b0b

Comment 2

[Patrick Brosset <:pbro>]

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

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8582322 [details]
MozReview Request: bz://1146566/pbrosset

https://reviewboard.mozilla.org/r/5931/#review4909

Looks fine.  Please see the comment regarding the return value when the queryselector doesn't match in getDomElementInfo

::: browser/devtools/markupview/test/head.js
(Diff revision 1)
> +function waitForContentMessage(name) {

I'd love if we could have a shared test-head-helpers module or something so we could implement things like this only once and use them in all of our tests.  Filed  1147012

::: browser/devtools/shared/frame-script-utils.js
(Diff revision 1)
> +  if (!node) {

If this is the case any test will just time out while waiting for the message.  Is this what we want?  If it instead sent a message with `null` and returned it may make test failures easier to debug / annotate and also allow us to pass in a selector that we know won't return anything and expect that.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8582322 [details]
> MozReview Request: bz://1146566/pbrosset
> 
> https://reviewboard.mozilla.org/r/5931/#review4909
> 
> Looks fine.  Please see the comment regarding the return value when the
> queryselector doesn't match in getDomElementInfo
> 
> ::: browser/devtools/markupview/test/head.js
> (Diff revision 1)
> > +function waitForContentMessage(name) {
> 
> I'd love if we could have a shared test-head-helpers module or something so
> we could implement things like this only once and use them in all of our
> tests.  Filed  1147012
Thanks, it's true that we have a good amount of duplication in our head.js files.
It will also be an opportunity to make our tests even more consistent.

> ::: browser/devtools/shared/frame-script-utils.js
> (Diff revision 1)
> > +  if (!node) {
> 
> If this is the case any test will just time out while waiting for the
> message.  Is this what we want?  If it instead sent a message with `null`
> and returned it may make test failures easier to debug / annotate and also
> allow us to pass in a selector that we know won't return anything and expect
> that.
Yeah, you're right, I'll get this changed.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8582322 [details]
MozReview Request: bz://1146566/pbrosset

https://reviewboard.mozilla.org/r/5931/#review4985

Ship It!

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8582322 [details]
MozReview Request: bz://1146566/pbrosset

/r/5933 - Bug 1146566 - 1 - Use devtools common frame-script in markupview tests and add helper; r=bgrins
/r/5935 - Bug 1146566 - 2 - Avoid using CPOWs in test browser_markupview_tag_edit_03.js; r=bgrins

Pull down these commits:

hg pull review -r 0b91f8d552c888287ae1fbb8606f4da722d6d82f

Comment 7

[Patrick Brosset <:pbro>]

remote:   https://hg.mozilla.org/integration/fx-team/rev/f97ddbbe1f8b
remote:   https://hg.mozilla.org/integration/fx-team/rev/0b91f8d552c8

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8582322 [details]
MozReview Request: bz://1146566/pbrosset

I don't know why this became R? ... so setting it back to R+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f97ddbbe1f8b
https://hg.mozilla.org/mozilla-central/rev/0b91f8d552c8

Comment 11

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1146566 - 1 - Use devtools common frame-script in markupview tests and add helper; r=bgrins

Comment 12

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1146566 - 2 - Avoid using CPOWs in test browser_markupview_tag_edit_03.js; r=bgrins