Closed Bug 1029494 Opened 6 years ago Closed 6 years ago

Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 7

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(6 files, 1 obsolete file)

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
This one is a little complicated because the context returned by |GetJSContext()| and the JSObject* returned by |GetParentObject()| is dependent on whether we have an |mScriptOwner| or a window owner.
This is coupled with the fact that |GetSuccessResult()| could be from 1 of 23 different classes. These seem to mainly use WrapNative or some form of reading structured clones, but without analysing all of them the easiest thing is to go for the legacy reporting root when we have a window owner.

If you have some insight into this and you think we can get rid of the legacy reporting, this could be simplified, although I'd re-instate the GetParentObject function.
Attachment #8446127 - Flags: review?(bobbyholley)
Just a WrapNative here, so should be straight forward.

I think the new API is working well.
Attachment #8446152 - Flags: review?(bobbyholley)
A set element on a newly created array, so no legacy error reporting required here I believe.
Attachment #8446251 - Flags: review?(bobbyholley)
Very similar to Part 2.
Attachment #8446423 - Flags: review?(bobbyholley)
Fairly straight forward I think.

GetNewOrUsed only seems to care about the runtime.
Attachment #8446458 - Flags: review?(bobbyholley)
Just a WrapNative, and this seemed to be a neater way to get the global.

Try push with all of these:
https://tbpl.mozilla.org/?tree=Try&rev=1f3b7509d94d

And another with the change to not hold the return value from GetScriptOwner() in Part 1, as it was causing a rooting hazard to be flagged:
https://tbpl.mozilla.org/?tree=Try&rev=3f09811cd274
Attachment #8446464 - Flags: review?(bobbyholley)
Comment on attachment 8446127 [details] [diff] [review]
Part 1: Replace AutoPushJSContext in IDBRequest::NotifyHelperCompleted v1

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

::: dom/indexedDB/IDBRequest.cpp
@@ +196,5 @@
> +    // the script owner's compartment.
> +    jsapi.Init();
> +    ac.construct(jsapi.cx(), GetScriptOwner());
> +  } else {
> +    // Otherwise our owner is a window and we use that to initialise.

Nit - en-US spelling of "initialize".
Attachment #8446127 - Flags: review?(bobbyholley) → review+
Attachment #8446152 - Flags: review?(bobbyholley) → review+
Attachment #8446251 - Flags: review?(bobbyholley) → review+
Attachment #8446423 - Flags: review?(bobbyholley) → review+
Attachment #8446458 - Flags: review?(bobbyholley) → review+
Comment on attachment 8446464 [details] [diff] [review]
Part 6: Replace AutoPushJSContext in nsMenuX::MenuConstruct v1

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

This is awesome work. Thanks Bob!
Attachment #8446464 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (PTO 6/25-6/29) from comment #8)
> Comment on attachment 8446464 [details] [diff] [review]
> Part 6: Replace AutoPushJSContext in nsMenuX::MenuConstruct v1
> 
> Review of attachment 8446464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is awesome work. Thanks Bob!

Thanks Bobby and thanks for all the reviews.

> > +    // Otherwise our owner is a window and we use that to initialise.
> 
> Nit - en-US spelling of "initialize".

OK, I'll try to remember to Americanise things ... I suppose ... although I already die a little inside every time I'm forced to write GetWrapperPreserveColor. :-)
r=bholley - from comment 7.

Changed to form of initialise with a _zed_. :-)
Attachment #8446127 - Attachment is obsolete: true
Attachment #8448598 - Flags: review+
Original try push:
https://tbpl.mozilla.org/?tree=Try&rev=1f3b7509d94d

And another with the rooting hazard in Part 1 fixed:
https://tbpl.mozilla.org/?tree=Try&rev=3f09811cd274

Landing in part number order please.
Thanks.
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.