Closed Bug 401865 Opened 15 years ago Closed 15 years ago

nsMicrosummaryService.js cleanup

Categories

(Firefox Graveyard :: Microsummaries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 3 beta2

People

(Reporter: dao, Assigned: dao)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #286810 - Flags: review?(mano)
Comment on attachment 286810 [details] [diff] [review]
patch

>Index: browser/components/microsummaries/src/nsMicrosummaryService.js

> function ArrayEnumerator(aItems) {
>-  this._index = 0;
>-  this._contents = [];
>   if (aItems) {
>     for (var i = 0; i < aItems.length; ++i) {
>       if (!aItems[i])
>-        aItems.splice(i, 1);      
>+        aItems.splice(i--, 1);

Hmm, this code is duplicated a lot in the tree (http://lxr.mozilla.org/seamonkey/search?string=ion%20ArrayEnumerator), I suppose we should fix them all?
Oh, yes, and there's more that can and should be fixed in those implementations. I'll file a new bug.
Comment on attachment 286810 [details] [diff] [review]
patch

nit: please fix getService alignment.

r=mano.
Attachment #286810 - Flags: review?(mano) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #286810 - Attachment is obsolete: true
Comment on attachment 287773 [details] [diff] [review]
patch v2

there were quite a few getService calls aligned that way. I also changed two other minor things while I was at it, interdiff should be helpful here.
Attachment #287773 - Flags: review?(mano)
Comment on attachment 287773 [details] [diff] [review]
patch v2

Just land v1, sorry for my fonts-issue again ;)
Attachment #287773 - Flags: review?(mano)
Attached patch patch v2 (obsolete) — Splinter Review
The second patch didn't just change the indention, it also contained a fix for the getPref function where getPrefType was called twice for int prefs.
Attachment #287773 - Attachment is obsolete: true
Attachment #288386 - Flags: review?(mano)
Attached patch patch v2Splinter Review
wrong patch ...
Attachment #288386 - Attachment is obsolete: true
Attachment #288387 - Flags: review?(mano)
Attachment #288386 - Flags: review?(mano)
Comment on attachment 288387 [details] [diff] [review]
patch v2

r=mano
Attachment #288387 - Flags: review?(mano) → review+
Attachment #288387 - Flags: approval1.9?
What's the goal of the cleanup?   Are we good with ensuring this doesn't call leaks?
This is mostly code shifting (e.g. inlining of internal methods and getters that have been used only once), small performance optimizations (e.g. in getPref and LOG), and it fixes the wrongly implemented ArrayEnumerator constructor.
Comment on attachment 288387 [details] [diff] [review]
patch v2

k - thnx for the explaination..
Attachment #288387 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in browser/components/microsummaries/src/nsMicrosummaryService.js;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js,v  <--  nsMicrosummaryService.js
new revision: 1.81; previous revision: 1.80
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M10
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.