Closed Bug 1439686 Opened 3 years ago Closed 3 years ago

Always print console API commands from chrome callers to stdout if browser.dom.window.dump.enabled is true

Categories

(DevTools :: Console, enhancement, P3)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Right now, only messages coming from consoles created via console.createInstance print to stdout.

I've discussed this with Andrea, and for consistency with Console.jsm and to allow for replacing things like NS_ASSERTION it would be good to always print to stdout if browser.dom.window.dump.enabled is true, as it is in local builds: https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/modules/libpref/init/all.js#1099
Blocks: 1439689
Attached patch console1.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8952784 - Flags: review?(bgrinstead)
Comment on attachment 8952784 [details] [diff] [review]
console1.patch

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

I was hoping this would only affect logs coming from chrome callers, i.e. loading `data:text/html,<script>console.log("hello");</script>` would not print to stdout. I'm not sure if there's a particular harm in it doing so, other than it just being noisy and people wanting to filter it out as evidenced by feature requests like Bug 1260877. Would it be easy to restrict this so content messages don't get printed?
Attachment #8952784 - Flags: review?(bgrinstead)
Attached patch console1.patch (obsolete) — Splinter Review
Attachment #8952784 - Attachment is obsolete: true
Attachment #8952792 - Flags: review?(bgrinstead)
Comment on attachment 8952792 [details] [diff] [review]
console1.patch

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

::: dom/console/Console.cpp
@@ +826,5 @@
>      }
>    }
>  
> +  // Let's enable the dumping to stdout by default for JSM.
> +  if (!mWindow && NS_IsMainThread()) {

I also want to see logging to stdout for scripts loaded in chrome windows (i.e. browser.js). So could this be just `NS_IsMainThread()?

Sorry for not being clear on the requirements in the bug description - I thought I wrote this down but it must've been on IRC.
Attachment #8952792 - Flags: review?(bgrinstead)
Attached patch console1.patchSplinter Review
Attachment #8952792 - Attachment is obsolete: true
Attachment #8952804 - Flags: review?(bgrinstead)
Comment on attachment 8952804 [details] [diff] [review]
console1.patch

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

Thanks, this works for me! Please update the commit message to indicate this is for chrome contexts only
Attachment #8952804 - Flags: review?(bgrinstead) → review+
Blocks: 1440094
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b192696c08a2
Console API should print logs on stdout when used by chrome code and if browser.dom.window.dump.enabled is true, r=bgrins
https://hg.mozilla.org/mozilla-central/rev/b192696c08a2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
See Also: → 1480544
You need to log in before you can comment on or make changes to this bug.