Closed Bug 674158 Opened 8 years ago Closed 8 years ago

OOify aboutMemory.js

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
aboutMemory.js is written in a very imperative style.  This makes it hard to
read, particularly because there are three similar but different object
types used to represent different kinds of memory reporters.

This patch makes aboutMemory.js more object-oriented, which makes the object
types clearer.  This is preliminary work that will allow aboutMemory.js to
be made simpler and consume less memory (which perturbs the measurements).

This patch also increases the amount of sanity checking done, asserting
(throwing exceptions) in more cases.
Attachment #548365 - Flags: review?(dolske)
dolske: any idea when you'll have a chance to review?
Comment on attachment 548365 [details] [diff] [review]
patch

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

Sorry for the delay here.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +156,5 @@
> +      this._amount += r._amount;
> +    } else if (this._amount === kUnknown && r._amount !== kUnknown) {
> +      this._amount = r._amount;
> +    }
> +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;

Can r._nMerged ever be nonzero?

@@ +292,5 @@
> +
> +TreeNode.prototype = {
> +  findKid: function(aName) {
> +    for (var i = 0; i < this._kids.length; i++) {
> +      if (this._kids[i]._name === aName) {

I guess this is just rectoring, but why strict-equality here?
Attachment #548365 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #2)
> ::: toolkit/components/aboutmemory/content/aboutMemory.js
> @@ +156,5 @@
> > +      this._amount += r._amount;
> > +    } else if (this._amount === kUnknown && r._amount !== kUnknown) {
> > +      this._amount = r._amount;
> > +    }
> > +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;
> 
> Can r._nMerged ever be nonzero?

Yes, if there are 3 or more reporters with the same path.


> @@ +292,5 @@
> > +
> > +TreeNode.prototype = {
> > +  findKid: function(aName) {
> > +    for (var i = 0; i < this._kids.length; i++) {
> > +      if (this._kids[i]._name === aName) {
> 
> I guess this is just rectoring, but why strict-equality here?

I use it everywhere because Crockford recommends it and I think he's right on this one.
(In reply to Nicholas Nethercote [:njn] from comment #3)

> > > +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;
> > 
> > Can r._nMerged ever be nonzero?
> 
> Yes, if there are 3 or more reporters with the same path.

Hmm, this shouldn't this code take that into account? It's only looking at this._nMerged, not r._nMerged.

Maybe just call it ._numReporters (or whatever), have each init to 1 by default, and then blindly add them together here?
(In reply to Justin Dolske [:Dolske] from comment #4)
> (In reply to Nicholas Nethercote [:njn] from comment #3)
> 
> > > > +    this._nMerged = this._nMerged ? this._nMerged + 1 : 2;
> > > 
> > > Can r._nMerged ever be nonzero?
> > 
> > Yes, if there are 3 or more reporters with the same path.
> 
> Hmm, this shouldn't this code take that into account? It's only looking at
> this._nMerged, not r._nMerged.

Oh, I see.  Sorry, r._nMerged cannot be non-zero.  I'll add an assertion to that effect.
Merged:
http://hg.mozilla.org/mozilla-central/rev/48c58d9cc3f3
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.