1052052 - Ensure that cx pushing only happens within AutoJSAPI

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The work here is 98% done by the other dependencies of bug 951991. The last step is to stop using it in AutoJSContext, and move it into a private namespace.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8085c27f6acb

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist AutoCxPusher into ScriptSettings.h. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist Auto*JSContext into nsContentUtils and kill nsCxPusher.{cpp,h}. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Convert AutoJSContext to AutoJSAPI. v1

With this change, the only cx push left in the tree happens inside of AutoJSAPI. \o/

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=e7dd4933d0c0

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8471696 [details] [diff] [review]
Part 1 - Hoist AutoCxPusher into ScriptSettings.h. v1

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

::: dom/base/ScriptSettings.h
@@ +23,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +namespace danger {

Could you add here a one liner comment that is more explicit? Something like it's private, don't use it always use AutoJSAPI instead.

Comment 7

[Bobby Holley (:bholley)]

Note that I ended up moving AutoJSContext and friends to ScriptSettings.h instead, since I think that's really where they belong.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #7)
> Note that I ended up moving AutoJSContext and friends to ScriptSettings.h
> instead, since I think that's really where they belong.

Sounds reasonable to me. Although what would make even more sense to me if all these fundamental AutoJSAPI/cx handling related classes ended up in a sub-header with a more descriptive name, since they are not necessary related to scripts, it's just an init for any call into JSAPI. But I really don't have a strong opinion here, what do you think?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> (In reply to Bobby Holley (:bholley) from comment #7)
> > Note that I ended up moving AutoJSContext and friends to ScriptSettings.h
> > instead, since I think that's really where they belong.
> 
> Sounds reasonable to me. Although what would make even more sense to me if
> all these fundamental AutoJSAPI/cx handling related classes ended up in a
> sub-header with a more descriptive name, since they are not necessary
> related to scripts, it's just an init for any call into JSAPI. But I really
> don't have a strong opinion here, what do you think?

We might consider renaming ScriptSettings.h at some point, but it's not a huge priority IMO. Shuffling all these headers around has actually taken me a lot of time (because I need to iteratively push to try to uncover Gonk failures), so I'm not really eager to mess around with it too much more at this point.

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #9)
> failures), so I'm not really eager to mess around with it too much more at
> this point.

Sure, I know how painful it can be... so no pushing really, was just an idea.

Comment 11

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2a45c7531e60

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4783e9a173c1

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a9b8c346d295
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17a9673ed782
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1a3161f614

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G non-unified bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d41a9159e58

https://tbpl.mozilla.org/php/getParsedLog.php?id=45969081&tree=Mozilla-Inbound

Comment 15

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=d256ff971026

Comment 16

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74f2a2d657e3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55783b8e332c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c51223ffc9

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74f2a2d657e3
https://hg.mozilla.org/mozilla-central/rev/55783b8e332c
https://hg.mozilla.org/mozilla-central/rev/f9c51223ffc9