about:memory lets you do various things: trigger GC and CC; save and load
memory reports; view reports in non-verbose and verbose mode (and diff mode). These are controlled via a combination of buttons and URL modifications.
There is scope for improvement here. I'm not yet sure what the final design should look like, but I have a couple of high level goals:
- Get rid of URL modifications.
- Don't do anything when the page is first loaded; wait for the user to request some action.

Created attachment 733668[details][diff][review]
Improve about:memory's functional UI.
Draft patch. jlebar, I'm interested in UI feedback at the moment more than
code feedback. You'll need to try it out... but the good news is that once
you apply the patch you'll be able to simply restart Firefox to see the changes
-- no need to rebuild.
Apart from the obvious changes (which you see as soon as you load
about:memory), I also changed things so that putting |?verbose| in the URL no
longer has any effect. In about:memory, you control that via the "Verbosity"
drop-down. In about:compartments, we now simply always show the full
compartment names.

Created attachment 733672[details]
about:memory screenshot
For those who are curious but too lazy to apply the patch, here's a screenshot of about:memory after loading. "Verbosity" is pretty obvious; "Pre-action" is one of None/GC/CC/MMU, and "Action" covers measure/save/load/clipboard/clear.

Created attachment 733737[details][diff][review]
(part 1) - Improve about:memory's functional UI.
Actually, here's a complete patch, with the tests all passing. So feel free to
review the code if you want as well :)
Things of note:
- about:compartments now catches exceptions, so if a problem occurs you'll at
least see an error message in the page. (Previously the page would just go
blank.)
- test_aboutcompartments.xul no longer tests about:compartments?verbose, since
that's been removed.

Did you consider radio buttons instead of drop-downs? Radio buttons take only one click to select, and they also involve no mystery meat -- all of their options are present onscreen with no clicking.
> - Get rid of URL modifications.
What's your motivation for doing this? I could imagine under this new scheme you select what you want and then are pushState'd to e.g.
about:memory?verbose=1&preAction=foo&action=bar
Then we could hand out those URLs instead of telling people which options to select.

Comment on attachment 733728[details][diff][review]
(part 2) - Add a test for "?file=" loading in about:memory.
>diff --git a/toolkit/components/aboutmemory/tests/test_aboutmemory4.xul b/toolkit/components/aboutmemory/tests/test_aboutmemory4.xul
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/aboutmemory/tests/test_aboutmemory4.xul>+ function finish()
>+ {
>+ SimpleTest.finish();
>+ }
I don't understand the purpose of this function.
>+ // Because the file load is async, we don't know when it will finish and
>+ // the output will show up. So we poll.
>+ // [Actually, it's no longer async, but this code still works.]
When I looked in the other bug, the file load looked pretty async to me. But
this is a new test; surely we can write it the right way, or at least we can
write a different comment.
>+ function copyPasteAndCheck() {
>+ // Copy and paste frame contents, and filter out non-deterministic
>+ // differences.
>+ synthesizeKey("A", {accelKey: true});
>+ synthesizeKey("C", {accelKey: true});
>+ let actual = SpecialPowers.getClipboardData("text/unicode");
>+ actual = actual.replace(/pid \d+/, "pid NNN");
Same /\bpid \d+\b/ business.

Now that I have a build: I'm not wild about this UI, because it suggests there are more combinatorial options than there actually are. Verbosity has no effect when saving to a file or when doing "clear". The pre-action has no (visible) effect when loading from clipboard or a file.
I do like how we say "Saved reports to /home/jlebar/Downloads/memory-report.json.gz" as opposed to asking the user to play find-and-seek. :)

> Did you consider radio buttons instead of drop-downs? Radio buttons take
> only one click to select, and they also involve no mystery meat -- all of
> their options are present onscreen with no clicking.
I did. I felt they made the top part too big -- "Action" already has five entries, which is a lot for a radio button, and it could get more in the future (e.g. "dump all strings"). But it wasn't an obvious decision.
> Now that I have a build: I'm not wild about this UI, because it suggests
> there are more combinatorial options than there actually are. Verbosity has
> no effect when saving to a file or when doing "clear". The pre-action has
> no (visible) effect when loading from clipboard or a file.
That's the single most difficult thing about this UI. I can think of 20 useful actions.
- nothing, measure and show, non-verbose
- nothing, measure and show, verbose
- GC, measure and show, non-verbose
- GC, measure and show, verbose
- CC, measure and show, non-verbose
- CC, measure and show, verbose
- MMU, measure and show, non-verbose
- MMU, measure and show, verbose
- nothing, measure and save to file
- GC, measure and save to file
- CC, measure and save to file
- MMU, measure and save to file
- nothing, load from file and show, non-verbose
- nothing, load from file and show, verbose
- nothing, read from clipboard and show, non-verbose
- nothing, read from clipboard and show, verbose
- nothing, clear
- GC, clear
- CC, clear
- MMU, clear
And a few more that are possible but less useful (such as "GC, load from file
and show, verbose"). And we'll soon likely have another four for
"nothing/GC/CC/MMU, save strings to file".
Presenting all 20 (or 24) in a single list would avoid the
non-useful/impossible combinations, but would be overwhelming. Having three
drop-downs was the best I came up with, though I'd be happy to hear other
suggestions.
> > - Get rid of URL modifications.
>
> What's your motivation for doing this? I could imagine under this new
> scheme you select what you want and then are pushState'd to e.g.
>
> about:memory?verbose=1&preAction=foo&action=bar
>
> Then we could hand out those URLs instead of telling people which options to
> select.
The primary motivation is that using the URL feels like a hack, and (now) unnecessary duplication of function. If we're regularly telling people to use URLs other than "about:memory" then I think we've failed with making the UI nice. But if you have motivation involving automation, then I might be convinced -- e.g. I kept the "?file=" feature because I know you use it in your B2G script.

I could separate the GC/CC/MMU buttons again, which would leave eight possibilities for the "action":
- measure and show, non-verbose
- measure and show, verbose
- load from file and show, non-verbose
- load from file and show, verbose
- read from clipboard and show, non-verbose
- read from clipboard and show, verbose
- measure and save to file
- clear
Eight is still a lot for a single UI element such as a drop-down, though.

> If we're regularly telling people to use URLs other than "about:memory" then I think we've failed
> with making the UI nice.
Good point!
What if we get rid of "clear"? I'm having a hard time seeing why that's essential when you can simply close the tab.
Then we could organize the UI elements into groups:
1. GC/CC/MMU
2. Show {local,file,clipboard} {normal/verbose}
3. Save {memory reports, strings} to file
With this organization, we can make normal/verbose a separate UI element that applies to the choice of action.
If we used buttons for everything, this is only seven or eight buttons plus a checkbox, which doesn't seem unconscionable.
Perhaps we replace the GC/CC/MMU buttons with a "clear" button when you've loaded from file/clipboard. That way we don't imply that GC/CC/MMU has anything to do with what you're looking at, but you can still get back to the original screen.

> >+ function finish()
> >+ {
> >+ SimpleTest.finish();
> >+ }
>
> I don't understand the purpose of this function.
Do you mean, why not just use SimpleTest.finish() directly? Because it's cut-and-pasted from another test where finish() does some other stuff as well :) I'll remove it.

Created attachment 734946[details]
jlebar's suggested UI
This UI has no mystery meat and no no-effect combinations. But it just looks a bit crap, IMO :/ (And I haven't put in the verbosity checkbox yet, either.)
Suggestions?

Created attachment 734989[details][diff][review]
(part 1) - Improve about:memory's functional UI.
Updated version. jlebar, this just tweaks your design a bit. I'm still not happy with the amount of vertical space it takes at the top, while wasting horizontal space.

Created attachment 735613[details][diff][review]
(part 1) - Improve about:memory's functional UI.
Here's a tweaked version that uses |inline-block| for the three boxes at the top, so that they will take up as much horizontal space as possible.
I also had forgotten that mobile has a different aboutMemory.css file, so I changed it too, and put the desktop- and mobile-specific parts at the bottoms of the respective files.

I was going to suggest that you use min-height: -moz-max-available so that bubbles in the same row are all the same height. Unfortunately that apparently doesn't exist yet, so I give up.
I think something like the button labels I had in attachment 734952[details] are better than the button labels in this patch. Clicking a button does something, so it's good for button labels to be verbs. I also don't think people will read the "Current" button by reading "Show memory reports" first -- instead, they'll read "Current", be confused, and then looking around for an explanation.
I'm happy to change "Garbage Collect" and "Cycle Collect" to "GC" and "CC", but I think "Show local reports", "Read from file", and "Save reports to file" are better than what's in this patch.
Can we hide the div that says "Choose an action" when there are no reports to show? Also all of the stuff below that.
I think people will be confused that the "save reports" button doesn't save what's showing on the screen, if you press "save reports" after "current".
I wonder how much additional memory it would take up to remember the memory reporters' results, so we /could/ make "save reports" save what's onscreen. That would be cool...

Created attachment 737798[details][diff][review]
(part 1) - Improve about:memory's functional UI.
> I was going to suggest that you use min-height: -moz-max-available so that
> bubbles in the same row are all the same height. Unfortunately that
> apparently doesn't exist yet, so I give up.
I too tried and gave up.
> I think something like the button labels I had in attachment 734952[details]
> are better than the button labels in this patch.
You're right that verbs are good. Here's what I've done and why:
- I changed "Current" to "Measure". I find "local" in "show local reports"
entirely meaningless, and "Measure" indicates better than "show" that it's
doing actual measurement.
- I kept "Load" as is. I prefer "Load" to "Read" because it contrasts better
with "Save", and also distinguishes it better from "Read from clipboard". I
left off the "from file" because it's redundant coming after "Load".
- I changed "Save reports" to "Measure and save", (a) to hopefully clarify that
it's not saving any reports that are on the screen, and (b) because "reports"
isn't necessary.
- I changed the "Save memory info" label to "Save memory reports"; "info"
might be better once bug 852010 is implemented, but "reports" is better right
now.
- Added ellipses to "Load" and "Measure and save", because they trigger a
dialog box.
> Can we hide the div that says "Choose an action" when there are no reports to
> show? Also all of the stuff below that.
Done. One wrinkle is the "Troubleshooting information" link to about:support,
which now only shows when there are reports on the screen. Is that link really
necessary? I could just remove it.
> I wonder how much additional memory it would take up to remember the memory
> reporters' results, so we /could/ make "save reports" save what's onscreen.
> That would be cool...
It would be, but it's beyond the scope of this bug.

jlebar: review ping. Not sure if this slipped through the cracks or if you're just swamped with B2G stuff, or both... I'd really like to land this patch, and the one in bug 857382 that depends on this, before I go on vacation at the end of this week.

Comment on attachment 739339[details][diff][review]
(part 1) - Improve about:memory's functional UI.
kats, would you be able to review this? jlebar would normally do it but he's swamped with B2G blockers right now.

> I'm also a little uneasy about using gVerbose.checked everywhere because
> that is a dynamic value that can be changed by the user.
Interesting point. I guess it's conceivable that the user might check it while the page is generating. But it seems unlikely, and I was unable to do so even in a debug build (which is much slower than normal) -- the UI locks up and the checkbox doesn't respond until the page has finished generating.
The reason I moved to using |gVerbose.checked| everywhere instead of having a separate |gVerbose| variable is that it was easy to forget to check the checkbox and update |gVerbose| before generating the page -- I actually hit that problem a couple of times in earlier versions. So I'd rather take this approach, which fixes a problem I've seen in practice, and worry about the theoretical problem in the future. Make sense?
> There are codepaths that assume gFooter is defined
Nice catch. I'll check if gFooter is undefined before using it.
> Don't need two identical URLs here
That file will be removed in bug 857382. I'll leave it unchanged to avoid giving myself conflicts.

(In reply to Nicholas Nethercote [:njn] from comment #29)
> So I'd rather take this
> approach, which fixes a problem I've seen in practice, and worry about the
> theoretical problem in the future. Make sense?
Yup, sounds fine.
> > Don't need two identical URLs here
>
> That file will be removed in bug 857382. I'll leave it unchanged to avoid
> giving myself conflicts.
Ok.

(In reply to Nicholas Nethercote [:njn] from comment #37)
> > Are there any high risk areas QA should pay attention to as Firefox 23
> > enters Beta?
>
> Not really. Just testing all the buttons (esp. save and load) should
> suffice. Thanks!
Does the in-testsuite automation not cover that? I'd like to make sure manual QA spends as much time focusing on high risk code changes in Firefox and this doesn't sound like it necessarily qualifies as described.

> Does the in-testsuite automation not cover that?
The in-testsuite automation does cover that. I'm comfortable without additional manual testing, but feel free to do some if you think it would be useful.