Ensure that cx pushing only happens within AutoJSAPI

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 4

5 years ago
With this change, the only cx push left in the tree happens inside of AutoJSAPI. \o/
Attachment #8471698 - Flags: review?(gkrizsanits)
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.
Attachment #8471696 - Flags: review?(gkrizsanits) → review+
Attachment #8471697 - Flags: review?(gkrizsanits) → review+
Attachment #8471698 - Flags: review?(gkrizsanits) → review+
Assignee

Comment 7

5 years ago
Note that I ended up moving AutoJSContext and friends to ScriptSettings.h instead, since I think that's really where they belong.
(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?
Assignee

Comment 9

5 years ago
(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.
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.