Closed Bug 1260877 Opened 8 years ago Closed 5 years ago

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

Categories

(DevTools :: Console, enhancement, P2)

Product:
DevTools
Component:
Console
Version:
46 Branch
Type:
enhancement

Tracking

(firefox68 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: bgrins, Assigned: nchevobbe)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

Bug 1260877 - Add a `chromeContext` property to ConsoleMessage. r=bgrins.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1260877 - Display a Show content messages checkbox in Browser Console. r=bgrins.
47 bytes, text/x-phabricator-request
Details | Review 
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.
Priority: -- → P3
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?
Flags: needinfo?(philip.chee)
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?
Flags: needinfo?(philip.chee) → needinfo?(bgrinstead)
(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.
Depends on: 1362023
Flags: needinfo?(bgrinstead)
Would a "Hide content messages" checkbox be enough ?
I feel like it wouldn't be hard to implement
(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.
: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?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
(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?
ni? for Comment 11
Flags: needinfo?(amarchesini)
Attached patch ConsoleEvent.chromeContext (obsolete) — — Splinter Review 
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?
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8955341 - Flags: review?(bgrinstead)
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.
Good to me.

About nsIConsoleService I'll send a new patch.
Blocks: 1442333
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.
Attachment #8955341 - Flags: review?(bgrinstead) → review+
Depends on: 1442778
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1442778 to land the platform pieces
Comment on attachment 8955341 [details] [diff] [review]
ConsoleEvent.chromeContext

Moved to bug 1442778.
Attachment #8955341 - Attachment is obsolete: true
Assignee: amarchesini → nobody
Product: Firefox → DevTools
Priority: P3 → P2

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.

Assignee: nobody → nchevobbe
Type: defect → enhancement

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

(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.

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.

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

See Also: → 1543391
Pushed by nchevobbe@mozilla.com:
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.
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10fd2d1f0fa2
delete trailing spaces on a CLOSED TREE

stubs need to be updated.

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
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.

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Blocks: 1544862

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

Flags: needinfo?(nchevobbe)

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

Flags: needinfo?(nchevobbe)
Depends on: 1561930

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)}}).

Flags: needinfo?(nchevobbe)
Keywords: dev-doc-needed → dev-doc-complete

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

Flags: needinfo?(nchevobbe)

(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!).

Flags: needinfo?(ismith)

(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.

Flags: needinfo?(ismith)
Depends on: 1613367
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: