Last Comment Bug 763776 - refactor nsXPConnect::Traverse
: refactor nsXPConnect::Traverse
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks: 754495
  Show dependency treegraph
 
Reported: 2012-06-11 17:06 PDT by Andrew McCreight [:mccr8]
Modified: 2012-06-20 02:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: add redundant vars to enable outlining (1.18 KB, patch)
2012-06-15 11:49 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Review
part 2: split Traverse into multiple pieces (7.25 KB, patch)
2012-06-15 11:51 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Review
part 3: remove Cisms from GC thing traversal (4.63 KB, patch)
2012-06-15 11:52 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Review
part 4: minor fiddly cleanup (2.39 KB, patch)
2012-06-15 11:53 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Review
part 5: make * conform to JS style consistently (1.95 KB, patch)
2012-06-15 11:54 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Review
part 6: clazz to clasp (6.87 KB, patch)
2012-06-18 14:33 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Review

Description Andrew McCreight [:mccr8] 2012-06-11 17:06:35 PDT
This function (which traces through JS objects for the cycle collector) is pretty big, but there are a few separate steps it takes, so I'd like to break it up a bit.  There's probably also some other cleanup I can do at the same time.

This will also make it easier for me to alter its behavior for CompartmentParticipant in bug 754495.
Comment 1 Andrew McCreight [:mccr8] 2012-06-15 11:49:35 PDT
Created attachment 633608 [details] [diff] [review]
part 1: add redundant vars to enable outlining

This causes slightly more work, but only when we're logging.
Comment 2 Andrew McCreight [:mccr8] 2012-06-15 11:51:42 PDT
Created attachment 633610 [details] [diff] [review]
part 2: split Traverse into multiple pieces

This will be bit rotted by bug 616262, but only in a trivial way.
Comment 3 Andrew McCreight [:mccr8] 2012-06-15 11:52:58 PDT
Created attachment 633611 [details] [diff] [review]
part 3: remove Cisms from GC thing traversal
Comment 4 Andrew McCreight [:mccr8] 2012-06-15 11:53:26 PDT
Created attachment 633612 [details] [diff] [review]
part 4: minor fiddly cleanup
Comment 5 Andrew McCreight [:mccr8] 2012-06-15 11:54:28 PDT
Created attachment 633613 [details] [diff] [review]
part 5: make * conform to JS style consistently
Comment 6 Andrew McCreight [:mccr8] 2012-06-15 13:47:33 PDT
Comment on attachment 633608 [details] [diff] [review]
part 1: add redundant vars to enable outlining

Please review the rest of the patches, too, I just think there's no need to spam the bug by setting the review flag on every piece.
Comment 7 Andrew McCreight [:mccr8] 2012-06-15 17:10:01 PDT
I'll leave the main thread cleanup stuff for another patch, after XPC singlethreading lands.  Or maybe it will just fix it.  Either way.
Comment 8 Bill McCloskey (:billm) 2012-06-18 12:10:00 PDT
Comment on attachment 633608 [details] [diff] [review]
part 1: add redundant vars to enable outlining

Is this all you wanted me to review?

As an aside, I think it would be nice if we could replace uses of |clazz| in xpconnect with |clasp|, which is what we use in the JS engine.
Comment 9 Andrew McCreight [:mccr8] 2012-06-18 12:25:22 PDT
Sure, I can do that for the code I touch in these patches.
Comment 10 Andrew McCreight [:mccr8] 2012-06-18 14:33:01 PDT
Created attachment 634172 [details] [diff] [review]
part 6: clazz to clasp
Comment 11 Bill McCloskey (:billm) 2012-06-18 15:44:18 PDT
Comment on attachment 634172 [details] [diff] [review]
part 6: clazz to clasp

Excellent, thanks!
Comment 12 Andrew McCreight [:mccr8] 2012-06-19 11:36:24 PDT
Thanks for the reviews!

Try run looked good.  Landed the patch in one chunk.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9118e53405c7
Comment 13 Ed Morley [:emorley] 2012-06-20 02:20:31 PDT
https://hg.mozilla.org/mozilla-central/rev/9118e53405c7

Note You need to log in before you can comment on or make changes to this bug.