Closed Bug 670084 Opened 12 years ago Closed 12 years ago

Separate content from presentation in about:memory


(Toolkit :: about:memory, defect)

Not set





(Reporter: int3, Assigned: int3)



(Whiteboard: [inbound])


(1 file, 2 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
Blocks: 472209
Component: General → about:memory
Ever confirmed: true
OS: Other → All
Product: Firefox → Toolkit
QA Contact: general → about.memory
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 :)
Attachment #544739 - Flags: review+
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...
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #544739 - Attachment is obsolete: true
Attachment #546680 - Flags: review?(nnethercote)
Comment on attachment 546680 [details] [diff] [review]
Patch v2

Review of attachment 546680 [details] [diff] [review]:
Attachment #546680 - Flags: review?(nnethercote) → review+
Keywords: checkin-needed
Unfortunately, this bitrotted already. Please also follow <> when using checkin-needed. Thanks!
Assignee: nobody → jezreel
Keywords: checkin-needed
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.

(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.
Attached patch patch, v3Splinter Review
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.
Attachment #546680 - Attachment is obsolete: true
Attachment #548348 - Flags: review?(jezreel)
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!
Attachment #548348 - Flags: review?(jezreel) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.