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

RESOLVED FIXED in mozilla33

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

2.15 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
1.59 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
2.38 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
2.63 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
2.71 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
4.48 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
(Assignee)

Comment 1

3 years ago
Created attachment 8446127 [details] [diff] [review]
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.
Attachment #8446127 - Flags: review?(bobbyholley)
(Assignee)

Comment 2

3 years ago
Created attachment 8446152 [details] [diff] [review]
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.
Attachment #8446152 - Flags: review?(bobbyholley)
(Assignee)

Comment 3

3 years ago
Created attachment 8446251 [details] [diff] [review]
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.
Attachment #8446251 - Flags: review?(bobbyholley)
(Assignee)

Comment 4

3 years ago
Created attachment 8446423 [details] [diff] [review]
Part 4: Replace AutoPushJSContext in MobileMessageCursorCallback::NotifyCursorSuccess v1

Very similar to Part 2.
Attachment #8446423 - Flags: review?(bobbyholley)
(Assignee)

Comment 5

3 years ago
Created attachment 8446458 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in nsNPAPIPlugin _getpluginelement v1

Fairly straight forward I think.

GetNewOrUsed only seems to care about the runtime.
Attachment #8446458 - Flags: review?(bobbyholley)
(Assignee)

Comment 6

3 years ago
Created attachment 8446464 [details] [diff] [review]
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
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+
(Assignee)

Comment 9

3 years ago
(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. :-)
(Assignee)

Comment 10

3 years ago
Created attachment 8448598 [details] [diff] [review]
Part 1: Replace AutoPushJSContext in IDBRequest::NotifyHelperCompleted v2

r=bholley - from comment 7.

Changed to form of initialise with a _zed_. :-)
Attachment #8446127 - Attachment is obsolete: true
Attachment #8448598 - Flags: review+
(Assignee)

Comment 11

3 years ago
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
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
Keywords: checkin-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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.