723402 - Auto-expand sub-trees containing invalid values in about:memory

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 698930 added highlighting of bogus values in about:memory.  This has already shown its use (bug 723324, bug 723088).

One minor shortcoming is that bogus values are sometimes hidden by default in about:memory, esp. when they're negative, and you have to do some clicking to expose them.  It would be nice if sub-trees containing bogus values were auto-expanded.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

The patch is a little fiddly, but straightforward.

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 594877 [details] [diff] [review]
patch

Hm, so if there's an error in a memory reporter and then we update(), if the error goes away, the node will be hidden again.  I guess that's OK.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 594877 [details] [diff] [review]
patch

>+function assertClassName(e, className) {
>+  assert(e, "undefined " + className);
>+  assert(e.classList.contains(className), "classname isn't " + className);
>+}

Maybe assertHasClass() or assertElementHasClass() would be a better name.  I'd
fix up the second assertion's error message, too (s/isn't/doesn't contain/, or
something).

>+function expandPathToThisElement(aElement)
>+{
>+  if (aElement.classList.contains("kids")) {
>+    // Unhide the kids.
>+    aElement.classList.remove("hidden");
>+    expandPathToThisElement(aElement.previousSibling);  // hasKids
>+
>+  } else if (aElement.classList.contains("hasKids")) {
>+    // Unhide the '--' separator and hide the '++' separator.
>+    var  plusSpan = aElement.childNodes[2];
>+    var minusSpan = aElement.childNodes[3];
>+    assertClassName(plusSpan,  "mrSep");
>+    assertClassName(minusSpan, "mrSep");
>+    plusSpan.classList.add("hidden");
>+    minusSpan.classList.remove("hidden");
>+    expandPathToThisElement(aElement.parentNode);       // kids or pre.tree
>+
>+  } else {
>+    assertClassName(aElement, "tree");
>+  }
>+}

I guess it's OK that unhidden nodes don't count as toggle()'d.  It's bit weird,
though, since if you refreshed and the memory reporter became valid, you'd want
the path to it to remain expanded, so you could see the new value.  Up to you
if you want to change it.

Comment 4

[Nicholas Nethercote [inactive]]

> I guess it's OK that unhidden nodes don't count as toggle()'d.  It's bit
> weird,
> though, since if you refreshed and the memory reporter became valid, you'd
> want
> the path to it to remain expanded, so you could see the new value.  Up to you
> if you want to change it.

Good point.  It's a little painful to do here.  It should be easier once bug 724863 is fixed, I'll add a reminder to myself to do it there.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4e640fb4d8

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c4e640fb4d8