603610 - JavaScript Error: "notificationBox is null" {file: "chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_588342_document_focus.js" line: 57}

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Rob Campbell [:rc] (:robcee)]

Seeing a lot of these interspersed with other tests after landing of bug 581069.

Comment 1

[Mihai Sucan [:msucan]]

Taking the bug. It's a test I wrote.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Proposed fix. Now it gets the chance to actually check if the document has focus after closing the Web Console. (3 tests pass, instead of only 2) Also, the notificationBox variable being null error is fixed.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 482812 [details] [diff] [review]
proposed fix

Thanks David for the feedback+! Asking for review now.

Comment 5

[David Dahl :ddahl]

Comment on attachment 482812 [details] [diff] [review]
proposed fix

This is a test-only change and does not require toolkit peer review.

Comment 6

[Dão Gottwald [:dao]]

landed and backed out:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_588342_document_focus.js | content document has focus - Got [object XrayWrapper [object Window]], expected [object Window]

Comment 7

[Mihai Sucan [:msucan]]

Oops, sorry! That must be a problem with the new XrayWrappers that I did not expect. I have tested the patch a few days ago. Tomorrow I'll rebase and fix the patch.

Comment 8

[David Dahl :ddahl]

I think we have a lot of issues that we need to iron out with this new wrapper type. arrggh.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to comment #8)
> I think we have a lot of issues that we need to iron out with this new wrapper
> type. arrggh.

Hehe, no worries. I'll do hg pull from m-c tomorrow and rebase all test fixes I have waiting to be checked-in, ironing out the issues with this new wrapper type.

Comment 10

[Dão Gottwald [:dao]]

I disabled the test: http://hg.mozilla.org/mozilla-central/rev/249a1313aa52

Comment 11

[Rob Campbell [:rc] (:robcee)]

disabled the test but didn't reland? This fix should be safe to checkin now, with that test disabled, right?

We should add that test to the list of bugs to fix and land in bug 604536. Adding there.

Comment 12

[Dão Gottwald [:dao]]

I didn't reland the patch, no. I don't know what exactly is left to do here or in bug 604536 -- my main interest was to stop the mochitest-browser-chrome log pollution.

Comment 13

[Rob Campbell [:rc] (:robcee)]

yep fair enough. I'll reland with some other fixes this morning.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch (fix for compartments landing)

Rebased patch and a one-line change to make the test work fine after the compartments landing.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 484002 [details] [diff] [review]
[checked-in] rebased patch (fix for compartments landing)

http://hg.mozilla.org/mozilla-central/rev/ce9dc1970352