587734 - Integrate lazy window console API into WebConsole code

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

The lazily-inited consoleApi needs to be integrated into the webconsole code. This should be pretty easy:

1. Inside WindowInitializer we can assume we always have a console object instead of checking for its existence.

2. The constructor for HeadsUpDisplay will change as we never need to "reattach a console"

3. HUD.reattachConsole will probably be changed so that is makes sure there is a reference to the newly created console in the HeadsUpDisplay - MAY NOT be needed actually.

4. HUD.createConsole will still be used, however the HUDConsole API methods will change into a single observer that calls sendToHUDService()

This is just off the cuff, so I am sure it will change a bit as I work on it.

Comment 1

[David Dahl :ddahl]

5. Also, on first creation of a WebConsole we need to get all stored messages for the window from the ConsoleStorageService

Comment 2

[Mihai Sucan [:msucan]]

(This is tangential to this bug. Buut, I am mentioning this issue because I see you are going to do some important changes related to the windowInitializer and reattachConsole methods. I hope you don't mind!)

With regards to the HeadsUpDisplay object and to reattachConsole: ideally we shouldn't have to create a new HeadsUpDisplay every time the user navigates the page, or for every iframe, etc, etc. We should just reference back to the HUD object.

If I am not mistaken, the reattachConsole method will still be needed inside the HUD object, because we'll need to handle the case when the content window object changes - that's always the case when windowInitializer is invoked (it's a new window object created).

To reliably reference back to the HUD object, when the windowInitializer is created, we need to keep HUD objects in a HUDService object - by id. This is already done now, but we use weakrefs, which causes a bug: HUD objects are removed by the GC.

When you work on the updated windowInitializer and reattachConsole, please also peek into attachment 466279 [details] [diff] [review] (from bug 578658). Thanks!

Comment 3

[David Dahl :ddahl]

(In reply to comment #2)
> With regards to the HeadsUpDisplay object and to reattachConsole: ideally we
> shouldn't have to create a new HeadsUpDisplay every time the user navigates the
> page, or for every iframe, etc, etc. We should just reference back to the HUD
> object.

indeed. We do not re-create the HeadsUpDisplay now nor will we do that in the future. 

> 
> If I am not mistaken, the reattachConsole method will still be needed inside
> the HUD object, because we'll need to handle the case when the content window
> object changes - that's always the case when windowInitializer is invoked (it's
> a new window object created).

Correct

> 
> To reliably reference back to the HUD object, when the windowInitializer is
> created, we need to keep HUD objects in a HUDService object - by id. This is
> already done now, but we use weakrefs, which causes a bug: HUD objects are
> removed by the GC.
> 
> When you work on the updated windowInitializer and reattachConsole, please also
> peek into attachment 466279 [details] [diff] [review] (from bug 578658). Thanks!

ok, will do.

perhaps we can try to make these bits a followup to this bug.

I want to very gingerly patch the WebConsole for this. We should try not to rock the boat here:)

Comment 4

[Kevin Dangoor]

Requesting blocking status for this bug, as this is fundamental to how the console should operate once the platform support is in place.

Comment 5

[David Dahl :ddahl]

More notes for me:

* need to make sure the window ID of the outer window is added as a property to the HUD object.

* need to add a method to the HUDService that converts OuterWindow ID to window object and back.

Comment 6

[Julian Viereck]

Note: In bug 587573 (Log image requests to the WebConsole) I add a reference between HUDs and contentWindows. This might be a good foundation to build onto. If that's simple to get the window ID from an window object, I wouldn't mind if we store window IDs where I store weak references to the content window in that patch.

Comment 7

[David Dahl :ddahl]

Attachment: v 0.1 patch - saving work for now. NOT TESTED

Comment 8

[Benjamin Smedberg]

kdangoor, what platform pieces does this depend on, and are they going to make beta5? This sounds like new API work which needs to hit feature-freeze, but the schedule/dependencies are unclear to me.

Comment 9

[David Dahl :ddahl]

(In reply to comment #8)
> kdangoor, what platform pieces does this depend on, and are they going to make
> beta5? This sounds like new API work which needs to hit feature-freeze, but the
> schedule/dependencies are unclear to me.

This depends on the new interface in attachment https://bug568629.bugzilla.mozilla.org/attachment.cgi?id=463676  in bug 568629

Comment 10

[Dietrich Ayala (:dietrich)]

This is a necessary sibling patch to bug 568629, blocking+.

Comment 11

[David Dahl :ddahl]

Attachment: 0.2 HUDConsolectomy

still broken, less so.

Comment 12

[Kevin Dangoor]

As I note in this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=568629#c66 this does not truly block beta 5, but should be a betaN+ blocker (and we hope to land this early in b6)

Comment 13

[David Dahl :ddahl]

Attachment: 0.3 HUDConsoleectomy

...getting closer... tracking down some issues inside of CAO_observe(). saving work.

Comment 14

[David Dahl :ddahl]

Attachment: 0.4 ConsoleEctomy

So this is working now, however, we have some failing tests:

mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | log logging turned off, 1 message hidden - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Emitted both console arguments
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | info logging turned off, 1 message hidden - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Emitted both console arguments
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | warn logging turned off, 1 message hidden - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Emitted both console arguments
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | error logging turned off, 1 message hidden - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Emitted both console arguments
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Test timed out
make: *** [mochitest-browser-chrome] Error 1

Comment 15

[David Dahl :ddahl]

<ddahl> jst: it seems like I cannot ever get more than 1 argument in the arguments array when calling my new console.log component, which seems weird
* jgriffin has quit (Client exited)
* jgriffin ([e-mail]) has joined #developers
<ddahl> jst: the array exists but it's length is always 1
* jgriffin has quit (Quit: jgriffin)
<ddahl> jst: does that have to do with the IDL constraints?
* neurolysis ([e-mail]) has joined #developers
* neurolysis has quit (Connection reset by peer)
<bsmedberg> ddahl: is it implemented in IDL/
<bsmedberg> ?
* jgriffin ([e-mail]) has joined #developers
<ddahl> bsmedberg: yeah
<bsmedberg> then yes, the IDL determines the argument count
* laura is now known as laura_afk
* kdcw has quit (Ping timeout)
<bsmedberg> ddahl: why is it implemented in IDL/
<ddahl> bsmedberg: ok. i guess our whole approach is flawed then:(
* sayrer has quit (Quit: sayrer)
* Yoric has quit (Quit: Yoric)
<bsmedberg> stop implementing it in IDL and the problem goes away, I bet
* syp ([e-mail]) has joined #developers
<ddahl> bsmedberg: becasue jst has a patch that lazily creates a console property on any window when called
<bsmedberg> use a message-manager script like we're using for InstallTrigger
<ddahl> hmm
<bsmedberg> with a getter, if necessary
<ted> does that work in-process?
<ddahl> bsmedberg: ok, will look into that
* mwu ([e-mail]) has joined #developers
<bsmedberg> ted: yes, we're using it for Firefox
<ted> ah

Comment 16

[David Dahl :ddahl]

So the problem mentioned above is covered in https://bugzilla.mozilla.org/show_bug.cgi?id=568629#c69

Comment 17

[David Dahl :ddahl]

(In reply to comment #16)
> So the problem mentioned above is covered in
> https://bugzilla.mozilla.org/show_bug.cgi?id=568629#c69

In the meantime I am using an IDL hack to make it work - for now.

Comment 18

[David Dahl :ddahl]

Attachment: Saving Work. Need to split this up

Comment 19

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 20

[David Dahl :ddahl]

Attachment: 0.5 unbitrotted, still needs splitting into integration and further lazy console patch

Saving work, not tested.

Comment 21

[David Dahl :ddahl]

Attachment: v 0.5.1 ALL TESTS PASS, however...

Still have some problems identifying consoles via the window.ID inside of the console observer.

Comment 22

[David Dahl :ddahl]

Attachment: v 0.6 Borked

Comment 23

[David Dahl :ddahl]

Comment on attachment 475366 [details] [diff] [review]
v 0.5.1 ALL TESTS PASS, however...

Today was a waste of time on this. The first console you use works, the second one gets confused as to what window it is attached to. I tried in vain all day to write tests to figure out why. Perhaps you can spot my problem? You need all of the patches from the dependent bug as well.

Comment 24

[David Dahl :ddahl]

Attachment: v 0.7 Working Patch

Re-imported old patch and now things are working as expected. It's niiice.

Comment 25

[David Dahl :ddahl]

And now a note about work-arounds:

Using the "21 args in the idl" to simulate JS arguments has at least one pitfall. If a developer wants to know about undefined args, they are out of luck, unless we keep all undefind args and print them out in the console as either a series of spaces or a series of "undefined" strings. Either way, this not a good solution. Until the wrapping can be properly handled via bug 580128

As part of this work-around, I will drop any undefined args that have no follwing defined args. Lame, but I see no other way to handle this for the time being.

Comment 26

[Johnathan Nightingale [:johnath]]

Kevin asserts that this can be post-feature-freeze. Moving to betaN+

Comment 27

[David Dahl :ddahl]

Attachment: test patch please ignore

just at test, saving work

Comment 28

[David Dahl :ddahl]

Attachment: Saving UnBitrot work

Saving work. Almost done unbitrotting this after some major hg FAIL.

Comment 29

[David Dahl :ddahl]

Attachment: 0.8 Working Patch, most tests pass

The main test file now passes, with some failures commented out. Followup bugs will be created. There is a massive assert to track down. Attaching the assertion bt.

Comment 30

[David Dahl :ddahl]

Attachment: assert

Comment 31

[David Dahl :ddahl]

I have to get the rest of the tests passing, and figure out what is assertng:

###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file c:/moz/mozilla-central/mozilla/obj-dbg/content/events/src/../../../../content/events/src/nsEven
tDispatcher.cpp, line 514
xul!nsEventDispatcher::DispatchDOMEvent+0x000000000000014A (c:\moz\mozilla-central\mozilla\content\events\src\nseventdispatcher.cpp, line 691)
xul!nsGlobalWindow::DispatchEvent+0x00000000000000EE (c:\moz\mozilla-central\mozilla\dom\base\nsglobalwindow.cpp, line 6864)
xul!nsGlobalWindow::DispatchEvent+0x0000000000000067 (c:\moz\mozilla-central\mozilla\dom\base\nsglobalwindow.cpp, line 6847)
xul!nsContentUtils::DispatchTrustedEvent+0x0000000000000125 (c:\moz\mozilla-central\mozilla\content\base\src\nscontentutils.cpp, line 3438)
xul!nsFocusManager::WindowLowered+0x0000000000000121 (c:\moz\mozilla-central\mozilla\dom\base\nsfocusmanager.cpp, line 742)
xul!nsWebShellWindow::HandleEvent+0x0000000000000619 (c:\moz\mozilla-central\mozilla\xpfe\appshell\src\nswebshellwindow.cpp, line 467)

Then I will get the review process started

Comment 32

[David Dahl :ddahl]

The rest of the test failures are mainly this:

the value of label 18 ends with a newline - Got  , expected
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_586142_insert_newlines.js |
the value of label 19 ends with a newline - Got  , expected

almost there.

Comment 33

[David Dahl :ddahl]

Attachment: 0.9 For review, all (almost) tests pass

Still have an assertion i am tracking down. Had to comment out a few test checks, see bug 600095

Comment 34

[Dietrich Ayala (:dietrich)]

Comment on attachment 478939 [details] [diff] [review]
0.9 For review, all (almost) tests pass

approach looks good. but if you have to disable a bunch of tests, this isn't ready. please kill the other bug, and fix the tests here. also, remove all the debugging code.

Comment 35

[David Dahl :ddahl]

Attachment: Saving Work, test cleanup

Comment 36

[David Dahl :ddahl]

Attachment: 0.10 All tests pass

fixed and re-enabled all borked tests

Comment 37

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This bug is not marked b7+, but we don't have a way for users to open the error console that does collect "previous" errors.  I expect that we are going to want a lot of people to help us diagnose site errors (maybe our bugs, maybe evangelism) in the expanded beta7 audience, and in our current state it will be far too difficult for them to do so.

Requesting re-evaluation of this bug as a b7 blocker, though the right thing might be to put the error console back instead.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

The error console persists, though the menuItem has been removed. This was perhaps an error in light of this bug, indeed. I'd be happy to revert that change for now, reinstating it once the web console provides the same functionality. I'll go find that bug and ask for that to be done.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #37)
> I expect that we are going to want a lot of people to help us diagnose site
> errors [...] and in our current state it will be far too difficult for them to
> do so.

I think this is an overstatement. I think that the majority of site errors that we'd want diagnosed can be reproduced after opening the console and reloading, and if they really can't, there is the workaround of flipping the console pref, opening a new window, and checking that (which can be simplified to just "flip the console pref" if we take bug 596249).

Comment 40

[David Dahl :ddahl]

(In reply to comment #38)
> The error console persists, though the menuItem has been removed. This was
> perhaps an error in light of this bug, indeed. I'd be happy to revert that
> change for now, reinstating it once the web console provides the same
> functionality. I'll go find that bug and ask for that to be done.

I vote for this regardless. We will never surface all chrome and xpconnect errors in the web console anyway. It would not be a crime to have a "Browser Console" as well as a "Web Console" - unless we figure out the slick UX and UI for this problem. This has been a conundrum for me since day one.

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

Filed bug 601201 to turn the Error Console back on for beta7

Comment 42

[David Dahl :ddahl]

Attachment: v 0.11 Added ability to see previous consoleAPI logging

Still working on a few tests that do not pass when the whole suite is run. They always pass individually. I may need to figure out proper setUp and tearDown solution for this suite. arrgghhh.

Comment 43

[Dietrich Ayala (:dietrich)]

Comment on attachment 480351 [details] [diff] [review]
v 0.11 Added ability to see previous consoleAPI logging

> [scriptable, uuid(b49c18f8-3379-4fc0-8c90-d7772c1a9ff3)]
> interface nsIConsoleAPI : nsISupports

rev the uuid

>+function log(aMsg)
>+{
>+  dump("*** ConsoleAPI: " + aMsg + "\n");
>+}
>+

remove

>+  __exposedProps__: {
>+    log: "r",
>+    info: "r",
>+    warn: "r",
>+    error: "r",
>+  },
>+

this looks fine to me, but you should get a review from mrbkap on these bits.

>+var log = function log(aMsg){dump("*** Storage *** " + aMsg + "\n");};
>+

remove

>+  getWindowId: function HS_getWindowId(aWindow)
>+  {
>+    let util = {};
>+    try {
>+      util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+        getInterface(Ci.nsIDOMWindowUtils);
>+      util =  XPCNativeWrapper.unwrap(util);
>+      return util.outerWindowID;
>+    }
>+    catch (ex) {
>+      Cu.reportError(ex);
>+      return null;
>+    }
>+  },

util isn't used outside the block, so just instanciate it inline.

>+
>+  /**
>+   * Get the "OuterWindow" by it's id

s/'//

>+   *
>+   * @param integer aId
>+   * @returns nsIDOMWindow
>+   */
>+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
>+  {
>+    let windowEnum = Services.wm.getEnumerator("navigator:browser");
>+
>+    while (windowEnum.hasMoreElements()) {
>+      let _window = windowEnum.getNext();
>+      let gBrowser = _window.gBrowser;
>+      let _browsers = gBrowser.browsers;
>+      let browserLen = _browsers.length;

why the mixed underscore prefix and not? make it consistent?

>@@ -1743,17 +1800,17 @@ HUD_SERVICE.prototype =
>         splitters[i].parentNode.removeChild(splitters[i]);
>         break;
>       }
>     }
>     // remove the DOM Nodes
>     parent.removeChild(outputNode);
> 
>     this.windowRegistry[id].forEach(function(aContentWindow) {
>-      if (aContentWindow.wrappedJSObject.console instanceof HUDConsole) {
>+    if (aContentWindow.wrappedJSObject.console instanceof Ci.nsIConsoleAPI) {
>         delete aContentWindow.wrappedJSObject.console;
>       }
>     });

fix indent

>+  observe: function CAO_observe(aSubject, aTopic, aData)
>   {
>+    aSubject = aSubject.wrappedJSObject;

rename this so that it's clear what it actually is, like you do with aData below.

also, move it inside the block since it's not used elsewhere.

>+
>+    if (aTopic == "console-api-log-event") {
>+      let windowId = parseInt(aData);
>+      // get the window from aData, which is the "Window.ID"
>+      let hudId;
>+      let displays = HUDService._headsUpDisplays;
>+      let foundConsoleId = false;
>+      for (let idx in displays) {
>+        if (parseInt(displays[idx].windowId) == parseInt(windowId)) {
>+          hudId = displays[idx].id;
>+          foundConsoleId = true;
>+          let webConsole = HUDService.hudWeakReferences[hudId].get();
>+
>+          this.sendToWebConsole(webConsole, aSubject.level, aSubject.arguments);
>+          return;
>+        }
>+      }
>+
>+      if (!foundConsoleId) {
>+        Cu.reportError("WebConsole is not registered in HUDService");
>+        return;
>+      }

nit: remove the early returns, not really necessary here.

>     node.setAttribute("class", "jsterm-output-line hud-clickable");
>     node.setAttribute("aria-haspopup", "true");
>     node.setAttribute("crop", "end");
>     node.onclick = function() {
>       self.openPropertyPanel(aEvalString, aOutputObject, node);
>-    }
>+    };

while you're here, please switch this to addEventListener.

>   if (aSubject.arguments.length) {
>     for (let i = 0; i < aSubject.arguments.length; i++) {
>+      if (aSubject.arguments[i]) {
>       ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
>     }
>   }
> }
>+}

fix indent

>     is(typeof console, "object", "window.console is an object, after page reload");
>     is(typeof console.log, "function", "console.log is a function");
>     is(typeof console.info, "function", "console.info is a function");
>     is(typeof console.warn, "function", "console.warn is a function");
>     is(typeof console.error, "function", "console.error is a function");
>-    is(typeof console.exception, "function", "console.exception is a function");
>+    // is(typeof console.exception, "function", "console.exception is a function");

??

mostly nits. r- because there's a bunch of them. will r+ with these fixed.

Comment 44

[David Dahl :ddahl]

Attachment: v 0.12 Nits addressed

Still need to investigate this assertion and a handful of checks still fail when the entire test suite is run - and it is intermittent as far as which 5 or 6 checks fail. I will address that completely as I unbitrot this patch - I am a few days out of sync with trunk because of some hg "malformed patch" issues.

Comment 45

[David Dahl :ddahl]

So: I still have a massive hg "malformed patch" problem:

MacFXDev:mozilla moco$ patch --verbose -p1 < ~/Desktop/integration-take-3 
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|# HG changeset patch
|# Parent d8a17282693639f59626213646e05582c0d56435
|
|diff --git a/toolkit/components/console/hudservice/ConsoleAPI.idl b/toolkit/components/console/hudservice/ConsoleAPI.idl
|--- a/toolkit/components/console/hudservice/ConsoleAPI.idl
|+++ b/toolkit/components/console/hudservice/ConsoleAPI.idl
--------------------------
Patching file toolkit/components/console/hudservice/ConsoleAPI.idl using Plan A...
Hunk #1 succeeded at 38.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/toolkit/components/console/hudservice/ConsoleAPI.js b/toolkit/components/console/hudservice/ConsoleAPI.js
|--- a/toolkit/components/console/hudservice/ConsoleAPI.js
|+++ b/toolkit/components/console/hudservice/ConsoleAPI.js
--------------------------
Patching file toolkit/components/console/hudservice/ConsoleAPI.js using Plan A...
Hunk #1 succeeded at 40.
Hunk #2 succeeded at 129.
Hunk #3 succeeded at 185.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/toolkit/components/console/hudservice/ConsoleAPI.manifest b/toolkit/components/console/hudservice/ConsoleAPI.manifest
|--- a/toolkit/components/console/hudservice/ConsoleAPI.manifest
|+++ b/toolkit/components/console/hudservice/ConsoleAPI.manifest
--------------------------
Patching file toolkit/components/console/hudservice/ConsoleAPI.manifest using Plan A...
Hunk #1 succeeded at 1.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/toolkit/components/console/hudservice/ConsoleStorageService.js b/toolkit/components/console/hudservice/ConsoleStorageService.js
|--- a/toolkit/components/console/hudservice/ConsoleStorageService.js
|+++ b/toolkit/components/console/hudservice/ConsoleStorageService.js
--------------------------
Patching file toolkit/components/console/hudservice/ConsoleStorageService.js using Plan A...
Hunk #1 succeeded at 128.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
|--- a/toolkit/components/console/hudservice/HUDService.jsm
|+++ b/toolkit/components/console/hudservice/HUDService.jsm
--------------------------
Patching file toolkit/components/console/hudservice/HUDService.jsm using Plan A...
Hunk #1 succeeded at 58.
Hunk #2 succeeded at 1248.
Hunk #3 succeeded at 1720.
Hunk #4 succeeded at 1800.
Hunk #5 succeeded at 1988.
Hunk #6 succeeded at 2027.
Hunk #7 succeeded at 2804.
Hunk #8 succeeded at 2850.
Hunk #9 succeeded at 2913.
Hunk #10 succeeded at 3000.
Hunk #11 succeeded at 3073.
patch: **** malformed patch at line 851:   * See bug 594304

What can we do to fix this? I have unbitrotted this by hand like 3 times now. I really have no idea why it is "malformed".

Comment 46

[Dietrich Ayala (:dietrich)]

something on your disk, or in your workflow, is not right. nothing to do with this bug, is my bet.

Comment 47

[David Dahl :ddahl]

(In reply to comment #46)
> something on your disk, or in your workflow, is not right. nothing to do with
> this bug, is my bet.

The crazy thing is that I re-created the patch from scratch on another machine altogether. I am at a loss as far as how this happened.

Comment 48

[Mihai Sucan [:msucan]]

Attachment: v 0.13 fixed patch

Fixed the patch hunks. Three of the patch hunks failed to apply with the error "bad hunk". I manually fixed them, and rebuilt the patch with Mercurial 1.6.4. The patch applies cleanly on my system.

Comment 49

[David Dahl :ddahl]

Looks like we are down to this:

INFO | runtests.py | Running tests: end.
mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js | 2 children in output
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js | found test-network
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js | found network console
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Failed to find the iframe network request in tab1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Found the iframe network request in tab2
make: *** [mochitest-browser-chrome] Error 1


Getting there!

Comment 50

[David Dahl :ddahl]

Fixed all tests, will file another followup for iframe issues. pushed to tryserver.

Comment 51

[David Dahl :ddahl]

Attachment: try server failures

Major failures on try server. Every test failed. Why does this all work locally?

Comment 52

[David Dahl :ddahl]

This actually looks like a build error, as all tests timeout after an instanceof check fails. Ci.nsIConsoleAPI must not be built correctly...

NEXT ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js | Test timed out
*** WebConsoleTest: TypeError: invalid 'instanceof' operand Ci.nsIConsoleAPI


log:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287030758.1287032863.29539.gz

jst, do you know what is going on?

Comment 53

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Did an XPT file not get added to the packaging or mega-XPT correctly?

Comment 54

[David Dahl :ddahl]

(In reply to comment #53)
> Did an XPT file not get added to the packaging or mega-XPT correctly?

that sounds like the ticket. for 2 or 3 components. whoops.

Comment 55

[David Dahl :ddahl]

xpcshell test issue on try:

TEST-UNEXPECTED-FAIL | c:\talos-slave\tryserver_win7_test-xpcshell\build\xpcshell\tests\toolkit\components\console\hudservice\tests\unit\test_ConsoleStorageService.js | test failed (with xpcshell return code: 3), see following log:
  >>>>>>>
  c:/talos-slave/tryserver_win7_test-xpcshell/build/xpcshell/tests/toolkit/components/console/hudservice/tests/unit/test_ConsoleStorageService.js:44: ReferenceError: Cc is not defined


There was also an orange warning due to an Assertion on the Mac Build:

###!!! ASSERTION: NULL mIconRequest!  Multiple calls to OnStopRequest()?: 'mIconRequest', file /builds/slave/tryserver-macosx-debug/build/widget/src/cocoa/nsMenuItemIconX.mm, line 549
TEST-UNEXPECTED-FAIL | automation.py | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:00:12.888954
INFO | automation.py | Reading PID log: /var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-/tmpgFckKWpidlog
PROCESS-CRASH | automation.py | application crashed (minidump found)
No symbols path given, can't process dump.
program finished with exit code 1
elapsedTime=13.687553

Comment 56

[David Dahl :ddahl]

a couple of tweaks and some tiny patch reviews and we can land it.

Comment 57

[David Dahl :ddahl]

Attachment: 0.14 Rebased Patch

Comment 58

[David Dahl :ddahl]

Rebased, rebuilt and am now seeing errors like this that just kill the test suite cold:

*** Compartment mismatch 0x45584800 vs. 0x465f5800
Assertion failure: compartment mismatched, at /home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jscntxtinlines.h:513
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_588967_input_expansion.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:00:38.603234
INFO | automation.py | Reading PID log: /tmp/tmpEjbp_ipidlog
PROCESS-CRASH | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_588967_input_expansion.js | application crashed (minidump found)
Neither MINIDUMP_STACKWALK nor MINIDUMP_STACKWALK_CGI is set, can't process dump.

Comment 59

[David Dahl :ddahl]

Attachment: 0.15 Fixed test crash -all tests pass

We were crashing with some command line tests, not anymore. there are still wrapper issues to deal with as well. All tests pass now

Comment 60

[David Dahl :ddahl]

ah. fixes tab-completion as well:)

Comment 61

[David Dahl :ddahl]

Attachment: 0.16 reverted a test - all tests pass

reverted a test

Comment 62

[David Dahl :ddahl]

shawn: i only need review on the interdiff between 0.14 and 0.16 - v. 0.14 has r+ already:)

Comment 63

[Dave Townsend [:mossop]]

Comment on attachment 483737 [details] [diff] [review]
0.16 reverted a test - all tests pass

Drive by:

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.idl b/toolkit/components/console/hudservice/ConsoleAPI.idl
>--- a/toolkit/components/console/hudservice/ConsoleAPI.idl
>+++ b/toolkit/components/console/hudservice/ConsoleAPI.idl
>@@ -38,20 +38,100 @@
> #include "nsISupports.idl"
> #include "nsIDOMWindow.idl"
> #include "nsIVariant.idl"
> 
> /**
>  * nsIConsoleAPI lazily provides a console property on any contentWindow.
>  * console.info, .log, .warn and .error are provided
>  */
>-
>-[scriptable, uuid(b49c18f8-3379-4fc0-8c90-d7772c1a9ff3)]
>+/* ce427238-d697-4db4-86b2-04bda5063a0d */

Wut?

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.js b/toolkit/components/console/hudservice/ConsoleAPI.js

> ConsoleAPI.prototype = {
> 
>+  wrappedJSObject: this,
>+
>   classDescription: "Logging API for Web content",
> 
>-  classID: Components.ID("{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}"),
>+  classID: Components.ID("{d488614d-8721-445a-924c-34bdd1536d8e}"),

No need to change the classID

>   contractID: "@mozilla.org/console-api;1",

Nit: This isn't generally needed anymore

>   getWindowId: function CA_getWindowId(aOuterWindow)
>   {
>     // aOuterWindow is an XPCWrappedNative
>     try {
>       let util = aOuterWindow.QueryInterface(Ci.nsIInterfaceRequestor).
> 	               getInterface(Ci.nsIDOMWindowUtils);
>+      util =  XPCNativeWrapper.unwrap(util);

Nit: double space

>       return util.outerWindowID;
>     }
>     catch (ex) {
>       Cu.reportError(ex);
>       return null;
>     }
>   },
> 
>   /**
>    * Init the component with the associated contentWindow
>    *
>    * @param nsIDOMWindow aOuterWindow
>    * @returns void
>    */
>   init: function CA_init(aOuterWindow)
>   {
>+    if (!(aOuterWindow.top === aOuterWindow)) {
>+      // If the aOuterWindow is an iframe, set it's aOuterWindow to the top


This comment makes little sense. How about "If aOuterWindow is a frame's window use the top-most window instead

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.manifest b/toolkit/components/console/hudservice/ConsoleAPI.manifest
>--- a/toolkit/components/console/hudservice/ConsoleAPI.manifest
>+++ b/toolkit/components/console/hudservice/ConsoleAPI.manifest
>@@ -1,3 +1,3 @@
>-component {b49c18f8-3379-4fc0-8c90-d7772c1a9ff3} ConsoleAPI.js
>-contract @mozilla.org/console-api;1 {b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}
>+component {d488614d-8721-445a-924c-34bdd1536d8e} ConsoleAPI.js
>+contract @mozilla.org/console-api;1 {d488614d-8721-445a-924c-34bdd1536d8e}
> category JavaScript-global-property console @mozilla.org/console-api;1

These changes aren't necessary if you don't change the class ID

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   /**
>+   * get "OuterWindow ID" from a window object

"Gets the ID of the outer window of this DOM window"

>+   *
>+   * @param nsIDOMWindow aWindow
>+   * @returns integer

These comments don't the regular javadoc syntax that we use elsewhere, but I guess they are the same through the rest of the file. Really though they are useless without actual comments.

>+   */
>+  getWindowId: function HS_getWindowId(aWindow)
>+  {
>+    let util = {};
>+    try {
>+      util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+        getInterface(Ci.nsIDOMWindowUtils);

Normally you'd put the . at the start of the second line and line it up with the . above

>+  /**
>+   * Get the "OuterWindow" by its id

"Gets the top level content window that has an outer window with the given ID or returns null if no such content window exists"

>+   *
>+   * @param integer aId
>+   * @returns nsIDOMWindow
>+   */
>+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
>+  {
>+    let windowEnum = Services.wm.getEnumerator("navigator:browser");
>+
>+    while (windowEnum.hasMoreElements()) {
>+      let window = windowEnum.getNext();
>+      let gBrowser = window.gBrowser;
>+      let browsers = gBrowser.browsers;
>+      let browserLen = browsers.length;
>+
>+      for (var i = 0; i < browserLen; i++) {
>+        let browser = browsers[i];
>+        let window = browser.contentWindow;
>+        let id = this.getWindowId(window);
>+        if (parseInt(id) === parseInt(aId)) {

Why is the parseInt necessary?

Comment 64

[David Dahl :ddahl]

(In reply to comment #63)

> >-[scriptable, uuid(b49c18f8-3379-4fc0-8c90-d7772c1a9ff3)]
> >+/* ce427238-d697-4db4-86b2-04bda5063a0d */
> 
> Wut?

oops. Nothing to see here. comment removal. 

> 
> >   contractID: "@mozilla.org/console-api;1",
> 
> Nit: This isn't generally needed anymore

Like not at all? wacky. where is that documented? (ha ha) (Now mossop finds the docs on MDC no doubt)

> 
> >   getWindowId: function CA_getWindowId(aOuterWindow)
> >   {
> >     // aOuterWindow is an XPCWrappedNative
> >     try {
> >       let util = aOuterWindow.QueryInterface(Ci.nsIInterfaceRequestor).
> > 	               getInterface(Ci.nsIDOMWindowUtils);
> >+      util =  XPCNativeWrapper.unwrap(util);
> 
> Nit: double space

don't get me started. some reviewers like it like that

> >   /**
> >    * Init the component with the associated contentWindow
> >    *
> >    * @param nsIDOMWindow aOuterWindow
> >    * @returns void
> >    */
> >   init: function CA_init(aOuterWindow)
> >   {
> >+    if (!(aOuterWindow.top === aOuterWindow)) {
> >+      // If the aOuterWindow is an iframe, set it's aOuterWindow to the top
> 
> 
> This comment makes little sense. How about "If aOuterWindow is a frame's window
> use the top-most window instead

ok

> >+contract @mozilla.org/console-api;1 {d488614d-8721-445a-924c-34bdd1536d8e}
> > category JavaScript-global-property console @mozilla.org/console-api;1
> 
> These changes aren't necessary if you don't change the class ID

things work, i am not changing it back:)
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   /**
> >+   * get "OuterWindow ID" from a window object
> 
> "Gets the ID of the outer window of this DOM window"
> 
> >+   *
> >+   * @param nsIDOMWindow aWindow
> >+   * @returns integer
> 
> These comments don't the regular javadoc syntax that we use elsewhere, but I
> guess they are the same through the rest of the file. Really though they are
> useless without actual comments.

what line? I cannot find a @param like this wihthout a comment preceeding

> 
> >+   */
> >+  getWindowId: function HS_getWindowId(aWindow)
> >+  {
> >+    let util = {};
> >+    try {
> >+      util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
> >+        getInterface(Ci.nsIDOMWindowUtils);
> 
> Normally you'd put the . at the start of the second line and line it up with
> the . above

ok. lining it up is not kosher to other reviewers. but you are the toolkit owner. I am so confused.

> >+   *
> >+   * @param integer aId
> >+   * @returns nsIDOMWindow
> >+   */
> >+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
> >+  {
> >+    let windowEnum = Services.wm.getEnumerator("navigator:browser");
> >+
> >+    while (windowEnum.hasMoreElements()) {
> >+      let window = windowEnum.getNext();
> >+      let gBrowser = window.gBrowser;
> >+      let browsers = gBrowser.browsers;
> >+      let browserLen = browsers.length;
> >+
> >+      for (var i = 0; i < browserLen; i++) {
> >+        let browser = browsers[i];
> >+        let window = browser.contentWindow;
> >+        let id = this.getWindowId(window);
> >+        if (parseInt(id) === parseInt(aId)) {
> 
> Why is the parseInt necessary?

I thought for a time that sometimes I would get a string instead of an int. so I made it al explicit.

You have reviewed everything but the changes needed to land this. (Since all of this was r+'d already) I do thank you for the improvements here, but I was hoping to have someone review the single line of unwrapping an object to fix 2 bugs with one patch here:

https://bugzilla.mozilla.org/attachment.cgi?oldid=483549&action=interdiff&newid=483737&headers=1

Your drive-by makes me think it would be helpful to have you review the entire HUDService.jsm before Firefox 4 ships. I'm sure it would help polish this up a lot - you know in your copious free time.

Thanks again!

Comment 65

[Shawn Wilsher :sdwilsh]

(In reply to comment #64)
> don't get me started. some reviewers like it like that
Er, that'd be counter to anything in the mozilla code base...

> what line? I cannot find a @param like this wihthout a comment preceeding
Pretty sure he's asking for descriptions of what the variables are like I've been doing in my reviews of this code.  Just having the argument name isn't terribly useful.

Comment 66

[David Dahl :ddahl]

(In reply to comment #65)
> (In reply to comment #64)
> > don't get me started. some reviewers like it like that
> Er, that'd be counter to anything in the mozilla code base...
> 
ah, ok.

> > what line? I cannot find a @param like this wihthout a comment preceeding
> Pretty sure he's asking for descriptions of what the variables are like I've
> been doing in my reviews of this code.  Just having the argument name isn't
> terribly useful.

I think I am going to stop working on weekends as I am clearly distracted.

Thanks.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 483737 [details] [diff] [review]
0.16 reverted a test - all tests pass

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.js b/toolkit/components/console/hudservice/ConsoleAPI.js

>   getWindowId: function CA_getWindowId(aOuterWindow)
>   {
>     // aOuterWindow is an XPCWrappedNative
>     try {
>       let util = aOuterWindow.QueryInterface(Ci.nsIInterfaceRequestor).
> 	               getInterface(Ci.nsIDOMWindowUtils);
>+      util =  XPCNativeWrapper.unwrap(util);
>       return util.outerWindowID;

This shouldn't be needed - I don't think getInterface will return an XPCNativeWrapper, but even if it does, outerWindowID is an IDL-exposed property, so unwrapping should have no effect. This same comment applies to the copy of this function in HUDService.jsm.

>   init: function CA_init(aOuterWindow)
>   {
>+    if (!(aOuterWindow.top === aOuterWindow)) {
>+      // If the aOuterWindow is an iframe, set it's aOuterWindow to the top
>+      aOuterWindow = aOuterWindow.top;
>+    }

No need for the check, just do "aOuterWindow = aOuterWindow.top;" unconditionally - if aOuterWindow is a topmost window, .top will return itself. "aOuterWindow" is a confusing variable name, though (since "outer windows" are actually a thing...).

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm

>+  getWindowByWindowId: function HS_getWindowByWindowId(aId)

Who uses this method? It's rather expensive to enumerate all windows and find them by window ID, and this won't find subframe windows. Is that the cause of bug 604238?

>     this.windowRegistry[id].forEach(function(aContentWindow) {
>-      if (aContentWindow.wrappedJSObject.console instanceof HUDConsole) {
>+      if (aContentWindow.wrappedJSObject.console instanceof Ci.nsIConsoleAPI) {
>         delete aContentWindow.wrappedJSObject.console;
>       }
>     });

Why do you need to delete the content window's console object?
 

>-    let _browser = gBrowser.
>-      getBrowserForDocument(aContentWindow.top.document.wrappedJSObject);
>+    let _browser =
>+      gBrowser.getBrowserForDocument(aContentWindow.wrappedJSObject.document);

Why are you losing the "top"? Why use wrappedJSObject, given that you're only getting .document (which should work just fine using a wrapper)?


>+    // need to detect that the console component has been paved over
>+    if (!(aContentWindow.wrappedJSObject.console instanceof Ci.nsIConsoleAPI)) {
>       this.logWarningAboutReplacedAPI(hudId);
>     }
>-    else {
>-      aContentWindow.wrappedJSObject.console = hud.console;
>-    }

Hurray :)

>+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)

>     let argumentArray = [];
>     for (var i = 0; i < aArguments.length; i++) {
>       argumentArray.push(aArguments[i]);
>     }
> 
>+    argumentArray.push("\n");
>     let message = argumentArray.join(' ');

Not really related to this bug, but this whole block could just be |let message = Array.join(aArguments, " ") + "\n";|, I believe.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js

>   observe: function CO_observe(aSubject, aTopic, aData)
>   {
>     if (aTopic == "console-api-log-event") {
>-      testConsoleData(aSubject.wrappedJSObject);
>+      aSubject = aSubject.wrappedJSObject;
>+      testConsoleData(aSubject);

This change doesn't seem beneficial (more verbose and overwrites an argument for no reason).

> function testConsoleData(aSubject)

>+    window = XPCNativeWrapper.unwrap(window);

This shouldn't be needed, since you don't actually get any properties off window AFAICT...

>+function testOpenUI()

>+  browser.contentWindow.wrappedJSObject.location = TEST_URI;

You don't need .wrappedJSObject here.

> function test()

>-  content.location = TEST_URI;
>+  addTab(TEST_URI);

>   browser.addEventListener("DOMContentLoaded", function onLoad(event) {

This doesn't look right - I don't see where addTab is defined, but unless it somehow resets the "browser" global, it seems like you're adding a listener to the wrong browser...

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js

>-    ok(nodes.length == 2, "2 children in output");
>+    ok(nodes.length == 3, "2 children in output");

Check doesn't seem to match comment now...

>-    ok(nodes[0].textContent.indexOf("test-network") > -1, "found test-network");
>+    ok(nodes[1].textContent.indexOf("test-network") > -1, "found test-network");

Why did the number/position of nodes change?

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js

>+  // TODO: this fails as we do not log iframes to the proper console anymore!
>+  // ok(nodes[1].textContent.indexOf("Network:"), "Found Network Request");
>+  // testLogEntry(outputNode1, "Network:",
>+  //             { success: successMsg1, err: errorMsg1}, true);

What is the plan to fix this?

>-  let hudId2 = HUDService.getHudIdByWindow(tab2.linkedBrowser.contentWindow);
>+  let hudId2 = HUDService.getHudIdByWindow(tab2.linkedBrowser.contentWindow.top);

This isn't needed - browser.contentWindow can only return a top level window.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js

>+  // TODO: this behavior has changed, the DOMMutation listener set in makeHUDNodes does not modify the visibility on these nodes that are filtereed out
>+  // this behavior is also going to change soon, as we need to get rid of the mutation listeners as they are heavy-weight

This comment is confusing (both "has changed" and "going to change soon"? Which is it?) Mention the specific bug #s?

>+  nodes = outputNode.querySelectorAll("label");
>+  for (i = 0; i < nodes.length; i++) {
>+    if (nodes[i].classList.contains("hud-filtered-by-type")) {
>+      foundCount ++;
>+    }
>+  }

foundCount = outputNode.querySelectorAll("label.hud-filtered-by-type").length? Also applies above for .hud-msg-node.hud-filtered-by-string (though shouldn't these use the same selector?)

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_consoleonpage.js

>-      is(consoleIFrame, undefined, "Console object was removed from iFrame");
>+      ok(consoleIFrame != null, "Console object was not removed from iFrame");

What's the justification for this change? More fallout from iframes being broken?

Comment 68

[Mihai Sucan [:msucan]]

(In reply to comment #67)
> Comment on attachment 483737 [details] [diff] [review]
> 0.16 reverted a test - all tests pass
> 
> >-    let _browser = gBrowser.
> >-      getBrowserForDocument(aContentWindow.top.document.wrappedJSObject);
> >+    let _browser =
> >+      gBrowser.getBrowserForDocument(aContentWindow.wrappedJSObject.document);
> 
> Why are you losing the "top"? Why use wrappedJSObject, given that you're only
> getting .document (which should work just fine using a wrapper)?

(just a quick chime in on the importance of this review comment!)

For David: we should not remove .top. In bug 593003 we changed the code to use .top to fix that bug.

Additionally, as Gavin says, .wrappedJSObject is not needed, it actually breaks the test in bug 603211 since the compartments stuff has landed.

Comment 69

[David Dahl :ddahl]

(In reply to comment #67)
> >+      util =  XPCNativeWrapper.unwrap(util);
> >       return util.outerWindowID;
> 
> This shouldn't be needed - I don't think getInterface will return an
> XPCNativeWrapper, but even if it does, outerWindowID is an IDL-exposed
> property, so unwrapping should have no effect. This same comment applies to the
> copy of this function in HUDService.jsm.

ah, ok, I will remove it and see what the test suites does. I had some major fail without it - I thought.

> No need for the check, just do "aOuterWindow = aOuterWindow.top;"
> unconditionally - if aOuterWindow is a topmost window, .top will return itself.
> "aOuterWindow" is a confusing variable name, though (since "outer windows" are
> actually a thing...).

ok

> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
> 
> Who uses this method? It's rather expensive to enumerate all windows and find
> them by window ID, and this won't find subframe windows. Is that the cause of
> bug 604238?
hmm, perhaps. We use this method inside of the ConsoelAPIObserver to get the window object from the integer ID.

> 
> >     this.windowRegistry[id].forEach(function(aContentWindow) {
> >-      if (aContentWindow.wrappedJSObject.console instanceof HUDConsole) {
> >+      if (aContentWindow.wrappedJSObject.console instanceof Ci.nsIConsoleAPI) {
> >         delete aContentWindow.wrappedJSObject.console;
> >       }
> >     });
> 
> Why do you need to delete the content window's console object?
we did have to do that before we implemented the lazy console - i think now that line can be removed.

> 
> 
> >-    let _browser = gBrowser.
> >-      getBrowserForDocument(aContentWindow.top.document.wrappedJSObject);
> >+    let _browser =
> >+      gBrowser.getBrowserForDocument(aContentWindow.wrappedJSObject.document);
> 
> Why are you losing the "top"? Why use wrappedJSObject, given that you're only
> getting .document (which should work just fine using a wrapper)?

losing the top was probably a testing thing i forgot to reset.

aContentWindow.document works in this scope? cool.

> 
> 
> >+    // need to detect that the console component has been paved over
> >+    if (!(aContentWindow.wrappedJSObject.console instanceof Ci.nsIConsoleAPI)) {
> >       this.logWarningAboutReplacedAPI(hudId);
> >     }
> >-    else {
> >-      aContentWindow.wrappedJSObject.console = hud.console;
> >-    }
> 
> Hurray :)
Indeed.

> 
> >+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)
> ...
> 
> Not really related to this bug, but this whole block could just be |let message
> = Array.join(aArguments, " ") + "\n";|, I believe.

Fancy. I'll change it.

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js
> 
> >   observe: function CO_observe(aSubject, aTopic, aData)
> >   {
> >     if (aTopic == "console-api-log-event") {
> >-      testConsoleData(aSubject.wrappedJSObject);
> >+      aSubject = aSubject.wrappedJSObject;
> >+      testConsoleData(aSubject);
> 
> This change doesn't seem beneficial (more verbose and overwrites an argument
> for no reason).

It's like that because of logging, and I must have forgotten to change it back to the original way it was.

> 
> > function testConsoleData(aSubject)
> 
> >+    window = XPCNativeWrapper.unwrap(window);
> 
> This shouldn't be needed, since you don't actually get any properties off
> window AFAICT...

ah cool. ok. wrappers confuse me.

> 
> >+function testOpenUI()
> 
> >+  browser.contentWindow.wrappedJSObject.location = TEST_URI;
> 
> You don't need .wrappedJSObject here.

Is there a table somewhere on what properties are accessible without wrappedJSObject?

> 
> > function test()
> 
> >-  content.location = TEST_URI;
> >+  addTab(TEST_URI);
> 
> >   browser.addEventListener("DOMContentLoaded", function onLoad(event) {
> 
> This doesn't look right - I don't see where addTab is defined, but unless it
> somehow resets the "browser" global, it seems like you're adding a listener to
> the wrong browser...
addTab and all boilerplat is in head.js

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js
> 
> >-    ok(nodes.length == 2, "2 children in output");
> >+    ok(nodes.length == 3, "2 children in output");
> 
> Check doesn't seem to match comment now...

will change that
> 
> >-    ok(nodes[0].textContent.indexOf("test-network") > -1, "found test-network");
> >+    ok(nodes[1].textContent.indexOf("test-network") > -1, "found test-network");
> 
> Why did the number/position of nodes change?
I simplified the querySelect that gets the nodes - it was very convoluted.

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js
> 
> >+  // TODO: this fails as we do not log iframes to the proper console anymore!
> >+  // ok(nodes[1].textContent.indexOf("Network:"), "Found Network Request");
> >+  // testLogEntry(outputNode1, "Network:",
> >+  //             { success: successMsg1, err: errorMsg1}, true);
> 
> What is the plan to fix this?
we have a bug, i will annotate it in the test source. bug 604238

> 
> >-  let hudId2 = HUDService.getHudIdByWindow(tab2.linkedBrowser.contentWindow);
> >+  let hudId2 = HUDService.getHudIdByWindow(tab2.linkedBrowser.contentWindow.top);
> 
> This isn't needed - browser.contentWindow can only return a top level window.
cool, did not know that.

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js
> 
> >+  // TODO: this behavior has changed, the DOMMutation listener set in makeHUDNodes does not modify the visibility on these nodes that are filtereed out
> >+  // this behavior is also going to change soon, as we need to get rid of the mutation listeners as they are heavy-weight
> 
> This comment is confusing (both "has changed" and "going to change soon"? Which
> is it?) Mention the specific bug #s?
The node filtering did not work the same way with the lazy console, it seemed like the event listener was not triggered when we logged things. I cannot remember or find the bug that some one mentioned to patrick walton that the DOMMutation listeners were too heavy weight to be on for filtering nodes. I'll try to find it.

> 
> >+  nodes = outputNode.querySelectorAll("label");
> >+  for (i = 0; i < nodes.length; i++) {
> >+    if (nodes[i].classList.contains("hud-filtered-by-type")) {
> >+      foundCount ++;
> >+    }
> >+  }
> 
> foundCount = outputNode.querySelectorAll("label.hud-filtered-by-type").length?
> Also applies above for .hud-msg-node.hud-filtered-by-string (though shouldn't
> these use the same selector?)

I am trying to isolate things after calling HUDService.clearDisplay()


> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_consoleonpage.js
> 
> >-      is(consoleIFrame, undefined, "Console object was removed from iFrame");
> >+      ok(consoleIFrame != null, "Console object was not removed from iFrame");
> 
> What's the justification for this change? More fallout from iframes being
> broken?

Actually, this check can be removed as you cannot make a console undefined now. Any call to window.console creates or gets a console.

Comment 70

[David Dahl :ddahl]

(In reply to comment #68)
> (just a quick chime in on the importance of this review comment!)
> 
> For David: we should not remove .top. In bug 593003 we changed the code to use
> .top to fix that bug.
> 
> Additionally, as Gavin says, .wrappedJSObject is not needed, it actually breaks
> the test in bug 603211 since the compartments stuff has landed.

Cool, thanks.

Comment 71

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #69)
> > You don't need .wrappedJSObject here.
> 
> Is there a table somewhere on what properties are accessible without
> wrappedJSObject?

You only need to unwrap when you want to retrieve non-IDL-exposed (or overridden IDL-exposed) properties. https://developer.mozilla.org/en/XPConnect_wrappers has a good overview.

> > >+  // TODO: this fails as we do not log iframes to the proper console anymore!
> > >+  // ok(nodes[1].textContent.indexOf("Network:"), "Found Network Request");
> > >+  // testLogEntry(outputNode1, "Network:",
> > >+  //             { success: successMsg1, err: errorMsg1}, true);
> > 
> > What is the plan to fix this?
> we have a bug, i will annotate it in the test source. bug 604238

I'm just nervous about landing a patch that knowingly breaks stuff without a concrete plan to fix it. Followup bug is OK if there's some justification for why we can't get things right before landing, and I haven't really seen that here (but haven't been paying close attention, so perhaps I just missed it). 

> The node filtering did not work the same way with the lazy console, it seemed
> like the event listener was not triggered when we logged things. I cannot
> remember or find the bug that some one mentioned to patrick walton that the
> DOMMutation listeners were too heavy weight to be on for filtering nodes. I'll
> try to find it.

You mean bug 603005? I filed that :) This comment still needs clarifying beyond that, though, since I'm still not sure what it's trying to say.

> I am trying to isolate things after calling HUDService.clearDisplay()

I don't understand this reply. I'm just suggesting using querySelectorAll rather than looping over the nodeList and checking classList.contains. Is that not doable for some reason?

Comment 72

[David Dahl :ddahl]

(In reply to comment #71)
> I'm just nervous about landing a patch that knowingly breaks stuff without a
> concrete plan to fix it. Followup bug is OK if there's some justification for
> why we can't get things right before landing, and I haven't really seen that
> here (but haven't been paying close attention, so perhaps I just missed it). 

Right. I will spend some time looking into this iframe issue. 

> > I am trying to isolate things after calling HUDService.clearDisplay()
> 
> I don't understand this reply. I'm just suggesting using querySelectorAll
> rather than looping over the nodeList and checking classList.contains. Is that
> not doable for some reason?

Ah, ok, that will work too. I misunderstood what you were talking about.

Comment 73

[Shawn Wilsher :sdwilsh]

Comment on attachment 483737 [details] [diff] [review]
0.16 reverted a test - all tests pass

Going to have you go ahead and address Mossop and gavin's drive-by review comments before I look at this.

Comment 74

[David Dahl :ddahl]

A note to myself, current failing tests:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js | Could not find the warning message about the replaced API
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_580030_errors_after_page_reload.js | Could not get the error message after page reload
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_589162_css_filter.js | could not find the unknown CSS property warning
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_589162_css_filter.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Failed to find the iframe network request in tab1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Found iframe network request
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/b

Comment 75

[David Dahl :ddahl]

(In reply to comment #71)
> > > >+  // TODO: this fails as we do not log iframes to the proper console anymore!
> > > >+  // ok(nodes[1].textContent.indexOf("Network:"), "Found Network Request");
> > > >+  // testLogEntry(outputNode1, "Network:",
> > > >+  //             { success: successMsg1, err: errorMsg1}, true);
> > > 
> > > What is the plan to fix this?
> > we have a bug, i will annotate it in the test source. bug 604238
> 
> I'm just nervous about landing a patch that knowingly breaks stuff without a
> concrete plan to fix it. Followup bug is OK if there's some justification for
> why we can't get things right before landing, and I haven't really seen that
> here (but haven't been paying close attention, so perhaps I just missed it). 

I have fixed these tests and they now work correctly, logging via iframes is working just fine. 

> 
> > The node filtering did not work the same way with the lazy console, it seemed
> > like the event listener was not triggered when we logged things. I cannot
> > remember or find the bug that some one mentioned to patrick walton that the
> > DOMMutation listeners were too heavy weight to be on for filtering nodes. I'll
> > try to find it.
> 
> You mean bug 603005? I filed that :) This comment still needs clarifying beyond
> that, though, since I'm still not sure what it's trying to say.

I removed the comment, and added a TODO whereby we should move all of the filtering tests out into their own test file. see bug 608135

Comment 76

[David Dahl :ddahl]

Attachment: 0.17 all tests pass

Fixed iframe tests

Comment 77

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 486770 [details] [diff] [review]
0.17 all tests pass

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.js b/toolkit/components/console/hudservice/ConsoleAPI.js

> ConsoleAPI.prototype = {

>+  wrappedJSObject: this,

Why is this needed?

>-  classID: Components.ID("{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}"),
>+  classID: Components.ID("{d488614d-8721-445a-924c-34bdd1536d8e}"),

This is wrong - component IDs and interface IDs (IIDs) are completely separate, each should have its own unique ID. You only need to bump the interface ID, in this case (due to the interface change).

>+  init: function CA_init(aWindow)

>+    // If aWindow is a frame's window use the top-most window instead
>+    aWindow = aWindow.top;

I'm not sure why we want this behavior. It seems like we should report things to the console-log-event observer using the correct window/windowID, and then have it handle coalescing messages if desired. In other words, this .top should be able to move to CAO_observe.

>diff --git a/toolkit/components/console/hudservice/ConsoleStorageService.js b/toolkit/components/console/hudservice/ConsoleStorageService.js

>   removeConsoleStorage: function CS_removeConsoleStorage(aId)

>+    else {
>+      throw new Error("There is no consoleStorage with ID: " + aId);

Why this change?

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  getWindowId: function HS_getWindowId(aWindow)

I don't think there's any reason to expect this to throw, unless you pass it an invalid argument, so I don't think the try/catch is necessary.
So this can just be:
return aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).outerWindowID;

(I may be wrong about the throwing, in which case just add that back)

>+  getWindowByWindowId: function HS_getWindowByWindowId(aId)

This can now use nsIDOMWindowUtils::getOuterWindowWithId, as in Patrick's patch for bug 597136.

>   registerDisplay: function HS_registerDisplay(aHUDId, aContentWindow)

>+    // get the window Id
>+    var windowId = this.getWindowId(aContentWindow);
>+    this._headsUpDisplays[aHUDId] = { id: aHUDId, windowId: windowId };

Doesn't this assume a 1->1 mapping between hudIDs and windowIDs? It's actually 1->many, with subframes taken into account... HUDs shouldn't really keep any reference to window IDs, IMO - instead use getHudIdByWindow(getWindowByWindowId(id)) in CAO_observe.

>-    // store this message in the storage module:
>-    this.storage.recordEntry(aMessage.hudId, aMessage);

Why this change?

>   logMessage: function HS_logMessage(aMessage, aConsoleNode, aMessageNode)

>     switch (aMessage.origin) {
>       case "network":
>       case "HUDConsole":
>+      case "WebConsole":
>       case "console-listener":
>+      case "DOM Events":
>+      case "DOM:HTML":
>         this.logHUDMessage(aMessage, aConsoleNode, aMessageNode);

I don't understand these changes - should "WebConsole" be "Web Console", to match the change to evalInSandbox?

>-    if (aContentWindow.document.location.href == "about:blank" &&
>-        HUDWindowObserver.initialConsoleCreated == false) {
>-      // TODO: need to make this work with about:blank in the future
>-      // see bug 568661
>-      return;
>-    }

This removal seems to remove the only read of initialConsoleCreated. I don't really understand what the current code was trying to do, so I guess that's OK, but you should remove the other traces of it as well.

>-      hud = this.hudWeakReferences[hudId].get();
>-      hud.reattachConsole(aContentWindow.top);
>+      let _hud;
>+      let hudWeakRef = this.hudWeakReferences[hudId];
>+      if (hudWeakRef) {
>+        _hud = hudWeakRef.get();
>+        _hud.contentWindow = aContentWindow;
>+        _hud.uriSpec = aContentWindow.location.href;
>+      }
>+      // TODO: name change?? doesn't actually re-attach the console
>+      _hud.reattachConsole(aContentWindow.top);

This doesn't look right - if hudWeakRef is null, the call to .reattachConsole will throw. If it can't be null, there's no use null checking it.

I don't really understand why we need to call reattachConsole(aContentWindow.top); here at all. What breaks if you remove this?

>+  // store the outerWindow Id
>+  this.windowId = HUDService.getWindowId(aConfig.contentWindow);

See comment above about not keeping windowID references.

>+  displayStoredConsoleMessages: function HUD_displayStoredConsoleMessages()

Can this be moved to a followup? This won't work right for windows with subframes. I don't see where messages for windows with no HUDs get stored in consoleStorage to begin with, is that in another patch?

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

> function namesAndValuesOf(aObject)
> {
>+  if (typeof aObject != "string") {
>+    aObject = XPCNativeWrapper.unwrap(aObject);
>+  }

Why this change?

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js

>   if (aSubject.arguments.length) {
>     for (let i = 0; i < aSubject.arguments.length; i++) {
>-      ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
>+      if (aSubject.arguments[i]) {
>+        ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
>+      }

This isn't a very useful check - it can never fail. Shouldn't you check that there's at least one argument?

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_580030_errors_after_page_reload.js
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_589162_css_filter.js

I don't understand the changes to these files - they seem to make the tests share less code.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js

>+  executeSoon(function (){
>+    finishTest();
>+  });

executeSoon(finishTest);

Why is it needed?

>+  var nodes = outputNode.querySelectorAll(".hud-msg-node");

>+  for (let i = 0; i < nodes.length; i++) {
>+    if (nodes[i].classList.contains("hud-filtered-by-string")) {
>+      foundCount ++;
>+    }
>+  }

This hasn't been updated per the last paragraph of comment 71.

>diff --git a/toolkit/components/console/hudservice/tests/browser/head.js b/toolkit/components/console/hudservice/tests/browser/head.js

>-  let msgs = aOutputNode.querySelectorAll(selector);
>+  let msgs = aOutputNode.querySelector(selector);
>   for (let i = 0, n = msgs.length; i < n; i++) {

This doesn't look right, querySelector returns a single element...

r- on this, but with these addressed it should be good to go.

Comment 78

[Mihai Sucan [:msucan]]

(In reply to comment #77)
> Comment on attachment 486770 [details] [diff] [review]
> 0.17 all tests pass
> 
> >-      hud = this.hudWeakReferences[hudId].get();
> >-      hud.reattachConsole(aContentWindow.top);
> >+      let _hud;
> >+      let hudWeakRef = this.hudWeakReferences[hudId];
> >+      if (hudWeakRef) {
> >+        _hud = hudWeakRef.get();
> >+        _hud.contentWindow = aContentWindow;
> >+        _hud.uriSpec = aContentWindow.location.href;
> >+      }
> >+      // TODO: name change?? doesn't actually re-attach the console
> >+      _hud.reattachConsole(aContentWindow.top);
> 
> This doesn't look right - if hudWeakRef is null, the call to .reattachConsole
> will throw. If it can't be null, there's no use null checking it.
> 
> I don't really understand why we need to call
> reattachConsole(aContentWindow.top); here at all. What breaks if you remove
> this?

Indeed, the hudWeakRef should never really be null. The code shouldn't check for that. If it's null, it really means something went very wrong and the weakref was GC'ed.

The call to reattachConsole() is needed to update a few properties in the _hud instance, including properties related to jsterm and evaluation. For example, jsterm evaluation would fail otherwise.

Comment 79

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #78)
> For example, jsterm evaluation would fail otherwise.

Fail in what cases? windowInitializer code is run for every window creation, including subframe windows. Why does the jsterm need to be re-initialized for every such window creation? I would imagine that only actually needs to happen for the top level window, so rather than always calling it and passing in aContentWindow.top, I think it should only be called when aContentWindow.top==aContentWindow.

Comment 80

[Mihai Sucan [:msucan]]

(In reply to comment #79)
> (In reply to comment #78)
> > For example, jsterm evaluation would fail otherwise.
> 
> Fail in what cases? windowInitializer code is run for every window creation,
> including subframe windows. Why does the jsterm need to be re-initialized for
> every such window creation? I would imagine that only actually needs to happen
> for the top level window, so rather than always calling it and passing in
> aContentWindow.top, I think it should only be called when
> aContentWindow.top==aContentWindow.

Good point. I agree, the call to reattachConsole() should only happen for aContentWindow.top.

Comment 81

[David Dahl :ddahl]

(In reply to comment #77)
> Comment on attachment 486770 [details] [diff] [review]
> >+  wrappedJSObject: this,
> 
> Why is this needed?

At one point, I may have thought it was needed and forgot to remove it.

> 
> >-  classID: Components.ID("{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}"),
> >+  classID: Components.ID("{d488614d-8721-445a-924c-34bdd1536d8e}"),
> 
> This is wrong - component IDs and interface IDs (IIDs) are completely separate,
> each should have its own unique ID. You only need to bump the interface ID, in
> this case (due to the interface change).

ok, asking a dumb question - I want to understand which ids are which. There is what appears to be a UUID in the component (classID), 2 uuids in the manifest:

component {b49c18f8-3379-4fc0-8c90-d7772c1a9ff3} ConsoleAPI.js
contract @mozilla.org/console-api;1 {b49c18f8-3379-4fc0-8c90-d7772c1a9ff3} 

and one in the IDL:

[scriptable, uuid(b49c18f8-3379-4fc0-8c90-d7772c1a9ff3)]

maybe there more? Not sure. What should each one be, and what are they?

> 
> >+  init: function CA_init(aWindow)
> 
> >+    // If aWindow is a frame's window use the top-most window instead
> >+    aWindow = aWindow.top;
> 
> I'm not sure why we want this behavior. It seems like we should report things
> to the console-log-event observer using the correct window/windowID, and then
> have it handle coalescing messages if desired. In other words, this .top should
> be able to move to CAO_observe.

I am doing this to simplify logging for iframes. The iframe will always log to the top window's web console. How would you implement this?

> 
> >diff --git a/toolkit/components/console/hudservice/ConsoleStorageService.js b/toolkit/components/console/hudservice/ConsoleStorageService.js
> 
> >   removeConsoleStorage: function CS_removeConsoleStorage(aId)
> 
> >+    else {
> >+      throw new Error("There is no consoleStorage with ID: " + aId);
> 
> Why this change?

Here is the rationale: https://bugzilla.mozilla.org/show_bug.cgi?id=568629#c61
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  getWindowId: function HS_getWindowId(aWindow)
> 
> I don't think there's any reason to expect this to throw, unless you pass it an
> invalid argument, so I don't think the try/catch is necessary.
> So this can just be:
> return
> aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
> 
> (I may be wrong about the throwing, in which case just add that back)
> 
ok.

> >+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
> 
> This can now use nsIDOMWindowUtils::getOuterWindowWithId, as in Patrick's patch
> for bug 597136.

That is sweet. nice.

> 
> >   registerDisplay: function HS_registerDisplay(aHUDId, aContentWindow)
> 
> >+    // get the window Id
> >+    var windowId = this.getWindowId(aContentWindow);
> >+    this._headsUpDisplays[aHUDId] = { id: aHUDId, windowId: windowId };
> 
> Doesn't this assume a 1->1 mapping between hudIDs and windowIDs? It's actually
> 1->many, with subframes taken into account... HUDs shouldn't really keep any
> reference to window IDs, IMO - instead use
> getHudIdByWindow(getWindowByWindowId(id)) in CAO_observe.

I'm not sure what you mean here.  Do you want me to only pass in the contentWindow to registerDisplay, then use getHudIdByWindow to get the windowId?

> 
> >-    // store this message in the storage module:
> >-    this.storage.recordEntry(aMessage.hudId, aMessage);
> 
> Why this change?

The this.storage is only going to be used with temporary filtering prefs. The new ConsoelStorageService will handle data storage. I imagine we should change its name to this.filterStorage or something.

> 
> >   logMessage: function HS_logMessage(aMessage, aConsoleNode, aMessageNode)
> 
> >     switch (aMessage.origin) {
> >       case "network":
> >       case "HUDConsole":
> >+      case "WebConsole":
> >       case "console-listener":
> >+      case "DOM Events":
> >+      case "DOM:HTML":
> >         this.logHUDMessage(aMessage, aConsoleNode, aMessageNode);
> 
> I don't understand these changes - should "WebConsole" be "Web Console", to
> match the change to evalInSandbox?

I think this is just a coincidence, the "Web Console" in evalInSandbox is not an origin property, but is a string that might be printed in the console as part of an exception message.  

> 
> >-    if (aContentWindow.document.location.href == "about:blank" &&
> >-        HUDWindowObserver.initialConsoleCreated == false) {
> >-      // TODO: need to make this work with about:blank in the future
> >-      // see bug 568661
> >-      return;
> >-    }
> 
> This removal seems to remove the only read of initialConsoleCreated. I don't
> really understand what the current code was trying to do, so I guess that's OK,
> but you should remove the other traces of it as well.

Will do.

> >+  // store the outerWindow Id
> >+  this.windowId = HUDService.getWindowId(aConfig.contentWindow);
> 
> See comment above about not keeping windowID references.

I created a getter to lazily fetch the windowID instead of storing it. 

> 
> >+  displayStoredConsoleMessages: function HUD_displayStoredConsoleMessages()
> 
> Can this be moved to a followup? This won't work right for windows with
> subframes. I don't see where messages for windows with no HUDs get stored in
> consoleStorage to begin with, is that in another patch?

yes. this is the only new function that uses the new storage service outside of a test.

will work on this and respond to further comments soon.

Comment 82

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #81)
> ok, asking a dumb question - I want to understand which ids are which.

No problem, it's kind of confusing. The manifest registers the component, so it should refer to the component ID ("classID" in ConsoleAPI.js) on both lines. So the component ID should be in three places - the component itself, and in the manifest (twice). The interface ID is only listed in one place (ConsoleAPI.idl), and should be different than the component ID. The interface ID needs to change when you change the interface, as you're doing with this patch.

> I am doing this to simplify logging for iframes. The iframe will always log to
> the top window's web console. How would you implement this?

Yeah, I'm not suggesting the behavior should change. I'm just suggesting that you should pass aWindow directly here (don't pass aWindow.top), and instead, in CAO_observe, where you call "getWindowByWindowId(), use getWindowByWindowId().top.

> I'm not sure what you mean here.  Do you want me to only pass in the
> contentWindow to registerDisplay, then use getHudIdByWindow to get the
> windowId?

Yeah, exactly.

Comment 83

[David Dahl :ddahl]

(In reply to comment #77)
> >diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm
> 
> > function namesAndValuesOf(aObject)
> > {
> >+  if (typeof aObject != "string") {
> >+    aObject = XPCNativeWrapper.unwrap(aObject);
> >+  }
> 
> Why this change?
we would crash calling pprint(window) if the window was xrayWrapped.:

call DumpJSStack():

0 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>
1 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>
2 namesAndValuesOf(aObject = [object XrayWrapper [object Window @ 0xac8f6900 (native @ 0xadc176d0)]]) ["resource://gre/modules/PropertyPanel.jsm":134]
    propName = undefined
    presentable = undefined
    value = undefined
    pairs = 
    this = [object BackstagePass @ 0xad9e4140 (native @ 0xb09ba0c4)]
3 JSTH_pprint(aObject = [object XrayWrapper [object Window @ 0xac8f6900 (native @ 0xadc176d0)]]) ["resource:///modules/HUDService.jsm":4063]
    pairs = undefined
    output = 
    this = [object BackstagePass @ 0xabe63bc0 (native @ 0xb09ba0c4)]
4 <TOP LEVEL> ["<unknown>":0]
    this = [object BackstagePass @ 0xabe63bc0 (native @ 0xb09ba0c4)]
5 <TOP LEVEL> ["Web Console":1]
    this = [object Window @ 0xac8f6900 (native @ 0xadc176d0)]

I filed a couple of bugs related to this kind of thing. bug 604422, bug 604760 I will file another one for this incident.

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js
> 
> >   if (aSubject.arguments.length) {
> >     for (let i = 0; i < aSubject.arguments.length; i++) {
> >-      ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
> >+      if (aSubject.arguments[i]) {
> >+        ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
> >+      }
> 
> This isn't a very useful check - it can never fail. Shouldn't you check that
> there's at least one argument?
indeed. I added this:

ok(aSubject.arguments.length > 0, "aSubject.arguments.length is 1 or more");

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_580030_errors_after_page_reload.js
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_589162_css_filter.js
> 
> I don't understand the changes to these files - they seem to make the tests
> share less code.

I was getting failures due to timing - "testLogEntry" would fail to work properly, when upon examination, the message sought was indeed logged to the console. So for these tests I had to just go with one-off querySelectorAll and checking the node.textContent.

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js
> 
> >+  executeSoon(function (){
> >+    finishTest();
> >+  });
> 
> executeSoon(finishTest);
> 
> Why is it needed?
I thought it was, but my latest testing says no.

> 
> >+  var nodes = outputNode.querySelectorAll(".hud-msg-node");
> 
> >+  for (let i = 0; i < nodes.length; i++) {
> >+    if (nodes[i].classList.contains("hud-filtered-by-string")) {
> >+      foundCount ++;
> >+    }
> >+  }
> 
> This hasn't been updated per the last paragraph of comment 71.

ah, yes. updated. what was I thinking?

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/head.js b/toolkit/components/console/hudservice/tests/browser/head.js
> 
> >-  let msgs = aOutputNode.querySelectorAll(selector);
> >+  let msgs = aOutputNode.querySelector(selector);
> >   for (let i = 0, n = msgs.length; i < n; i++) {
> 
> This doesn't look right, querySelector returns a single element...

weird. i fixed it.
> 
> r- on this, but with these addressed it should be good to go.

Comment 84

[David Dahl :ddahl]

(In reply to comment #82)
> > I'm not sure what you mean here.  Do you want me to only pass in the
> > contentWindow to registerDisplay, then use getHudIdByWindow to get the
> > windowId?
> 
> Yeah, exactly.

Unfortunately, getHudIdByWindow depends on the hudId being set in registerDisplay.

Comment 85

[David Dahl :ddahl]

A note to self, current failing tests:

INFO | runtests.py | Running tests: end.
mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_594477_clickable_output.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js | Page load was logged
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_network_panel.js | Test timed out
make: *** [mochitest-browser-chrome] Error 1

Comment 86

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #84)
> Unfortunately, getHudIdByWindow depends on the hudId being set in
> registerDisplay.

Ah, that's being fixed by bug 597136.

Comment 87

[David Dahl :ddahl]

Attachment: v 0.18 Review Comments addressed

Going to push this to try server. Review comments addressed.

Comment 88

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #83)
> I was getting failures due to timing - "testLogEntry" would fail to work
> properly, when upon examination, the message sought was indeed logged to the
> console. So for these tests I had to just go with one-off querySelectorAll and
> checking the node.textContent.

I don't really understand this. testLogEntry doesn't seem to do anything timing dependent, and boils down to just doing the same querySelectorAll, so these changes really shouldn't be necessary. If testLogEntry is broken we should fix it, rather than changing all of its callers to roll-their-own.

Comment 89

[Mihai Sucan [:msucan]]

Comment on attachment 488313 [details] [diff] [review]
v 0.18 Review Comments addressed

I looked through the code and I have the following comments:

+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
+  {
+    let someWindow = Services.ww.activeWindow;

activeWindow can be null, for example, when an alert() is (about to be) displayed.


+    let windowUtils = someWindow.getInterface(Ci.nsIDOMWindowUtils);
+    let content = windowUtils.getOuterWindowWithId(aId);
+    if (content) {
+      return content;
+    }
+    return null;

content can be null, if no window is found with the given ID. So, I think you can just do:

return windowUtils.getOuterWindowWithId(aId);


In function HS_logMessage():

       case "console-listener":
+      case "DOM Events":
+      case "DOM:HTML":

These are not needed. This is fallout from bug 595934.


In browser_ConsoleAPITests.js: you add testLogEntry() but you also have it in head.js. Why? I know that the iframe test failed *because* of that. To make it run fine, I only had to remove testLogEntry() from the iframe test.

In browser_webconsole_bug_580030_errors_after_page_reload.js: the change you made requires that the fooBazBaz exception is *always* shown as the second message in the console output. This is why I preferred testLogEntry(). We could get intermittent failures when, say, the browser also requests the favicon.

(the same applies to the similar changes in the other tests)

In browser_webconsole_bug_594477_clickable_output.js:

-  let hudId = HUDService.getHudIdByWindow(content);
+  let hudId = HUDService.getHudIdByWindow(browser.contentWindow);

Isn't that the same?

Comment 90

[David Dahl :ddahl]

(In reply to comment #88)
> I don't really understand this. testLogEntry doesn't seem to do anything timing
> dependent, and boils down to just doing the same querySelectorAll, so these
> changes really shouldn't be necessary. If testLogEntry is broken we should fix
> it, rather than changing all of its callers to roll-their-own.

I wasn't changing all of the callers or suggesting we change them all, I just happened to find that the tests where I did change to a unique solution worked whereas testLogEntry failed. I think I am doing this 3 or 4 times.

I should file a followup bug to make testLogEntry work for every test.

Comment 91

[David Dahl :ddahl]

(In reply to comment #89)
> Comment on attachment 488313 [details] [diff] [review]
> v 0.18 Review Comments addressed
> 
> I looked through the code and I have the following comments:
> 
> +  getWindowByWindowId: function HS_getWindowByWindowId(aId)
> +  {
> +    let someWindow = Services.ww.activeWindow;
> 
> activeWindow can be null, for example, when an alert() is (about to be)
> displayed.
> 
> 
> +    let windowUtils = someWindow.getInterface(Ci.nsIDOMWindowUtils);
> +    let content = windowUtils.getOuterWindowWithId(aId);
> +    if (content) {
> +      return content;
> +    }
> +    return null;
> 
> content can be null, if no window is found with the given ID. So, I think you
> can just do:
> 
> return windowUtils.getOuterWindowWithId(aId);

OK.

> 
> 
> In function HS_logMessage():
> 
>        case "console-listener":
> +      case "DOM Events":
> +      case "DOM:HTML":
> 
> These are not needed. This is fallout from bug 595934.
> 
ah, OK. will remove.

> 
> In browser_ConsoleAPITests.js: you add testLogEntry() but you also have it in
> head.js. Why? I know that the iframe test failed *because* of that. To make it
> run fine, I only had to remove testLogEntry() from the iframe test.
> 
> In browser_webconsole_bug_580030_errors_after_page_reload.js: the change you
> made requires that the fooBazBaz exception is *always* shown as the second
> message in the console output. This is why I preferred testLogEntry().

As do I, but testLogEntry just would not work for me in this test. Failed every time. I will re-visit this problem today.

> 
> In browser_webconsole_bug_594477_clickable_output.js:
> 
> -  let hudId = HUDService.getHudIdByWindow(content);
> +  let hudId = HUDService.getHudIdByWindow(browser.contentWindow);
> 
> Isn't that the same?

Perhaps it is, I was not 100% sure at the time of making this change. It is more explicit (to me).

Thank you.

Comment 92

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488313 [details] [diff] [review]
v 0.18 Review Comments addressed

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.idl b/toolkit/components/console/hudservice/ConsoleAPI.idl

Do we have a bug on file for the better solution to multiple argument stuff?

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.js b/toolkit/components/console/hudservice/ConsoleAPI.js

>+  init: function CA_init(aWindow)

>+    // If aWindow is a frame's window use the top-most window instead
>+    aWindow = aWindow.top;

This shouldn't be necessary now (and is in fact wrong since it will prevent the listener from differentiating subframe messages).

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  getWindowByWindowId: function HS_getWindowByWindowId(aId)
>+  {
>+    let someWindow = Services.ww.activeWindow;

See discussion starting at bug 597136 comment 41 - should use Services.wm.getMostRecentWindow(null) here, and handle it being null. We should also make use of this method in the code being added by that patch.

>   /**
>    * Register a new Heads Up Display
>    *
>-   * @param string aHUDId
>    * @param nsIDOMWindow aContentWindow
>    * @returns void
>    */
>-  registerDisplay: function HS_registerDisplay(aHUDId, aContentWindow)
>+  registerDisplay: function HS_registerDisplay(aHudId, aContentWindow)
>   {
>     // register a display DOM node Id and HUD uriSpec with the service
> 
>-    if (!aHUDId || !aContentWindow){
>+    if (!aContentWindow){

These changes all seem gratuitous.

>+    // get the window Id
>+    var windowId = this.getWindowId(aContentWindow);
>+    this._headsUpDisplays[aHudId] = { id: aHudId, windowId: windowId };

As far as I can tell, this windowId is never used

>-      hud = this.hudWeakReferences[hudId].get();
>-      hud.reattachConsole(aContentWindow.top);
>+      let _hud;
>+      let hudWeakRef = this.hudWeakReferences[hudId];
>+      if (hudWeakRef) {
>+        _hud = hudWeakRef.get();
>+        _hud.contentWindow = aContentWindow;
>+        _hud.uriSpec = aContentWindow.location.href;
>+      }

There should be no null check here, per comment 78.

> HeadsUpDisplay.prototype = {

>+  get windowId()
>+  {
>+    return HUDService.getWindowId(this.contentWindow);
>+  },

Is this used anywhere? Only use of ".windowId" that I see seems to be the one set in registerDisplay above.

>+let ConsoleAPIObserver = {

>+  observe: function CAO_observe(aMessage, aTopic, aData)
>+  {
>+    if (aTopic == "console-api-log-event") {

As mentioned earlier, this method should use getHudIdByWindow(getWindowByWindowId(ID)) once bug 597136 is landed. If that doesn't happen before this lands, we need to make sure it gets a followup.

>+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)

And this method seems like it reaches into HUDService/aWebConsole a bit too much - seems like there should be a HUDService.logConsoleMessage(aHudID, aLevel, aArguments) that contains most of this code (the existing logConsoleMessage seems unused).

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

> function namesAndValuesOf(aObject)
> {
>+  if (typeof aObject != "string") {
>+    aObject = XPCNativeWrapper.unwrap(aObject);
>+  }

This seems to be obsoleted by the patch in bug 608358 (and is unrelated to this bug either way, right?).

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js

>+function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj)
>+{
>+  var msgs = aOutputNode.querySelectorAll(".hud-msg-node");
>+  for (var i = 1; i < msgs.length; i++) {

Why start at index 1?

>+function testOpenUI()
>+{
>+  // test to see if the messages are
>+  // displayed when the console UI is opened

Where is this functionality implemented? I'm a bit confused, because I thought that was in an earlier version of this patch and then removed, so I would expect this to be failing...

Comment 93

[David Dahl :ddahl]

(In reply to comment #92)
> Do we have a bug on file for the better solution to multiple argument stuff?
> 
I was using the original bug, but will file a followup

> >+    // get the window Id
> >+    var windowId = this.getWindowId(aContentWindow);
> >+    this._headsUpDisplays[aHudId] = { id: aHudId, windowId: windowId };
> 
> As far as I can tell, this windowId is never used

It is used in CAO_observe

> > HeadsUpDisplay.prototype = {
> 
> >+  get windowId()
> >+  {
> >+    return HUDService.getWindowId(this.contentWindow);
> >+  },
> 
> Is this used anywhere? Only use of ".windowId" that I see seems to be the one
> set in registerDisplay above.
>
Good catch, looks like it is not used anymore
 
> >+let ConsoleAPIObserver = {
> 
> >+  observe: function CAO_observe(aMessage, aTopic, aData)
> >+  {
> >+    if (aTopic == "console-api-log-event") {
> 
> As mentioned earlier, this method should use
> getHudIdByWindow(getWindowByWindowId(ID)) once bug 597136 is landed. If that
> doesn't happen before this lands, we need to make sure it gets a followup.
> 

filed bug 609950

> >+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)
> 
> And this method seems like it reaches into HUDService/aWebConsole a bit too
> much - seems like there should be a HUDService.logConsoleMessage(aHudID,
> aLevel, aArguments) that contains most of this code (the existing
> logConsoleMessage seems unused).

I will file a followup (bug 609952)
> 
> >diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm
> 
> > function namesAndValuesOf(aObject)
> > {
> >+  if (typeof aObject != "string") {
> >+    aObject = XPCNativeWrapper.unwrap(aObject);
> >+  }
> 
> This seems to be obsoleted by the patch in bug 608358 (and is unrelated to this
> bug either way, right?).

True, I already removed it as per robcee's advice.
> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js
> 
> >+function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj)
> >+{
> >+  var msgs = aOutputNode.querySelectorAll(".hud-msg-node");
> >+  for (var i = 1; i < msgs.length; i++) {
> 
> Why start at index 1?

Wow. that is broken. No idea why that would be. That function will be removed anyway in favor of the one in head.js.

> 
> >+function testOpenUI()
> >+{
> >+  // test to see if the messages are
> >+  // displayed when the console UI is opened
> 
> Where is this functionality implemented? I'm a bit confused, because I thought
> that was in an earlier version of this patch and then removed, so I would
> expect this to be failing...

This will be removed as it is part of a followup bug.

Comment 94

[David Dahl :ddahl]

(In reply to comment #93)
> (In reply to comment #92)
> > Do we have a bug on file for the better solution to multiple argument stuff?
> > 
> I was using the original bug, but will file a followup

see bug 609955

Comment 95

[David Dahl :ddahl]

Attachment: v 0.19 Comments addressed

Saving work, I may still a have a tweak or two

Comment 96

[David Dahl :ddahl]

Comment on attachment 488648 [details] [diff] [review]
v 0.19 Comments addressed

Just ran it through try server cleanly

Comment 97

[Mihai Sucan [:msucan]]

(In reply to comment #92)
> Comment on attachment 488313 [details] [diff] [review]
> v 0.18 Review Comments addressed
> 
> >+function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj)
> >+{
> >+  var msgs = aOutputNode.querySelectorAll(".hud-msg-node");
> >+  for (var i = 1; i < msgs.length; i++) {
> 
> Why start at index 1?

This looks like fallout from the times when we had a splitter between message nodes, one in each group, as the first element in the group.

Comment 98

[David Dahl :ddahl]

(In reply to comment #97)
> This looks like fallout from the times when we had a splitter between message
> nodes, one in each group, as the first element in the group.

I have been doing the query like: querySelctorAll(".hud-msg-node")

as it is much simpler.

Comment 99

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488648 [details] [diff] [review]
v 0.19 Comments addressed

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+XPCOMUtils.defineLazyServiceGetter(this, "consoleStorage",
>+                                   "@mozilla.org/console-storage-service;1",
>+                                   "nsIConsoleStorageService");

>-    // store this message in the storage module:
>-    this.storage.recordEntry(aMessage.hudId, aMessage);

These changes don't look related to this bug, so better to revert them I think.

>+function pprint(aObj)

Probably not worth adding this either.

>-      hud = this.hudWeakReferences[hudId].get();
>-      hud.reattachConsole(aContentWindow.top);
>+      let _hud;
>+      let hudWeakRef = this.hudWeakReferences[hudId];
>+      _hud = hudWeakRef.get();

Don't really need the hudWeakRef variable. And you can rework things so that "hud" is only declared separately in both blocks, since it isn't used outside of them.

>+      _hud.contentWindow = aContentWindow;
>+      _hud.uriSpec = aContentWindow.location.href;

These additions look wrong - you don't want to change a hud's window/uriSpec when a subframe window is created.

> HeadsUpDisplay.prototype = {
>+
>+  get windowId()
>+  {
>+    return HUDService.getWindowId(this.contentWindow);
>+  },

This one is still unused.

>+let ConsoleAPIObserver = {

>+  QueryInterface: XPCOMUtils.generateQI(
>+    [Ci.nsIObserver,]
>+  ),

nit: just put this on a single line (omit trailing comma)

>+  observe: function CAO_observe(aMessage, aTopic, aData)

>+      // get the window from aData, which is the "Window.ID"

This comment is misplaced (should be above), but I think you can just remove it.

>-    if (this.activityObject.category == "CSS Parser") {
>-      this.messageNode.classList.add("hud-cssparser");
>+    if (this.activityObject.category) {
>+      if (this.activityObject.category == "CSS Parser") {
>+        this.messageNode.classList.add("hud-cssparser");
>+      }
>     }

This change doesn't look useful.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js

>+      if (aSubject.arguments[i]) {
>+        ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
>+      }

This is still a useless check - the OK will never fail given that it only runs after you've null checked its argument. If you really want to print out the arguments, use info(), but I think you should just remove this.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js

> function tab1Reloaded(aEvent) {

>-  let hudId1 = HUDService.getHudIdByWindow(tab1.linkedBrowser.contentWindow);
>+  let hudId1 = HUDService.getHudIdByWindow(tab1.linkedBrowser.contentWindow.top);

This change isn't needed, browser.contentWindow can only point to a top-level window.

>+  let nodes = outputNode1.querySelectorAll(".hud-msg-node");

This doesn't appear to be used.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js

>+  ok(nodes.length == 1, "1 hidden " + aMethod  + " node found (via classList)");
>+  ok(nodes.length == 0, aMethod + " logging turned off, 1 message hidden");

You should continue using is() for these checks.

Comment 100

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488648 [details] [diff] [review]
v 0.19 Comments addressed

r- because of the windowInitializer changes, but with just the changes described in comment 99 this should be good to land.

Comment 101

[David Dahl :ddahl]

(In reply to comment #99)
> Comment on attachment 488648 [details] [diff] [review]
> v 0.19 Comments addressed
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+XPCOMUtils.defineLazyServiceGetter(this, "consoleStorage",
> >+                                   "@mozilla.org/console-storage-service;1",
> >+                                   "nsIConsoleStorageService");
> 
> >-    // store this message in the storage module:
> >-    this.storage.recordEntry(aMessage.hudId, aMessage);
> 
> These changes don't look related to this bug, so better to revert them I think.

The recordEntry call can be removed as we don't actually do anything with that data, we just keep filling an object with records.

> 
> >+function pprint(aObj)
> 
> Probably not worth adding this either.

ah, yes. debug helper i forgot to remove.
> 
> >-      hud = this.hudWeakReferences[hudId].get();
> >-      hud.reattachConsole(aContentWindow.top);
> >+      let _hud;
> >+      let hudWeakRef = this.hudWeakReferences[hudId];
> >+      _hud = hudWeakRef.get();
> 
> Don't really need the hudWeakRef variable. And you can rework things so that
> "hud" is only declared separately in both blocks, since it isn't used outside
> of them.
> 
> >+      _hud.contentWindow = aContentWindow;
> >+      _hud.uriSpec = aContentWindow.location.href;
> 
> These additions look wrong - you don't want to change a hud's window/uriSpec
> when a subframe window is created.

I thought we would need to update these properties if you re-use the same console UI but visit a new url. As far as not updating for a subframe - should I just check to make sure aContentWindow == aContentWindow.top before updating?

> 
> >-    if (this.activityObject.category == "CSS Parser") {
> >-      this.messageNode.classList.add("hud-cssparser");
> >+    if (this.activityObject.category) {
> >+      if (this.activityObject.category == "CSS Parser") {
> >+        this.messageNode.classList.add("hud-cssparser");
> >+      }
> >     }
> 
> This change doesn't look useful.
I have run into cases where the category is null in the activityObject and this throws.

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPITests.js
> 
> >+      if (aSubject.arguments[i]) {
> >+        ok(aSubject.arguments[i], "arg: " + aSubject.arguments[i]);
> >+      }
> 
> This is still a useless check - the OK will never fail given that it only runs
> after you've null checked its argument. If you really want to print out the
> arguments, use info(), but I think you should just remove this.
ok

> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js
> 
> > function tab1Reloaded(aEvent) {
> 
> >-  let hudId1 = HUDService.getHudIdByWindow(tab1.linkedBrowser.contentWindow);
> >+  let hudId1 = HUDService.getHudIdByWindow(tab1.linkedBrowser.contentWindow.top);
> 
> This change isn't needed, browser.contentWindow can only point to a top-level
> window.
> 
good to know.

> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_console_logging_api.js
> 
> >+  ok(nodes.length == 1, "1 hidden " + aMethod  + " node found (via classList)");
> >+  ok(nodes.length == 0, aMethod + " logging turned off, 1 message hidden");
> 
> You should continue using is() for these checks.
ok:)

Why the preference for is over ok?

Comment 102

[David Dahl :ddahl]

Attachment: v 0.20 Comments adressed

Comment 103

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #101)
> The recordEntry call can be removed as we don't actually do anything with that
> data, we just keep filling an object with records.

That's fine, but I'd rather not roll that change into this bug since this patch is large enough as it is, and landing a bunch of unrelated changes in a single patch leads to confusion. There's also another call to recordEntry, so it doesn't make sense to do this piecemeal.

> I thought we would need to update these properties if you re-use the same
> console UI but visit a new url. As far as not updating for a subframe - should
> I just check to make sure aContentWindow == aContentWindow.top before updating?

You shouldn't need to update contentWindow (outer window). I suppose you may need to update the URI assuming content-document-global-created is called for inner window creation, but that's also not related to this bug, and we have other bugs that cover it (bug 594523).

> > >+    if (this.activityObject.category) {
> > >+      if (this.activityObject.category == "CSS Parser") {
> > >+        this.messageNode.classList.add("hud-cssparser");
> > >+      }
> > >     }
> > 
> > This change doesn't look useful.
> I have run into cases where the category is null in the activityObject and this
> throws.

category being null wouldn't cause that to throw, since you're not accessing a property on it. It just compares null to "CSS Parser", which isn't a problem.

> Why the preference for is over ok?

The failure case message is clearer (tells you "got foo, expected bar").

Comment 104

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488918 [details] [diff] [review]
v 0.20 Comments adressed

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+      hud.contentWindow = aContentWindow;
>+      hud.uriSpec = aContentWindow.location.href;

Let's not add these lines, per comment 103.

>-    // TODO: need to replace these DOM calls with internal functions
>+    // TODO: need to re!place these DOM calls with internal functions

oops :)

>+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)

>+    // check to see if logging is on for this level before logging!
>+    var filterState = HUDService.getFilterState(aWebConsole.hudId, aLevel);
>+    // if (!filterState) {
>+    //   // Ignoring log message
>+    //   return;
>+    // }

Why is this now commented out?

With these addressed and the activityObject.category change and recordEntry removal undone per comment 103, I think this is good to go.

Comment 105

[David Dahl :ddahl]

(In reply to comment #104)
> Comment on attachment 488918 [details] [diff] [review]
> v 0.20 Comments adressed
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+      hud.contentWindow = aContentWindow;
> >+      hud.uriSpec = aContentWindow.location.href;
> 
> Let's not add these lines, per comment 103.

ok

> 
> >+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)
> 
> >+    // check to see if logging is on for this level before logging!
> >+    var filterState = HUDService.getFilterState(aWebConsole.hudId, aLevel);
> >+    // if (!filterState) {
> >+    //   // Ignoring log message
> >+    //   return;
> >+    // }
> 
> Why is this now commented out?
The filter tests all fail with this enabled - I meant to remove it. This is an artifact from before one of the filtering patches that landed where classes are added to hide filtered log output. With this code enabled, we never get back 

> 
> With these addressed and the activityObject.category change and recordEntry
> removal undone per comment 103, I think this is good to go.

ok, uploading new patch now.

Comment 106

[David Dahl :ddahl]

Attachment: v 0.21 Comments addressed

Comment 107

[David Dahl :ddahl]

Attachment: ddahl-integration-patch

Comment 108

[David Dahl :ddahl]

Attachment: v 0.22 Lazy Console Integration

All tests pass

Comment 109

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 490215 [details] [diff] [review]
v 0.22 Lazy Console Integration

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+const CLASS_ID_VALUE = "{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}";

CONSOLEAPI_CLASS_ID?

>+  getWindowByWindowId: function HS_getWindowByWindowId(aId)

>+    let content = windowUtils.getOuterWindowWithId(aId);
>+    if (content) {
>+      return content;
>+    }
>+    return null;

you can just |return windowUtils.getOuterWindowWithId(aId);|

>   /**
>    * Register a new Heads Up Display
>    *
>-   * @param string aHUDId

>-    if (!aHUDId || !aContentWindow){
>+    if (!aContentWindow){
>       throw new Error(ERRORS.MISSING_ARGS);
>     }

Don't really see any reason for these changes (parameter still exists).

>   logMessage: function HS_logMessage(aMessage, aConsoleNode, aMessageNode)

>     switch (aMessage.origin) {
>       case "network":
>       case "HUDConsole":
>+      case "WebConsole":

Seems like we can remove "HUDConsole", and also generateConsoleMessage() while we're at it.

>+      hud.contentWindow = aContentWindow;
>+      hud.uriSpec = aContentWindow.location.href;

Looks like you forgot to remove these per discussion above.

>+    // need to detect that the console component has been paved over
>+    if (!(aContentWindow.wrappedJSObject.console.classID == CLASS_ID_VALUE)) {
>       this.logWarningAboutReplacedAPI(hudId);
>     }

Hmm, this check can be fooled by sites replacing window.console (they just need to set a classID property with the right value, and we won't show the warning). I think you might be able to avoid that by using this instead:

let consoleObject = XPCNativeWrapper.unwrap(aContentWindow).console;
if (!(consoleObject instanceof Ci.nsISupports)) { // or nsIDOMGlobalPropertyInitializer ?
  
}

>+let ConsoleAPIObserver = {

>+  function CAO_sendToWebConsole(aWebConsole, aLevel, aArguments)

>+    // check to see if logging is on for this level before logging!
>+    var filterState = HUDService.getFilterState(aWebConsole.hudId, aLevel);

This now looks unused.

>diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in

>+	browser_webconsole_bug_593003_iframe_wrong_hud.js \

> # compartment-disabled
> #	browser_webconsole_bug_593003_iframe_wrong_hud.js \

remove the commented one

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_601352_scroll.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_601352_scroll.js

>+    HUD = longMessage = hudId = node = rectNode = rectOutput = height = top = bottom = null;

This shouldn't be needed.

>diff --git a/toolkit/components/console/hudservice/tests/browser/head.js b/toolkit/components/console/hudservice/tests/browser/head.js

>+  // let selector = ".hud-group > *";
>+  let selector = ".hud-msg-node";

remove commented version

Comment 110

[David Dahl :ddahl]

Attachment: v 0.23 Inegration Patch

All comments addressed and followup bug filed

Comment 111

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 490916 [details] [diff] [review]
v 0.23 Inegration Patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>     else {
>       hud = this.hudWeakReferences[hudId].get();
>-      hud.reattachConsole(aContentWindow.top);
>+      // TODO: change how we detect our console: bug 612405
>+      if (!(aContentWindow.wrappedJSObject.console.classID == CONSOLEAPI_CLASS_ID)) {
>+        // TODO: name change?? doesn't actually re-attach the console
>+        hud.reattachConsole(aContentWindow.top);
>+      }


>-    // Check if aContentWindow has a console object. If so, don't attach
>-    // our console, but warn the user about this.
>+    // need to detect that the console component has been paved over
>     if (aContentWindow.wrappedJSObject.console) {
>       this.logWarningAboutReplacedAPI(hudId);
>     }

This looks like it got messed up - the CONSOLEAPI_CLASS_ID check should be around the logWarningAboutReplacedAPI call, and you need the window.top==window check around the reattachConsole call, as in previous iterations of this patch.

r=me with that fixed up.

Comment 112

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/aafeff43ea25
http://hg.mozilla.org/mozilla-central/rev/19e98c731909