Closed Bug 1107681 Opened 8 years ago Closed 8 years ago

fix up the dom uses of WrapptedJSToDictionary to use the cx-less interface


(Core :: DOM: Device Interfaces, defect)

Not set





(Reporter: huseby, Assigned: huseby)




(1 file, 2 obsolete files)

There are a number of places in the dom code where we handle Observe events that contain wrapped JSObjects that get converted to Dictionaries.  With a better cx-less WrappedJSToDictionary function in bug 1107673 all of that code should be cleaned up to use the new function.
Attached patch Bug_1107681.patch (obsolete) — Splinter Review
fixes up all of the uses of WrappedJSToDictionary in the dom code.
Attachment #8541032 - Flags: review?(bobbyholley)
try pass:
The try pass is clean.  The only failures are failures in the test framework.  Re-running to see if they go away.
Comment on attachment 8541032 [details] [diff] [review]

Review of attachment 8541032 [details] [diff] [review]:

This looks great! However, I have a few ergonomic changes I'd like to see:

* I don't want to define a local called "cx" here, because that might lead somebody to accidentally using it for non-rooting purposes. So the call should be inlined directly into the rooting constructor.

* Given that this seems to be used frequently, I think it's worth introducing a call that indicates that this is explicitly for rooting, and shaves off a few characters to boot.

How about nsContentUtils::RootingCX() and nsContent::ThreadsafeRootingCX() which would be inline aliases for GetSafeJSContext and ThreadsafeGetSafeJSContext respectively?

Attachment #8541032 - Flags: review?(bobbyholley)
Attached patch Bug_1107681.patch (obsolete) — Splinter Review
updated to make review changes.

try pass looks clean:
Attachment #8541032 - Attachment is obsolete: true
Attachment #8541861 - Flags: review?(bobbyholley)
Comment on attachment 8541861 [details] [diff] [review]

Review of attachment 8541861 [details] [diff] [review]:

This looks great, assuming all of this stuff runs on the main thread. r=me

Thanks for doing this!
Attachment #8541861 - Flags: review?(bobbyholley) → review+
Keywords: checkin-needed
Keywords: checkin-needed
hold off for a moment while I double check that all of these are on the main thread.
Alright, I went through and every place I couldn't guarantee that we were executing on the main thread, I changed the call to RootingCxForThread().  The try pass is here:  If that comes back clean, then this can be checked in.

FYI, RootingCxForThread is an alias for GetDefaultJSContextForThread which returns RootingCx if on the main thread otherwise it returns the current thread JS context.
Attachment #8541861 - Attachment is obsolete: true
Ok, the try pass is clean.  This can go in.
Keywords: checkin-needed
Blocks: 1114667
Closed: 8 years ago
Resolution: --- → FIXED
Dave, stuff generally only gets resolved when it merges to mozilla-central (and it happens automatically).
Flags: needinfo?(huseby)
k thanks.
Flags: needinfo?(huseby)
You need to log in before you can comment on or make changes to this bug.