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

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

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

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 on attachment 546680 [details] [diff] [review] Patch v2 Review of attachment 546680 [details] [diff] [review]: -----------------------------------------------------------------

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!

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

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.

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.

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

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

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.

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

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