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

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in IDBRequest::NotifyHelperCompleted v1

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.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace AutoPushJSContext in MobileMessageCallback::NotifySuccess v1

Just a WrapNative here, so should be straight forward.

I think the new API is working well.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace AutoPushJSContext in MobileMessageCallback::NotifyMessageDeleted v1

A set element on a newly created array, so no legacy error reporting required here I believe.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace AutoPushJSContext in MobileMessageCursorCallback::NotifyCursorSuccess v1

Very similar to Part 2.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in nsNPAPIPlugin _getpluginelement v1

Fairly straight forward I think.

GetNewOrUsed only seems to care about the runtime.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace AutoPushJSContext in nsMenuX::MenuConstruct v1

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

Comment 7

[Bobby Holley (:bholley)]

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".

Comment 8

[Bobby Holley (:bholley)]

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!

Comment 9

[Bob Owen (:bobowen)]

(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. :-)

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in IDBRequest::NotifyHelperCompleted v2

r=bholley - from comment 7.

Changed to form of initialise with a _zed_. :-)

Comment 11

[Bob Owen (:bobowen)]

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.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/647fe3ad1a64
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5522a7bb611
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b97d8bd9ab7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df28a790773
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a360bd6e398
https://hg.mozilla.org/integration/mozilla-inbound/rev/679fa12ebb18

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/647fe3ad1a64
https://hg.mozilla.org/mozilla-central/rev/e5522a7bb611
https://hg.mozilla.org/mozilla-central/rev/9b97d8bd9ab7
https://hg.mozilla.org/mozilla-central/rev/7df28a790773
https://hg.mozilla.org/mozilla-central/rev/9a360bd6e398
https://hg.mozilla.org/mozilla-central/rev/679fa12ebb18