Separate content from presentation in about:memory

RESOLVED FIXED in mozilla8



7 years ago
7 years ago


(Reporter: int3, Assigned: int3)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [inbound])


(1 attachment, 2 obsolete attachments)



7 years ago
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

7 years ago
Created attachment 544739 [details] [diff] [review]
Patch v1


7 years ago
Blocks: 472209


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

Comment 5

7 years ago
Created attachment 546680 [details] [diff] [review]
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.
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+


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

Comment 9

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

Comment 13

7 years ago
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.
Created attachment 548348 [details] [diff] [review]
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.
Attachment #546680 - Attachment is obsolete: true
Attachment #548348 - Flags: review?(jezreel)

Comment 16

7 years ago
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+
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.