1260877 - Add a 'chrome'/'content' filtering for the Browser Console

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Brian Grinstead [:bgrins]]

There are a number of duplicate bugs / related bugs to Bug 802543 which have to do generally logs from content showing up in the Browser Console.

Apparently console2 (https://addons.mozilla.org/en-US/firefox/addon/console%C2%B2/) has a 'chrome' and 'content' switch that allow you to turn off non-relevant messages.  Some kind of pref or UI to do this in the Browser Console would be nice to limit the amount of messages when you only care about one type.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Hi Philip, would you be able to help out here with either a patch or an explanation of how you filtered where the events were coming from so a new contributor may be able to pick up this bug?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Brian, would this be as simple as checking the if the data-url of any of the frames of the callstack begin with "resource://" or "chrome://"? Or even the `.frame-link` descendant of each `.message` in the HUD console?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Brian, would this be as simple as checking the if the data-url of any of the
> frames of the callstack begin with "resource://" or "chrome://"? Or even the
> `.frame-link` descendant of each `.message` in the HUD console?

I think that would work, although I'd prefer to add this into the new UI rather than make changes to the old one (which will be removed once we finish porting over tests and get the new one turned on in the Browser Console). We could either look at the stack as you say, or modify the packets that get passed to the frontend with a new isChrome bool:

- https://dxr.mozilla.org/mozilla-central/rev/b5a3b8ef6902998507fc881b6d628b055457fe31/devtools/server/actors/webconsole.js#1529
- https://dxr.mozilla.org/mozilla-central/rev/b5a3b8ef6902998507fc881b6d628b055457fe31/devtools/server/actors/webconsole.js#1797

I'd prefer to handle this on the server-side since I suspect we can more quickly/reliably check for this case there, where we have the actual objects handy.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Would a "Hide content messages" checkbox be enough ?
I feel like it wouldn't be hard to implement

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> Would a "Hide content messages" checkbox be enough ?
> I feel like it wouldn't be hard to implement

Yes I'm pretty sure a checkbox for content (in the place where 'persist logs' was) would handle the request here. I could also see adding a filter UI that lets you toggle content / addon meessage / frame scripts / etc, but that seems unnecessary for this bug.

Comment 9

[Brian Grinstead [:bgrins]]

:baku, what would be the best way to detect if a console API messages and console service messages are coming from a chrome context? If there isn't already a consistent way to check, is that something that could be added to those APIs?

Comment 10

[Andrea Marchesini [:baku]]

I would introduce the principal in ConsoleEvent dictionary. Then devtools can check if that principal is system or not, and if it matches with the content one. Let me know if this solution is ok for you. I can quickly implement it.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Andrea Marchesini [:baku] from comment #10)
> I would introduce the principal in ConsoleEvent dictionary. Then devtools
> can check if that principal is system or not, and if it matches with the
> content one. Let me know if this solution is ok for you. I can quickly
> implement it.

That sounds great! That would support both API and Service messages?

Comment 12

[Brian Grinstead [:bgrins]]

ni? for Comment 11

Comment 13

[Andrea Marchesini [:baku]]

Attachment: ConsoleEvent.chromeContext

This is for the Console API. Probably we don't need the full principal, but just a chrome context boolean could be enough. Feedback?

About nsIConsoleService, do we currently show any message matching a window ID?
Probably we should not change that. Do we have any particular use-case where we need to add a chrome context boolean here?

Comment 14

[Brian Grinstead [:bgrins]]

We currently seem to show all console service messages in the Browser Console.  For example if you load https://bgrins.github.io/devtools-demos/console/all.html I see a few errors in the BC from all.html, like "ReferenceError: xhrException is not defined". And I'd want those to go away when the user disables content messages in the UI.

Comment 15

[Andrea Marchesini [:baku]]

Good to me.

About nsIConsoleService I'll send a new patch.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8955341 [details] [diff] [review]
ConsoleEvent.chromeContext

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

Looks right to me. Let me spin off a bug blocking this one to land this (and the console service portion), since this bug has a bunch of dupes for the actual feature request.

Comment 17

[Brian Grinstead [:bgrins]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1442778 to land the platform pieces

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8955341 [details] [diff] [review]
ConsoleEvent.chromeContext

Moved to bug 1442778.

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

We are missing the flag for network messages, but Honza pointed out https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/devtools/server/actors/network-monitor/network-observer.js#53 where we already filter out Chrome logs. I guess we could use that to identify Content originated network calls as well.

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Also, this bug is for the Browser Console, but should we expand this logic to the Browser Toolbox?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe from comment #21)

Also, this bug is for the Browser Console, but should we expand this logic to the Browser Toolbox?

IMO the two main things we get out of this are (1) less noise in the UI when you really only care about chrome and (2) better performance so that opening the console doesn't slow down the rest of the UI. Both (1) and (2) are important to the Browser Console and primarily (1) is important to the Browser Toolbox. I think it'd make sense to add to the Browser Toolbox if it's not too much work, but I'd be happy to punt that into another bug as well.

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1260877 - Add a `chromeContext` property to ConsoleMessage. r=bgrins.

This property was already in console api message packet,
and this patch also adds it to pageError packets (retrieving
it from nsIScriptError.isFromChromeContext).

Stubs are updated to include the new property.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1260877 - Display a Show content messages checkbox in Browser Console. r=bgrins.

A preference is added to enable this feature, another one
to store the last value of the checkbox.
When unchecked, the console hides all the messages that
don't originate from a chrome contect, via the chromeContext
property on the message.

Since we already have a test to check that content messages are
displayed in the console output, we use it to assert the
effects of the "Show content messages" checkbox.

Depends on D26336

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17059c813d40
Add a `chromeContext` property to ConsoleMessage. r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/e4f28c656165
Display a Show content messages checkbox in Browser Console. r=bgrins.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10fd2d1f0fa2
delete trailing spaces on a CLOSED TREE

Comment 27

[Razvan Maries]

Backed out for perma dt fails on devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_page_error.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=e4f28c65616566391d87789bb02a362942e86f11

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240566195&repo=autoland&lineNumber=5643

Backout: https://hg.mozilla.org/integration/autoland/rev/eb8d178cd72a4b86306bfb67872a96d26d348f9d

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

stubs need to be updated.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5164ff07d12
Add a `chromeContext` property to ConsoleMessage. r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/8186ed6d36ff
Display a Show content messages checkbox in Browser Console. r=bgrins.

Comment 30

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b5164ff07d12
https://hg.mozilla.org/mozilla-central/rev/8186ed6d36ff

Comment 31

[Irene Smith]

There doesn't seem to be any kind of checkbox in the browser console to show this information. Am I missing something?

Comment 32

[Nicolas Chevobbe [:nchevobbe]]

You need to set the devtools.browserconsole.filterContentMessages preference to true.
We plan to enable it by default in the next weeks.

Comment 33

[Irene Smith]

Added a paragraph to the end of the introductory section of the Browser Console page describing the checkbox. I'd like to add a bit more content here, however, describing what those content messages are so readers understand the context.

Also added this to the Firefox 68 release notes:

The Browser Console now allows you to show or hide messages from the content process by setting or clearing the checkbox labeled Show Content Messages ({{bug(1260877)}}).

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Thanks Irene, I added some precision to what are content process messages and asked for review on the mdn page.

Comment 35

[Chris Mills [:cmills]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #34)

Thanks Irene, I added some precision to what are content process messages and asked for review on the mdn page.

As an FYI, it is probably not a good idea to use the Editorial review flag on the page itself. These don't really flag anything, or ping someone, and tend to just sit there and get ignored. It would be better to ni Irene in the bug (done!).

Comment 36

[Irene Smith]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #35)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #34)

Thanks Irene, I added some precision to what are content process messages and asked for review on the mdn page.

As an FYI, it is probably not a good idea to use the Editorial review flag on the page itself. These don't really flag anything, or ping someone, and tend to just sit there and get ignored. It would be better to ni Irene in the bug (done!).

Thanks for the update, Nicolas, I took a quick look at your changes and appreciate your time.