add a way to log to web console from toolbox

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: Framework
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

Trunk
Firefox 56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
For bug 1345533, we need a way for the toolbox to be able to log something
to the web console.  I'm splitting this out to a separate bug so I can
make some incremental progress.
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8891009 [details]
Bug 1385032 - add logErrorInPage to tab target in devtools;

https://reviewboard.mozilla.org/r/162184/#review167486

::: devtools/client/framework/target.js:722
(Diff revision 1)
> +   * Log a warning message to the tab's console.
> +   *
> +   * @param {String} text
> +   *                 The text to log.
> +   */
> +  logMessage: function (text) {

We should also add a `logMessage` function for WorkerTarget - at least to prevent errors (it could be a noop if we don't need it to do anything)

Also, I'm thinking we should rename "logMessage" throughout to something more specific. Any ideas? I thought of "logErrorInPage, "logErrorInDebuggee", "logErrorInServer"

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_logmessage.js:16
(Diff revision 1)
> +
> +add_task(async function () {
> +  const hud = await openNewTabAndConsole(TEST_URI);
> +  const toolbox = hud.ui.newConsoleOutput.toolbox;
> +
> +  const receivedMessages = waitForMessages({

You can drop the waitForMessages call / `await receivedMessages` since you are doing `await waitFor(() => findMessage(...)` instead

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_logmessage.js:23
(Diff revision 1)
> +    messages: [
> +      { text: "beware the octopus" },
> +    ]
> +  });
> +
> +  toolbox._target.logMessage("beware the octopus");

Nit: there's a `toolbox.target` getter

::: devtools/server/actors/tab.js:671
(Diff revision 1)
> +  onLogMessage(request) {
> +    let text = request.text;
> +    let scriptErrorClass = Cc["@mozilla.org/scripterror;1"];
> +    let scriptError = scriptErrorClass.createInstance(Ci.nsIScriptError);
> +    scriptError.initWithWindowID(text, null, null, 0, 0, 1,
> +                                 "content javascript", getInnerId(this.window));

It would be helpful if we could pass a custom category with the request instead of using "content javascript", so that we can create Learn More links, etc.

This would allow us to add a new "Source Map Load Error" category that has a link to an MDN page with tips for fixing the problem, like so: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/errordocs.js#95.  It appears that we'd also want to update documentation for new categories here (and also make sure that the list doesn't have unused ones): https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIScriptError#Categories_the_Web_Console_displays.
Attachment #8891009 - Flags: review?(bgrinstead)
(Assignee)

Comment 3

7 months ago
> This would allow us to add a new "Source Map Load Error" category that has a link to an MDN page with tips for fixing the problem

Great idea.  I filed bug 1385337 for the MDN page; and maybe I can land the
change to errordocs.js there as well (or file another followup).
(Assignee)

Comment 4

7 months ago
> We should also add a `logMessage` function for WorkerTarget - at least to prevent errors (it could be a noop if we don't need it to do anything)

I'm going to make this a no-op for now and file a followup bug to deal with workers --
I think source maps and workers need a few fixes and more investigation on my part.
(Assignee)

Comment 5

7 months ago
(In reply to Tom Tromey :tromey from comment #4)

> I'm going to make this a no-op for now and file a followup bug to deal with
> workers --
> I think source maps and workers need a few fixes and more investigation on
> my part.

I'm going to add a note to bug 1368680 to deal with the worker bits.
(Assignee)

Comment 6

7 months ago
mozreview-review-reply
Comment on attachment 8891009 [details]
Bug 1385032 - add logErrorInPage to tab target in devtools;

https://reviewboard.mozilla.org/r/162184/#review167486

> We should also add a `logMessage` function for WorkerTarget - at least to prevent errors (it could be a noop if we don't need it to do anything)
> 
> Also, I'm thinking we should rename "logMessage" throughout to something more specific. Any ideas? I thought of "logErrorInPage, "logErrorInDebuggee", "logErrorInServer"

I went with logErrorInPage.
Comment hidden (mozreview-request)

Comment 8

7 months ago
mozreview-review
Comment on attachment 8891009 [details]
Bug 1385032 - add logErrorInPage to tab target in devtools;

https://reviewboard.mozilla.org/r/162184/#review167928

Thanks, looks good
Attachment #8891009 - Flags: review?(bgrinstead) → review+

Comment 9

7 months ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bfd30365fa0b
add logErrorInPage to tab target in devtools; r=bgrins

Comment 10

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bfd30365fa0b
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.