Status

enhancement
P3
normal
2 years ago
2 months ago

People

(Reporter: bgrins, Unassigned, NeedInfo)

Tracking

(Depends on 1 bug, Blocks 1 bug, {dev-doc-needed})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M4, firefox59 affected)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Now that Bug 1425574 has landed, we should be able to rewrite `new ConsoleAPI` with `Console.createInstance` and then remove Console.jsm and associated tests.
Posted patch console-jsm-prefix.patch (obsolete) — Splinter Review
Baku, with this patch applied if I open Customize mode I see logging as expected which is great! However, I don't see the prefix string in the Browser Console, only stdout.

In stdout I see messages like:
  console.debug: CustomizableUI: "Iterating the actual nodes of the window palette"

In the Browser Console it is just:
  "Iterating the actual nodes of the window palette"

What console.jsm does for the prefix is store the prefix in ConsoleAPIStorage i.e. https://dxr.mozilla.org/mozilla-central/rev/21ddfb9e6cc008e47da89db50e22697dc7b38635/toolkit/modules/Console.jsm#545, then the Browser Console frontend detects this and displays it with a bit of a different color so it's easy to scan. Would it be easy to include the prefix in a similar way with the WebIDL console instance?
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Posted patch prefix.patch (obsolete) — Splinter Review
Attachment #8943141 - Flags: review?(bgrinstead)
Posted patch prefix.patch (obsolete) — Splinter Review
Test included
Attachment #8943141 - Attachment is obsolete: true
Attachment #8943141 - Flags: review?(bgrinstead)
Attachment #8943202 - Flags: review?(bugs)
Attachment #8943202 - Flags: review?(bgrinstead)
Attachment #8943202 - Flags: review?(bugs) → review+
Probably it's better to move this patch into a separate bug.
Depends on: 1431105
Comment on attachment 8943202 [details] [diff] [review]
prefix.patch

Patch moved to bug 1431105.
Attachment #8943202 - Attachment is obsolete: true
Attachment #8943202 - Flags: review?(bgrinstead)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4626fa6c2331
Prefix in Console when used by JSM, r=smaug
Assignee: amarchesini → nobody
bug 1431105 is landed.
Flags: needinfo?(bgrinstead)
:baku - I'm attempting to do this in a couple of steps, the first of which is to delete the implementation of Console.jsm and replace it with the console global (so we don't need to update all consumers at once and we can also choose to keep it there if we are worried about backwards-compat).  However, with this patch applied (attached to the bug) I saw an issue with console.time / console.timeEnd: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf570ed1f9620c7ee05e706896faa488cb1a5f86&selectedJob=156709324.

When looking into this I realized that running this code in a JSM file (for instance, inside of LightweightThemeManager.jsm) the time / timeEnd doesn't show up in the Browser Console but the log does:

  console.log("LightweightThemeManager.jsm");
  console.time("LightweightThemeManager.jsm timer");
  console.timeEnd("LightweightThemeManager.jsm timer");

And then if I run this code in the Browser Console I confirm the timer isn't in the ConsoleAPIStorage (but the log is):

  var ConsoleAPIStorage = Cc["@mozilla.org/consoleAPI-storage;1"]
                              .getService(Ci.nsIConsoleAPIStorage);
  ConsoleAPIStorage.getEvents().map(m=>JSON.stringify(m.arguments))

Any ideas why the time/timeEnd calls aren't showing up in the ConsoleAPIStorage?
Flags: needinfo?(bgrinstead) → needinfo?(amarchesini)
I did some tests using Scratchpad and running this code:

var a = console.createInstance();
a.time("A");
a.timeEnd('A');

var s = Cc["@mozilla.org/consoleAPI-storage;1"]
                            .getService(Ci.nsIConsoleAPIStorage);
s.getEvents().forEach(e => {
 console.log(e.ID, e.timer);
});

I see:

jsm Object { name: "A" }
jsm Object { duration: 0, name: "A" }

Running your code, I see:

"["jsm",{"duration":0,"name":"A"}]"

definitely it's a bug having duration == 0. Is this what you see? I'm going to fix this here, but if you see something more, let me know.
Flags: needinfo?(amarchesini) → needinfo?(bgrinstead)
Summary: Remove console.jsm → [Meta-Bug] Remove console.jsm
Depends on: 1433625
After re-testing with Bug 1433625 resolved I'm now seeing a different error on both devtools/client/webconsole/test/browser_console_consolejsm_output.js and with the LightweightThemeManager example script in Comment 10. If I include these lines in LightweightThemeManager.jsm:

  console.log("LightweightThemeManager.jsm");
  console.time("LightweightThemeManager.jsm timer");
  console.timeEnd("LightweightThemeManager.jsm timer");

Then when I `./mach run --jsconsole` I see an error in the Browser Console:

  Error: Timer “LightweightThemeManager.jsm timer” doesn’t exist.
  Stack trace:
  logConsoleAPIMessage@resource://devtools/shared/base-loader.js -> resource://devtools/client/webconsole/webconsole.js:1278:25
  _outputMessageFromQueue@resource://devtools/shared/base-loader.js -> resource://devtools/client/webconsole/webconsole.js:2103:16
  _flushMessageQueue@resource://devtools/shared/base-loader.js -> resource://devtools/client/webconsole/webconsole.js:1992:20

We are receiving this packet from the actor which means we are somehow hitting the eTimerDoesntExist case in Console.cpp:

  {
    "addonId": "",
    "arguments": [
      "LightweightThemeManager.jsm timer"
    ],
    "columnNumber": 3,
    "counter": null,
    "filename": "resource://gre/modules/LightweightThemeManager.jsm",
    "functionName": "",
    "groupName": "",
    "level": "timeEnd",
    "lineNumber": 20,
    "prefix": "",
    "private": false,
    "timeStamp": 1517266277688,
    "timer": {
      "error": "timerDoesntExist",
      "name": "LightweightThemeManager.jsm timer"
    },
    "workerType": "none",
    "styles": [],
    "category": "webdev",
    "_type": "ConsoleAPI"
  }

Here's the interesting thing - If I do this script instead it works just as expected:

  var c = console.createInstance();
  c.log("LightweightThemeManager.jsm");
  c.time("LightweightThemeManager.jsm timer");
  c.timeEnd("LightweightThemeManager.jsm timer");

So I guess this only is an issue when using the webidl console global directly from a JSM, and it's not a problem when first calling createInstance. Seems like a bug, yeah?
Flags: needinfo?(amarchesini)
Clearing needinfo about time/timeEnd as I've moved this into Bug 1440464
Flags: needinfo?(amarchesini)
Product: Firefox → DevTools
Fission Milestone: --- → ?
Priority: -- → P3
Fission Milestone: ? → M4
You need to log in before you can comment on or make changes to this bug.