670084 - Separate content from presentation in about:memory

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Jez Ng [:int3]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110704 Firefox/6.0a2
Build ID: 20110704042003

Steps to reproduce:

I'm working on adding a graph to about:memory. The code that generates the memory tree is currently closely coupled to the code that creates the treeview, making it hard to add another view (the graph) without code duplication. Hence I've done some refactoring.

I'm filing this as a separate bug because the graph itself is not ready to land yet, but it would be nice if further changes to about:memory were done on this refactored version to save me the trouble of merging in the changes.

Comment 1

[Jez Ng [:int3]]

Attachment: Patch v1

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 544739 [details] [diff] [review]
Patch v1

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

You didn't ask for a review but I'm giving you r+ anyway, just because I'm feeling nice :)

Comment 3

[Nicholas Nethercote [inactive]]

Hmm, I wonder if this broke the threshold code.  See bug 472209 comment 7.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> Hmm, I wonder if this broke the threshold code.  See bug 472209 comment 7.

I've confirmed that it did.  The problem is the aT parameter you added to shouldOmit().  It needs to always be the root of the aT tree, but you've made it the root of the sub-tree being filtered.

test_aboutmemory.xul hopefully would have picked this up anyway...

Comment 5

[Jez Ng [:int3]]

Attachment: Patch v2

The threshold code works now, and the patch passes test_aboutmemory.xul. I've also factored out the code that populates the _description field from the buildTree function, since the graph doesn't need that information.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 546680 [details] [diff] [review]
Patch v2

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

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Unfortunately, this bitrotted already. Please also follow <http://www.google.com/url?sa=D&q=https://developer.mozilla.org/en/Mercurial_FAQ%23How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f> when using checkin-needed. Thanks!

Comment 8

[Nicholas Nethercote [inactive]]

Does separating the _description population help reduce the size of the sawtooth in the graph?  If not, separating it doesn't seem helpful.

Comment 9

[Jez Ng [:int3]]

No, it doesn't reduce the size of the sawtooth, but I still think it should be factored out from a design point of view, since it's not data that's relevant to the graph.

Comment 10

[Nicholas Nethercote [inactive]]

Jez, aboutMemory.js should be a little more stable right now.  If you can post a new patch and add "checkin-needed" to the whiteboard, I'll land it for you.

Comment 11

[Justin Lebar (not reading bugmail)]

Well, what's one more change?  Jez, I'm going to land bug 672731 on mozilla-inbound in just a minute.  It should get merged into m-c within 24 hours, but if you want to hack on this sooner, you should pull a m-i tree.  See [1] for making this quick and painless, and find me or njn on IRC if you have any problems.

[1] jlebar.com/2011/5/20/Faster_and_smaller_clones_of_branches.html

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to comment #11)
> Well, what's one more change?  Jez, I'm going to land bug 672731 on
> mozilla-inbound in just a minute.

I suspect that won't conflict with Jez's patch.  Fingers crossed.

Comment 13

[Jez Ng [:int3]]

I'm tackling some session restore stuff at the moment, so I'm in no hurry.. I'll wait till jlebar's patch lands on m-c.

Comment 14

[Justin Lebar (not reading bugmail)]

The change was just merged in, so you're good to go whenever.

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: patch, v3

I ended up doing this because I want to rework aboutMemory.js and it was blocking my progress.  Jeze, I removed addDescription(), which means that buildTree() will always add the descriptions.  I know you don't need descriptions for your graphs but addDescription() will just get in the way of my subsequent changes.  Hope this is ok;  reduced memory usage while generating about:memory is definitely in my plans.

Comment 16

[Jez Ng [:int3]]

Comment on attachment 548348 [details] [diff] [review]
patch, v3

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

Looks good to me (: Thanks for helping me with the refactoring!

Comment 17

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1d3b050ee50a

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/1d3b050ee50a