856917 - Improve about:memory's functional UI

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: 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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: 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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add a test for "?file=" loading in about:memory.

While in the neighbourhood, I thought I'd add some testing of the "?file="
loading.

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[Justin Lebar (not reading bugmail)]

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 6

[Justin Lebar (not reading bugmail)]

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.

Comment 7

[Justin Lebar (not reading bugmail)]

Needs the patches from bug 848560 to apply cleanly; I have no idea if they're actually needed.

Sometimes I really wish we all used git...

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

[Nicholas Nethercote [inactive]]

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

Comment 10

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Justin Lebar (not reading bugmail)]

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

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 733737 [details] [diff] [review]
(part 1) - Improve about:memory's functional UI.

Clearing the r? to reflect that we're still talking about this, not as a synonym for r-.

Comment 13

[Nicholas Nethercote [inactive]]

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

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: 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?

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: How about this UI?

Comment 16

[Nicholas Nethercote [inactive]]

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

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: screenshot

Comment 18

[Justin Lebar (not reading bugmail)]

> I'm still not happy with the amount of vertical space it takes at the top, while wasting horizontal 
> space.

We could move it under the main <div> when we're showing reports?

Comment 19

[Nicholas Nethercote [inactive]]

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

Comment 20

[Justin Lebar (not reading bugmail)]

Nit: Whitespace errors.  /dev/fd/63 is the patch file.

$ git apply <(pbpaste)
/dev/fd/63:456: trailing whitespace.
  }); 
/dev/fd/63:1137: trailing whitespace.
        
/dev/fd/63:155: new blank line at EOF.
+
/dev/fd/63:270: new blank line at EOF.
+
warning: 4 lines add whitespace errors.

Comment 21

[Justin Lebar (not reading bugmail)]

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

Comment 22

[Nicholas Nethercote [inactive]]

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

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: screenshot

New screenshot.

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Improve about:memory's functional UI.

Minor changes over the last version, relating to the handling of the "verbose"
checkbox.

Comment 25

[Nicholas Nethercote [inactive]]

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 26

[Justin Lebar (not reading bugmail)]

Sorry to keep you waiting.  I was at a work week last week, but I'll look over this code tomorrow.

Comment 27

[Nicholas Nethercote [inactive]]

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.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 739339 [details] [diff] [review]
(part 1) - Improve about:memory's functional UI.

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

Looks pretty good. There's enough things below that I'd like to see an updated patch, but I'm not expecting a lot of changes.

I'm also a little uneasy about using gVerbose.checked everywhere because that is a dynamic value that can be changed by the user. It doesn't look like any of the uses of gVerbose.checked happen once the page is all set up, but if the code changes in the future to e.g. call formatBytes when the user expands a node, then you could end up with some of the output being in verbose mode and other being in non-verbose mode. I'm not sure if it's worth worrying about here though.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +263,4 @@
>  {
> +  let tmp = gMain.cloneNode(false);
> +  gMain.parentNode.replaceChild(tmp, gMain);
> +  gMain = tmp;

Please add a comment explaining that this is to empty out the node. It was not obvious to me at first. Alternatively, you could write it as

while (gMain.firstChild) {
  gMain.removeChild(gMain.firstChild);
}

@@ +274,5 @@
> +
> +  if (aMsg) {
> +    let className = "section";
> +    if (aClassName) {
> +      className = className + " badInputWarning";

s/" badInputWarning"/aClassName/

@@ +283,5 @@
> +  switch (aFooterAction) {
> +   case HIDE_FOOTER:   gFooter.classList.add('hidden');    break;
> +   case SHOW_FOOTER:   gFooter.classList.remove('hidden'); break;
> +   case IGNORE_FOOTER:                                     break;
> +   default:            assertInput(false, "bad units in TreeNode.toString");

This error message seems inappropriate.

@@ +292,5 @@
> +{
> +  gFooter.classList.add('hidden');
> +}
> +
> +function showFooter()

Doesn't look like these showFooter/hideFooter functions are called anywhere. Take them out?

@@ +412,5 @@
> +function appendButton(aP, aTitle, aOnClick, aText, aId)
> +{
> +  let b = appendElementWithText(aP, "button", "", aText);
> +  b.title = aTitle;
> +  b.onclick = aOnClick

Missing trailing semi-colon. For consistency if nothing else, I guess.

@@ +432,5 @@
> +  input.addEventListener("change", function() {
> +    let file = this.files[0];
> +    let filename = file.mozFullPath;
> +    updateAboutMemoryFromFile(filename);
> +  }); 

Trailing whitespace

@@ +465,5 @@
> +  const kEllipsis = "\u2026";
> +
> +  // The "measureButton" id is used for testing.
> +  appendButton(row1, CuDesc, doMeasure, "Measure", "measureButton");
> +  appendButton(row1, LdDesc, () => input.click(), "Load" + kEllipsis);

Rename "input" to e.g. "filePickerInput" up above so this line is more clear.

@@ +501,5 @@
> +  let legendText2 = "Hover the pointer over the name of a memory report " +
> +                    "to see a description of what it measures.";
> +
> +  appendElementWithText(gFooter, "div", "legend", legendText1);
> +  appendElementWithText(gFooter, "div", "legend hiddenOnMobile", legendText2);

On some "mobile" devices (e.g. the Ouya game console) there is a mouse-cursor-like input that does allow you to hover over things. I'm not entirely sure if we have fully hooked up hover support in Fennec but it's something to keep in mind. No change needed to this patch for now.

@@ +1741,5 @@
>  function onLoadAboutCompartments()
>  {
> +  // Generate the main div, where content will go.  about:compartments doesn't
> +  // have a header or footer.
> +  gMain = appendElement(document.body, 'div', 'section');

There are codepaths that assume gFooter is defined (e.g. updateAboutCompartments calls handleException which calls updateMainAndFooter with HIDE_FOOTER). So I would either add a if (gFooter) guard in updateMainAndFooter, or just create a dummy gFooter element here.

::: toolkit/components/aboutmemory/tools/diff-memory-reports.js
@@ +6,5 @@
>  
>  //---------------------------------------------------------------------------
>  // This script diffs two memory report dumps produced by about:memory.  Run it
>  // using a JS shell.  View the result in about:memory using its diff mode, e.g.
> +// visit "about:memory?diff" or "about:memory?diff".

Don't need two identical URLs here

Comment 29

[Nicholas Nethercote [inactive]]

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

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Improve about:memory's functional UI.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 741115 [details] [diff] [review]
(part 1) - Improve about:memory's functional UI.

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +285,5 @@
> +    switch (aFooterAction) {
> +     case HIDE_FOOTER:   gFooter.classList.add('hidden');    break;
> +     case SHOW_FOOTER:   gFooter.classList.remove('hidden'); break;
> +     case IGNORE_FOOTER:                                     break;
> +     default: assertInput(false, "bad footer action in TreeNode.toString");

s/TreeNode.toString/updateMainAndFooter/

Comment 32

[Kartikaya Gupta (email:[e-mail])]

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

Comment 33

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2c39985587
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5977e8d52f

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b2c39985587
https://hg.mozilla.org/mozilla-central/rev/1c5977e8d52f

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 36

[u279076]

Are there any high risk areas QA should pay attention to as Firefox 23 enters Beta?

Comment 37

[Nicholas Nethercote [inactive]]

> 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!

Comment 38

[u279076]

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

Comment 39

[Nicholas Nethercote [inactive]]

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

Comment 40

[u279076]

Thanks Nicholas. I'm going to deprioritize this for QA. Please add the qawanted keyword if there's something high risk that comes to mind needing testing here.