Closed Bug 1425574 Opened 2 years ago Closed 2 years ago

Fill the feature gap between Console.jsm and Console API

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 3 obsolete files)

18.15 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.74 KB, patch
smaug
: review+
bgrins
: review+
Details | Diff | Splinter Review
4.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.10 KB, patch
smaug
: review+
bgrins
: review+
Details | Diff | Splinter Review
617 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
4.09 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.36 KB, patch
smaug
: review+
bgrins
: review+
Details | Diff | Splinter Review
6.26 KB, patch
smaug
: review+
bgrins
: review+
Details | Diff | Splinter Review
12.80 KB, patch
smaug
: review+
bgrins
: review+
Details | Diff | Splinter Review
As discussed during the All Hands meeting, Console API doesn't have all the features of Console.jsm and that is a blocker for get rid of Console.jsm.
This bug is about introducing them. Here the list:

1. Console API is not exposed to system. Bug 1425463 covers this point.

2. No custom console instances with custom options. We need a constructor for the Console.

3. ConsoleID attribute in ConsoleEvent

4. Customizable InnerID

5. Dump function support

6. Prefix for the dump function
Smaug, are you OK with reviewing this patch?

I spoke with bz and there is no way (currently) to avoid the introducing a new interface at WebIDL binding level. But it doesn't seem too bad, actually.
Attachment #8937170 - Flags: review?(bugs)
Attachment #8937171 - Flags: review?(bugs)
Attachment #8937172 - Flags: review?(bugs)
Attached patch part 4 - dump function (obsolete) — Splinter Review
By default, if nothing is passed, Console.jsm uses dump(). Here I do the same.
Attachment #8937173 - Flags: review?(bugs)
Attached patch part 5 - prefixSplinter Review
Attachment #8937174 - Flags: review?(bugs)
We discussed this. Just a confirmation that we don't care about setting a maxLogLevel value.
Attachment #8937175 - Flags: review?(bgrinstead)
Console must be always active when used by JSM in custom instances.
Attachment #8937176 - Flags: review?(bugs)
Comment on attachment 8937175 [details] [diff] [review]
part 6 - no maxLogLevel support

Review of attachment 8937175 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure if this is an alternate or complementary approach, but at some stage we attempted to keep Console.jsm around and have it wrap the WebIDL console in Bug 1191571 (see https://bugzilla.mozilla.org/show_bug.cgi?id=1191571#c16). It didn't work at the time, but once Bug 1425463 lands it may be possible to revisit that approach if we want to keep some of the functionality around in a JSM for consumers that want it (like max log levels, etc). I'll send an email to the list to gauge interest in this feature, as it does appear to be used a fair amount: https://dxr.mozilla.org/mozilla-central/search?q=maxLogLevel&redirect=true.
Attachment #8937176 - Flags: review?(bugs) → review+
Attachment #8937175 - Flags: review?(bgrinstead) → review+
Comment on attachment 8937174 [details] [diff] [review]
part 5 - prefix

Someone familiar with the .jsm should review this too.
Attachment #8937174 - Flags: review?(bugs) → review+
Comment on attachment 8937173 [details] [diff] [review]
part 4 - dump function

Someone familiar with .jsm usage must review this.

This leaks since mDumpFunction isn't cycle collected.
Attachment #8937173 - Flags: review?(bugs) → review-
Comment on attachment 8937172 [details] [diff] [review]
part 3 - custom innerID

Someone familiar with the .jsm should review this too.
Attachment #8937172 - Flags: review?(bugs) → review+
Comment on attachment 8937171 [details] [diff] [review]
part 2 - consoleID in ConsoleEvent

Someone familiar with the .jsm should review this too.
(It is unclear to me why we need both consoleID and innerID)
Attachment #8937171 - Flags: review?(bugs) → review+
Attachment #8937171 - Flags: review?(bgrinstead)
Attachment #8937172 - Flags: review?(bgrinstead)
Attached patch part 4 - dump function (obsolete) — Splinter Review
Attachment #8937173 - Attachment is obsolete: true
Attachment #8937666 - Flags: review?(bugs)
Attachment #8937666 - Flags: review?(bgrinstead)
Attachment #8937174 - Flags: review?(bgrinstead)
> I'm not sure if this is an alternate or complementary approach, but at some

Yes it is. The idea is that, when this code lands, we can get rid of Console.jsm completely.
Several good reasons to do this:

1. a single implementation of the console API.
2. Console API can be used in Workers as well when running in JSM.
3. If we want to have a for an actor based approach everywhere, DOM Console already has the support for this and we don't have to touch Console.jsm.

Note that it's not a big deal to implement maxLogLevel. If we need it, I can add it.
(In reply to Andrea Marchesini [:baku] from comment #14)
> > I'm not sure if this is an alternate or complementary approach, but at some
> 
> Yes it is. The idea is that, when this code lands, we can get rid of
> Console.jsm completely.
> Several good reasons to do this:
> 
> 1. a single implementation of the console API.
> 2. Console API can be used in Workers as well when running in JSM.
> 3. If we want to have a for an actor based approach everywhere, DOM Console
> already has the support for this and we don't have to touch Console.jsm.

Agree that completely getting rid of Console.jsm is the ideal here.

> Note that it's not a big deal to implement maxLogLevel. If we need it, I can
> add it.

My inclination (although I'm still waiting to hear back from more folks) is that maxLogLevel isn't that big of a deal but that maxLogLevelPref is actually used and we'll need to account for it (or something like it). And if we are adding maxLogLevelPref  then we'd have the machinery in place for maxLogLevel and we may as well keep it for API compatibility.
Depends on: 1425463
See Also: → 1191571
Comment on attachment 8937666 [details] [diff] [review]
part 4 - dump function

> Console::ProfileMethodInternal(JSContext* aCx, const nsAString& aAction,
>                                const Sequence<JS::Value>& aData)
> {
>   // Make all Console API no-op if DevTools aren't enabled.
>   if (!nsContentUtils::DevToolsEnabled(aCx)) {
>     return;
>   }
> 
>+  MaybeExecuteDumpFunction(aCx, aAction, aData);
>+
ok, I don't understand when dump is supposed to be called. Why here?

>@@ -1331,16 +1337,24 @@ Console::MethodInternal(JSContext* aCx, 
> 
>   else if (aMethodName == MethodCount) {
>     callData->mCountValue = IncreaseCounter(aCx, aData, callData->mCountLabel);
>     if (!callData->mCountValue) {
>       return;
>     }
>   }
> 
>+  // Before processing this CallData differently, it's time to call the dump
>+  // function.
>+  if (aMethodName == MethodTrace) {
>+    MaybeExecuteDumpFunctionForTrace(aCx, stack);
>+  } else {
>+    MaybeExecuteDumpFunction(aCx, aMethodString, aData);
>+  }
and here

It doesn't seem to map too well to what Console.jsm has, since there this kind of dumping depends on log level. Could you explain this some more?
Attachment #8937666 - Flags: review?(bugs) → review-
Comment on attachment 8937175 [details] [diff] [review]
part 6 - no maxLogLevel support

So I guess this might not be the right approach.
Attachment #8937170 - Flags: review?(bugs) → review+
Priority: -- → P3
Attachment #8937171 - Flags: review?(bgrinstead) → review+
> It doesn't seem to map too well to what Console.jsm has, since there this
> kind of dumping depends on log level. Could you explain this some more?

That is not supported yet. I'm going to submit a patch for this.

The reason why I need to call it in 2 different places, is that, dump(), when called for trace() must show the trace log but that is not available immediately. I need to wait until the trace stack is part of callData.
Comment on attachment 8937174 [details] [diff] [review]
part 5 - prefix

Review of attachment 8937174 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/console/tests/test_jsm.xul
@@ +18,5 @@
>  const JSM = "chrome://mochitests/content/chrome/dom/console/tests/console.jsm";
>  
>  let dumpCalled = 0;
>  function dumpFunction(msg) {
> +  ok(msg.indexOf("_PREFIX_") != -1, "we have a prefix");

Could check that _PREFIX_ is actually at the start of the message like `msg.startsWith("_PREFIX_")`
Attachment #8937174 - Flags: review?(bgrinstead) → review+
Comment on attachment 8937666 [details] [diff] [review]
part 4 - dump function

Review of attachment 8937666 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/console/Console.cpp
@@ +2592,5 @@
> +  message.AssignLiteral(": ");
> +
> +  for (uint32_t i = 0; i < aData.Length(); ++i) {
> +    JS::Rooted<JS::Value> v(aCx, aData[i]);
> +    JS::Rooted<JSString*> jsString(aCx, JS::ToString(aCx, v));

Will this print an object like `{ foo: 'bar' }` as "[object Object]"?

Console.jsm attempts to stringify as: https://dxr.mozilla.org/mozilla-central/rev/1624b88874765bf57e9feba176d30149c748d9d2/toolkit/modules/Console.jsm#120-168. I don't think we need to match this behavior exactly but if there was a way to dump out stringified versions at least for plain objects that would be nice for parity.

::: dom/console/ConsoleInstance.cpp
@@ +28,5 @@
> +  if (aOptions.mDump.WasPassed()) {
> +    mConsole->mDumpFunction = &aOptions.mDump.Value();
> +  } else {
> +    // For historical reasons, ConsoleInstance prints messages on stdout.
> +    mConsole->mDumpToStdout = true;

Just confirming, this is the case that gets hit when using the Console API from a JSM?  i.e. we will get dumping by default in that case?
Comment on attachment 8937172 [details] [diff] [review]
part 3 - custom innerID

Review of attachment 8937172 [details] [diff] [review]:
-----------------------------------------------------------------

I've looked through the consumers of ConsoleAPI from Console.jsm and none of them set innerID at construction except for one test:
- https://dxr.mozilla.org/mozilla-central/search?q=new+consoleapi
- https://dxr.mozilla.org/mozilla-central/search?q=inner
- https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/test/test_consoleapi_innerID.html#23

So I think the only time this is set is when actual messages are logged in Console.jsm (set to aFrame.filename here: https://dxr.mozilla.org/mozilla-central/rev/1624b88874765bf57e9feba176d30149c748d9d2/toolkit/modules/Console.jsm#537). But will messages logged via Console API already have an innerID if they come from JSMs? If so maybe this feature (and the associated test can be removed).
Attachment #8937172 - Flags: review?(bgrinstead)
> Will this print an object like `{ foo: 'bar' }` as "[object Object]"?

Yes. I'll try to have something better.

> Just confirming, this is the case that gets hit when using the Console API
> from a JSM?  i.e. we will get dumping by default in that case?

By default it writes on stdout, yes.
> Could check that _PREFIX_ is actually at the start of the message like
> `msg.startsWith("_PREFIX_")`

I cannot, because the message is: "Console.<methodName>: <prefix> : <the message>"
Attached patch part 7 - levelSplinter Review
Attachment #8938323 - Flags: review?(bugs)
Attached patch part 8 - pref (obsolete) — Splinter Review
Attachment #8938325 - Flags: review?(bugs)
Attachment #8938323 - Attachment description: part 7 - pref → part 7 - level
Attached patch part 8 - prefSplinter Review
Attachment #8938325 - Attachment is obsolete: true
Attachment #8938325 - Flags: review?(bugs)
Attachment #8938362 - Flags: review?(bugs)
Attachment #8938362 - Flags: review?(bgrinstead)
Attachment #8938323 - Flags: review?(bgrinstead)
Comment on attachment 8938362 [details] [diff] [review]
part 8 - pref

(Console.jsm is a bit weird API, but no reasons to change it too much in this bug)
Attachment #8938362 - Flags: review?(bugs) → review+
Attachment #8938323 - Flags: review?(bugs) → review+
Smaug, are you OK with the 'dump function' part of these patches? Have you seen the comment?
Flags: needinfo?(bugs)
Same patch, but using ToSource()
Attachment #8938638 - Flags: review?(bugs)
Attachment #8938638 - Flags: review?(bgrinstead)
Flags: needinfo?(bugs)
Attachment #8938638 - Flags: review?(bugs) → review+
Attachment #8937666 - Attachment is obsolete: true
Attachment #8937666 - Flags: review?(bgrinstead)
Comment on attachment 8938323 [details] [diff] [review]
part 7 - level

Review of attachment 8938323 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/console/Console.cpp
@@ +1095,5 @@
>    if (!IsEnabled(aCx)) {
>      return;
>    }
>  
> +  if (!ShouldProceed(aMethodName)) {

I don't think there's a requirement to distinguish MethodProfile and MethodProfileEnd here for log levels. We should be able to treat both as MethodProfile or even MethodLog - there's no reason these should ever have different levels from each other. Doing this should allow this patch to be simplified (i.e. no need for the signature changes to ProfileMethod/ProfileMethodInternal).
Attachment #8938362 - Flags: review?(bgrinstead) → review+
Comment on attachment 8938638 [details] [diff] [review]
part 4 - dump function

Review of attachment 8938638 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/console/Console.cpp
@@ +2592,5 @@
> +  message.AssignLiteral(": ");
> +
> +  for (uint32_t i = 0; i < aData.Length(); ++i) {
> +    JS::Rooted<JS::Value> v(aCx, aData[i]);
> +    JS::Rooted<JSString*> jsString(aCx, JS_ValueToSource(aCx, v));

I believe that using toSource() will sometimes not work as well as toString(), for example in these cases the toString() gives more useful info:

`window.location.toString()`: "https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1425574&attachment=8938638
`window.location.toSource()`: "({get href() { [native code] }, set href() { [native code] }, ...})"

`document.body.toString()`: "[object HTMLBodyElement]"
`document.body.toSource()`: "({})"

I'm not really sure how we can decide what will give better output for each object - maybe for plain JS objects we should use toSource and for others toString? In the meantime, I'm OK to land it and iterate in follow ups
Attachment #8938638 - Flags: review?(bgrinstead) → review+
> I don't think there's a requirement to distinguish MethodProfile and
> MethodProfileEnd here for log levels. We should be able to treat both as

Yes, you are right, but it seems more consistent in that way. If you don't mind, I would land the patches as they are.
Flags: needinfo?(bgrinstead)
(In reply to Andrea Marchesini [:baku] from comment #32)
> > I don't think there's a requirement to distinguish MethodProfile and
> > MethodProfileEnd here for log levels. We should be able to treat both as
> 
> Yes, you are right, but it seems more consistent in that way. If you don't
> mind, I would land the patches as they are.

If you prefer it this way it's fine with me
Flags: needinfo?(bgrinstead)
Attachment #8938323 - Flags: review?(bgrinstead) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a832b492dd
Fill the feature gap between Console.jsm and Console API - part 1 - Console.createInstance(), r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb19ee84ce26
Fill the feature gap between Console.jsm and Console API - part 2 - consoleID in ConsoleEvent, r=smaug, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd2d2b4d9e6
Fill the feature gap between Console.jsm and Console API - part 3 - custom innerID, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/465b929da94d
Fill the feature gap between Console.jsm and Console API - part 4 - dump function, r=smaug, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/69dc8f8bea22
Fill the feature gap between Console.jsm and Console API - part 5 - prefix, r=smaug, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/0882de3b96b9
Fill the feature gap between Console.jsm and Console API - part 6 - no maxLogLevel support, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f44d89eda68
Fill the feature gap between Console.jsm and Console API - part 7 - Console active, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4e812ee5ad
Fill the feature gap between Console.jsm and Console API - part 8 - maxLogLevel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb150980a7f
Fill the feature gap between Console.jsm and Console API - part 9 - maxLogLevel pref, r=smaug, r=bgrins
Duplicate of this bug: 988636
Duplicate of this bug: 1191571
Duplicate of this bug: 942820
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.