Last Comment Bug 759193 - Don't display [System Principal] for system compartments
: Don't display [System Principal] for system compartments
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
: Nicholas Nethercote [:njn]
Depends on: 754771
  Show dependency treegraph
Reported: 2012-05-28 13:59 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-10-22 12:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (1.73 KB, patch)
2012-05-29 05:38 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review-
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-05-28 13:59:59 PDT
Here's what I see in about:memory:

>     1.70 MB (00.44%) ++ compartment([System Principal], chrome://adblockplu
>     1.58 MB (00.41%) ++ compartment([System Principal], resource:///modules
>     0.94 MB (00.24%) ++ compartment([System Principal], chrome://browser/co
>     0.87 MB (00.23%) ++ compartment([System Principal], resource://gre/modu
>     0.79 MB (00.21%) ++ compartment([System Principal], resource://gre/modu
>     0.77 MB (00.20%) ++ compartment([System Principal], about:newtab)
>     0.61 MB (00.16%) ++ compartment([System Principal], jar:file:///home/jl

It would be helpful to be able to see more of the compartment's name in non-verbose mode.

We can increase the width of about:memory in a separate bug, but for here: There's no need to say [System Principal] for these compartments.  All the URIs have special schemes indicating that they're not web content.

Can we just get rid of [System Principal]?
Comment 1 Justin Lebar (not reading bugmail) 2012-05-28 14:05:24 PDT
Ouch, this is much worse after we landed bug 757299, because now you can't hover over one of these compartments to get the full compartment URI.

Maybe we should back that out.
Comment 2 Justin Lebar (not reading bugmail) 2012-05-29 05:38:34 PDT
Created attachment 627923 [details] [diff] [review]
Patch v1
Comment 3 Nicholas Nethercote [:njn] 2012-05-29 18:46:30 PDT
Comment on attachment 627923 [details] [diff] [review]
Patch v1

Review of attachment 627923 [details] [diff] [review]:

I like having system compartments easily identifiable.  Literally five minutes ago I was grepping through about:memory?verbose for "System Principal" while looking at bug 756261.

We could do something like replacing "[System Principal]" with "sys" when it's present... that would require exposing SYSTEM_PRINCIPAL_SPEC from caps/src/nsSystemPrincipal.cpp to js/xpconnect/src/XPCJSRuntime.cpp.  Hmm, that's a bit ugly.

How about this instead:  replace "compartment(...)" with "user-compartment(...)" or "system-compartment(...)" as appropriate?  That's a win (in terms of width) for System Principal compartments, but a loss for user compartments... maybe keep "compartment(...)" for user compartments?
Comment 4 Andrew McCreight [:mccr8] 2012-05-29 19:32:47 PDT
system-compartment and compartment sounds good to me.
Comment 5 Justin Lebar (not reading bugmail) 2012-05-29 21:38:13 PDT
(In reply to Andrew McCreight [:mccr8] from comment #4)
> system-compartment and compartment sounds good to me.

Sounds good to me as well!
Comment 6 Nicholas Nethercote [:njn] 2012-05-29 23:56:55 PDT
And the nice thing about "system-compartment" is that it won't show up in about:compartments, which is good because about:compartments already segregates system compartments.
Comment 7 Nicholas Nethercote [:njn] 2012-05-30 02:52:48 PDT
Bonus points if you can separately identify add-on compartments using the instructions in :)
Comment 8 Justin Lebar (not reading bugmail) 2012-10-22 12:25:43 PDT
I don't think we need to fix this anymore because we recently made non-verbose about:memory expand to the full width of the browser window.

Note You need to log in before you can comment on or make changes to this bug.