1430810 - [META] Remove console.sys.mjs

Status: NEW

(DevTools :: Console, task, P3)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: console-jsm-prefix.patch

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?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: prefix.patch

Comment 3

[Andrea Marchesini [:baku]]

Attachment: prefix.patch

Test included

Comment 4

[Andrea Marchesini [:baku]]

Probably it's better to move this patch into a separate bug.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8943202 [details] [diff] [review]
prefix.patch

Patch moved to bug 1431105.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4626fa6c2331
Prefix in Console when used by JSM, r=smaug

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/113a0bd831c4
Backed out - wrong bug ID in the patch

Comment 8

[Andrea Marchesini [:baku]]

bug 1431105 is landed.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1430810 - Remove Console.jsm implementation, using the WebIDL implementation behind the scenes

Review commit: https://reviewboard.mozilla.org/r/215400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215400/

Comment 10

[Brian Grinstead [:bgrins]]

: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?

Comment 11

[Andrea Marchesini [:baku]]

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.

Comment 12

[Brian Grinstead [:bgrins]]

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?

Comment 13

[Brian Grinstead [:bgrins]]

Clearing needinfo about time/timeEnd as I've moved this into Bug 1440464

Comment 14

[Chris Peterson [:cpeterson]]

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.

Comment 15

[Chris Peterson [:cpeterson]]

Moving old "dt-fission" bugs to "dt-fission-future" because they don't block Fission MVP.

Comment 16

[Alexandre Poirot [:ochameau]]

(this is not related to fission)