Closed Bug 1146566 Opened 5 years ago Closed 5 years ago

[e10s] browser_markupview_tag_edit_03.js causes unsafe CPOW usage warnings

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(e10sm8+, firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
e10s m8+ ---
firefox39 --- fixed

People

(Reporter: Kwan, Assigned: pbro)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

+++ 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
});
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
/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
Attachment #8582322 - Flags: review?(bgrinstead)
Blocks: 1146568
Blocks: 1146582
Blocks: 1146591
Attachment #8582322 - Flags: review?(bgrinstead) → review+
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.
(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 on attachment 8582322 [details]
MozReview Request: bz://1146566/pbrosset

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

Ship It!
Attachment #8582322 - Flags: review+
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
Attachment #8582322 - Flags: review?(bgrinstead)
Attachment #8582322 - Flags: review+
Comment on attachment 8582322 [details]
MozReview Request: bz://1146566/pbrosset

I don't know why this became R? ... so setting it back to R+
Attachment #8582322 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/f97ddbbe1f8b
https://hg.mozilla.org/mozilla-central/rev/0b91f8d552c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Attachment #8582322 - Attachment is obsolete: true
Attachment #8619851 - Flags: review+
Attachment #8619852 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.