851231 - Output console.jsm API calls to the browser console

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

With the new Browser Console landing soon we want to better interact with the Console.jsm messages. We should output all calls to the browser console, in addition to dumping into the terminal as plaintext.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: first cut

Changes for Console.jsm to make it output to the Browser Console. I also added support for the time() and timeEnd() methods.

Remaining things to do:

- add comments.
- do we have Console.jsm tests?
- potentially use ConsoleAPIStorage.jsm to keep API calls in memory, so they don't get lost if the Browser Console is not open. My worry is that we currently group console API calls by window IDs, and jsm's have no window ID, and I need to check when we can purge objects from the storage. Should we do it in this patch?


Thoughts?

Comment 2

[Joe Walker [:jwalker]]

(In reply to Mihai Sucan [:msucan] from comment #1)
> Created attachment 725052 [details] [diff] [review]
> first cut
> 
> Changes for Console.jsm to make it output to the Browser Console. I also
> added support for the time() and timeEnd() methods.
> 
> Remaining things to do:
> 
> - add comments.
> - do we have Console.jsm tests?

No, it started as a quick hack and has never been done properly.

> - potentially use ConsoleAPIStorage.jsm to keep API calls in memory, so they
> don't get lost if the Browser Console is not open. My worry is that we
> currently group console API calls by window IDs, and jsm's have no window
> ID, and I need to check when we can purge objects from the storage. Should
> we do it in this patch?

I'd be happy to do it as a followup

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 725052 [details] [diff] [review]
first cut

Review of attachment 725052 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great so far

Comment 4

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Cleaned-up the patch, added comments, made some bug fixes and added a test.

The test uses the new waitForMessages() helper, which I still add features to. Trying to keep actual tests as simple as possible. Suggestions are welcome.

Caching in ConsoleAPIStorage.jsm should be a follow up. I will open a bug report about that.

Thank you!

Comment 5

[Mihai Sucan [:msucan]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2b63f2db523c

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 736896 [details] [diff] [review]
proposed patch

Review of attachment 736896 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/Console.jsm
@@ +317,5 @@
> + *         An array of {filename, lineNumber, functionName, language} objects.
> + *         These objects follow the same format as other console-api-log-event
> + *         messages.
> + */
> +function getStackForConsoleAPI(aFrame, aMaxDepth = 0) {

I don't think we need both getStackForConsoleAPI and getStack do we? Can't we make callers of getStack use the data structure returned by getStackForConsoleAPI ?

Comment 7

[Mihai Sucan [:msucan]]

Attachment: updated patch

Thanks for the review. Yes, it makes sense to remove the old getStack(). I actually renamed getStackForConsoleAPI() into getStack(), since we don't keep both.

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/2cecd940c219

(try push was green)

Comment 8

[Mihai Sucan [:msucan]]

It would be nice if there would be some MDN page where devs are told about this Console.jsm and that extension authors can Cu.import it. Using the console API with the Browser Console is much nicer than alert() or dump().

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

For devtools bugs marked dev-doc-needed we should also cc Will Bamberg just to make the bug a little more visible for him.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2cecd940c219