Open Bug 1430810 Opened 3 years ago Updated 3 months ago

[Meta-Bug] Remove console.jsm

Categories

(DevTools :: Console, enhancement, P3)

enhancement

Tracking

(Fission Milestone:Future, firefox59 affected)

Fission Milestone Future
Tracking Status
firefox59 --- affected

People

(Reporter: bgrins, Unassigned, NeedInfo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: dt-fission)

Attachments

(1 file, 3 obsolete files)

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.
Keywords: dev-doc-needed
Attached 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)
Attached patch prefix.patch (obsolete) — Splinter Review
Attachment #8943141 - Flags: review?(bgrinstead)
Attached 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)
Attachment #8943037 - Attachment is obsolete: true
: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)
Depends on: 1439677
Depends on: 1439686
Depends on: 1439689
Depends on: 1440094
Depends on: 1440464
Clearing needinfo about time/timeEnd as I've moved this into Bug 1440464
Flags: needinfo?(amarchesini)
Depends on: 1447210
Product: Firefox → DevTools
Fission Milestone: --- → ?
Priority: -- → P3
Fission Milestone: ? → M4
Fission Milestone: M4 → Future

Adding dt-fission whiteboard tag to DevTools bugs that mention Fission or block Fission meta bugs but don't already have a dt-fission whiteboard tag.

Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.