Closed Bug 1438482 Opened 6 years ago Closed 6 years ago

logErrorInPage should produce an error log

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

Bug 1438482 - logErrorInPage should produce an error log;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
I am seeing just one log with no repeat number when executing the following:

toolbox.target.logErrorInPage("mymsg", "mycat");
toolbox.target.logErrorInPage("mymsg", "mycat");

(yes, twice the same)

Also, the icon in front of the message is orange exclamation mark indicating a warning not error. `logErrorInPage` is supposed to produce an error, correct?

Honza
Assignee: nobody → odvarko
@Nicolas, what about the icon? Should we display an error icon instead?

Honza
Status: NEW → ASSIGNED
Flags: needinfo?(nchevobbe)
The repeat number should be fixed in bug 1405245.
So, keep this one for the icon.

I think that `logErrorInPage` should produce an error log and so, the patch is using
error flag for `scriptError.initWithWindowID`.

Honza
Summary: Missing repeat number for logs made by `logErrorInPage` → logErrorInPage should produce an error log
Whiteboard: [reserve-console-html]
(In reply to Jan Honza Odvarko [:Honza] from comment #0)

> Also, the icon in front of the message is orange exclamation mark indicating
> a warning not error. `logErrorInPage` is supposed to produce an error,
> correct?

See bug 1385032 for where this was added.
It was originally added to let us log source map problems.
I don't know which icon is better here, but I do think it might be nicer for users
if you also updated the screenshots on MDN when changing this.
See https://developer.mozilla.org/en-US/docs/Tools/Debugger/Source_map_errors
(In reply to Tom Tromey :tromey from comment #5)
> See bug 1385032 for where this was added.
Thanks Tom!

Since source map warning is really rather a warning and should stay as is I introduced `logWarningInPage`, which is now used to log warnings and made `logErrorInPage` to produce real error logs.

I was yet thinking about back-comp:

  logWarningInPage: function (text, category) {
    if (this.activeTab && this.activeTab.traits.logWarningInPage) {
      let packet = {
        to: this.form.actor,
        type: "logWarningInPage",
        text,
        category,
      };
      this.client.request(packet);
    } else {
      // For backward compatibility with back-end
      // not supporting `logWarningInPage` yet.
      this.logErrorInPage(text, category);
    }
  },

Does that sound good?

Honza
Flags: needinfo?(nchevobbe)
Comment on attachment 8951221 [details]
Bug 1438482 - logErrorInPage should produce an error log;

https://reviewboard.mozilla.org/r/220486/#review226916

::: devtools/client/framework/target.js:744
(Diff revision 4)
> +   * @param {String} text
> +   *                 The text to log.
> +   * @param {String} category
> +   *                 The category of the message.  @see nsIScriptError.
> +   */
> +  logWarningInPage: function (text, category) {

I like splitting these two functions out at the target level. But looking at the server side, I wonder if we could instead make a single server method `logInPage` that takes `flags` in (kind of like we take `category` in here). I just imagine we would want to have a `logInfoInPage` in the future, and doing it this way would avoid some extra boilerplate / bloat on the TabActor side.

I really don't feel strongly about this so feel free to re-request after making the test change below if you don't want to do it this way. But it does seem more consistent with how we handle category.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_logWarningInPage.js:18
(Diff revision 4)
> +  const hud = await openNewTabAndConsole(TEST_URI);
> +  const toolbox = hud.ui.newConsoleOutput.toolbox;
> +
> +  toolbox.target.logWarningInPage("beware the octopus", "content javascript");
> +
> +  const node = await waitFor(() => findMessage(hud, "octopus"));

Can we assert that this has the proper severity (warning in this case, and error in browser_webconsole_logErrorInPage.js)
Attachment #8951221 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #9)
> I just imagine we would want to have a `logInfoInPage` in the future, and
> doing it this way would avoid some extra boilerplate / bloat on the TabActor
> side.
Done

> Can we assert that this has the proper severity (warning in this case, and
> error in browser_webconsole_logErrorInPage.js)
Done

What about the back-comp?

Thanks,
Honza
Comment on attachment 8951221 [details]
Bug 1438482 - logErrorInPage should produce an error log;

https://reviewboard.mozilla.org/r/220486/#review227194


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/tab.js:667
(Diff revision 5)
> -    let {text, category} = request;
> +    let {text, category, flags} = request;
>      let scriptErrorClass = Cc["@mozilla.org/scripterror;1"];
>      let scriptError = scriptErrorClass.createInstance(Ci.nsIScriptError);
> -    scriptError.initWithWindowID(text, null, null, 0, 0, 1,
> +    scriptError.initWithWindowID(text, null, null, 0, 0, flags,
>                                   category, getInnerId(this.window));
> -    Services.console.logMessage(scriptError);
> +    let console = Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService);

Error: Use services.console rather than getservice(). [eslint: mozilla/use-services]
Priority: -- → P1
Whiteboard: [reserve-console-html] → [newconsole-mvp]
Comment on attachment 8951221 [details]
Bug 1438482 - logErrorInPage should produce an error log;

https://reviewboard.mozilla.org/r/220486/#review227288

::: devtools/server/actors/tab.js:665
(Diff revision 6)
>  
> -  onLogErrorInPage(request) {
> -    let {text, category} = request;
> +  onLogInPage(request) {
> +    let {text, category, flags} = request;
>      let scriptErrorClass = Cc["@mozilla.org/scripterror;1"];
>      let scriptError = scriptErrorClass.createInstance(Ci.nsIScriptError);
> -    scriptError.initWithWindowID(text, null, null, 0, 0, 1,
> +    scriptError.initWithWindowID(text, null, null, 0, 0, flags,

If we provide 1 as a default value for `flags` and then also keep `logErrorInPage` in the list of requests (but point it to `onLogInPage`) then I think that would handle back-compat.

I'm not sure how much we want to worry about it in this case: if the only symptom is that the source map warning doesn't show up on older servers then I'd consider just moving forward and breaking back-compat, but if it causes some other kind of breakage due to an unknown request that seems like a bigger deal. (I haven't tested to see which of those cases is true - this is non protocol.js which may change things).

Maybe :jryans has an opinion on how to proceed here?

::: devtools/server/actors/tab.js:1439
(Diff revision 6)
>    "reconfigure": TabActor.prototype.onReconfigure,
>    "switchToFrame": TabActor.prototype.onSwitchToFrame,
>    "listFrames": TabActor.prototype.onListFrames,
>    "listWorkers": TabActor.prototype.onListWorkers,
> -  "logErrorInPage": TabActor.prototype.onLogErrorInPage,
> +  "logInPage": TabActor.prototype.onLogInPage,
> +  "logWarningInPage": TabActor.prototype.onLogWarningInPage,

I don't think you meant to leave this here. As mentioned above, if we want to attempt to keep backwards-compat we could point `"logErrorInPage"` at `onLogInPage`. Otherwise, remove this line.
Attachment #8951221 - Flags: review?(bgrinstead) → review+
Attachment #8951221 - Flags: review?(jryans)
Asking :jryans as per Comment 14
Comment on attachment 8951221 [details]
Bug 1438482 - logErrorInPage should produce an error log;

https://reviewboard.mozilla.org/r/220486/#review227438

Thanks for asking about protocol compat! :)

I think it seems okay to break compat here if you want to, since the usage of this request is currently very limited, and already tests for the related traits.  However, if you want to proceed that way, then you should actively _remove_ the old trait, since you are no longer supporting the original request.  So, something like remove trait `logErrorInPage` and add trait `logInPage`, perhaps?

By choosing to break compat here, the impact would be that remote debugging versions 56 - 59 would no longer log a few messages, such as: source map error messages, HAR request finished, and Console API replaced.

If you want to maintain compat, it is still fine to change the server side however you like because we enforce that the client is equal to or newer than the server.  So, only the client side (in target.js) needs to consider both both versions.  This would likely mean testing the old vs. new traits to decide whether to send your new request style or the old one.

Feel free to re-request review again if you'd like me to check over the final version.

::: devtools/server/actors/tab.js:234
(Diff revision 6)
>      // to what it was before using the TabActor
>      noTabReconfigureOnClose: true,
>      // Supports the logErrorInPage request.
>      logErrorInPage: true,
> +    // Supports the logWarningInPage request.
> +    logWarningInPage: true,

Maybe this trait should be different, since the request is really `logInPage`?
Attachment #8951221 - Flags: review?(jryans) → review+
(In reply to Brian Grinstead [:bgrins] from comment #14)
> >    "reconfigure": TabActor.prototype.onReconfigure,
> >    "switchToFrame": TabActor.prototype.onSwitchToFrame,
> >    "listFrames": TabActor.prototype.onListFrames,
> >    "listWorkers": TabActor.prototype.onListWorkers,
> > -  "logErrorInPage": TabActor.prototype.onLogErrorInPage,
> > +  "logInPage": TabActor.prototype.onLogInPage,
> > +  "logWarningInPage": TabActor.prototype.onLogWarningInPage,
> 
> I don't think you meant to leave this here.
Yes, removed

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> >      // to what it was before using the TabActor
> >      noTabReconfigureOnClose: true,
> >      // Supports the logErrorInPage request.
> >      logErrorInPage: true,
> > +    // Supports the logWarningInPage request.
> > +    logWarningInPage: true,
> 
> Maybe this trait should be different, since the request is really
> `logInPage`?
Yes, fixed.

> I think it seems okay to break compat here if you want to, since the usage
I think that we all agree that back-compat is not necessary in this case,
so let's not introduce more code that needed.

Thanks Jryans!

Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d5900e45d27
logErrorInPage should produce an error log; r=bgrins,jryans
https://hg.mozilla.org/mozilla-central/rev/1d5900e45d27
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1440291
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: