Closed
Bug 1037904
Opened 10 years ago
Closed 10 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 10
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(4 files, 6 obsolete files)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc. See bug 951991 for details.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=dac42309f90b
Assignee: nobody → bobowencode
Assignee | ||
Comment 2•10 years ago
|
||
Given the AutoJSExceptionReporter seems like this needs the legacy error reporting. I've just split the GetJSContext function, so we can get the intermediate object for our purposes.
Attachment #8456183 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 3•10 years ago
|
||
Very similar to part 1.
Attachment #8456186 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•10 years ago
|
||
Only slight concern here was whether JS_GetPropertyById could cause script to run.
Attachment #8456188 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•10 years ago
|
||
Same concern over JS_SetPropertyById possibly running script?
Attachment #8456191 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 6•10 years ago
|
||
Variation on a theme. :)
Attachment #8456192 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 7•10 years ago
|
||
Last one in the set (apart from the one in doInvoke, which would seem to point to an AutoEntryScript, so I've left it for now).
Attachment #8456194 -
Flags: review?(bobbyholley)
Comment 8•10 years ago
|
||
Comment on attachment 8456183 [details] [diff] [review] Part 1: Replace nsCxPusher in nsJSObjWrapper::NP_HasMethod v1 Review of attachment 8456183 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsJSNPRuntime.cpp @@ +292,5 @@ > namespace plugins { > namespace parent { > > +static nsIScriptGlobalObject* > +GetScriptGlobalObject(NPP npp) Please have this function return an nsIGlobalObject and call it GetGlobalObject. nsIScriptGlobalObject is deprecated.
Attachment #8456183 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8456186 -
Flags: review?(bobbyholley) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8456188 [details] [diff] [review] Part 3: Replace nsCxPusher in nsJSObjWrapper::NP_GetProperty v1 Review of attachment 8456188 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I think we need an AutoEntryScript here. :-(
Attachment #8456188 -
Flags: review?(bobbyholley) → review-
Comment 10•10 years ago
|
||
Comment on attachment 8456191 [details] [diff] [review] Part 4: Replace nsCxPusher in nsJSObjWrapper::NP_SetProperty v1 Review of attachment 8456191 [details] [diff] [review]: ----------------------------------------------------------------- And here. I'm guessing that there is plugin code that relies on triggering getters/setters.
Attachment #8456191 -
Flags: review?(bobbyholley) → review-
Comment 11•10 years ago
|
||
Comment on attachment 8456192 [details] [diff] [review] Part 5: Replace nsCxPusher in nsJSObjWrapper::NP_RemoveProperty v1 Review of attachment 8456192 [details] [diff] [review]: ----------------------------------------------------------------- The rest of these would only trigger script in the case of a proxy, which ye olde NPAPI plugins are unlikely to depend on.
Attachment #8456192 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8456194 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8) Thanks for all the reviews. > Please have this function return an nsIGlobalObject and call it > GetGlobalObject. nsIScriptGlobalObject is deprecated. Well I needed an nsIScriptGlobalObject for the GetJSContext, so do you just want me to add another do_QueryInterface to the top of that? That doesn't seem to gain us much. (In reply to Bobby Holley (:bholley) from comment #9) > Yeah, I think we need an AutoEntryScript here. :-( OK, I'll obsolete Parts 3 and 4 and leave them until after Bug 1027553.
Flags: needinfo?(bobbyholley)
Comment 13•10 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #12) > (In reply to Bobby Holley (:bholley) from comment #8) > > Thanks for all the reviews. > > > Please have this function return an nsIGlobalObject and call it > > GetGlobalObject. nsIScriptGlobalObject is deprecated. > > Well I needed an nsIScriptGlobalObject for the GetJSContext, so do you just > want me to add another do_QueryInterface to the top of that? > That doesn't seem to gain us much. Ah ok. GetJSContext will go away at some point, right? Anyway, I don't care all that much. Do what makes sense.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #13) > Ah ok. GetJSContext will go away at some point, right? Anyway, I don't care > all that much. Do what makes sense. Yeah, sorry being dim. It means we won't need to touch GetGlobalObject when we do that, I'll sort it.
Assignee | ||
Updated•10 years ago
|
Attachment #8456188 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8456191 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8) > > +static nsIScriptGlobalObject* > > +GetScriptGlobalObject(NPP npp) > > Please have this function return an nsIGlobalObject and call it > GetGlobalObject. nsIScriptGlobalObject is deprecated. I've changed this and I've used |doc->GetScopeObject();| to get the nsIGlobalObject*. From debugging it looks like this gives us the inner window, whereas before we were getting the outer window. I think the inner window is actually what we want for AutoJSAPI and as |GetContext()| (used in |GetJSContext|) delegates to the outer window the result there should be the same. Sorry, to r? again, but I wanted to make sure that this made sense for plugins.
Attachment #8456183 -
Attachment is obsolete: true
Attachment #8457934 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 16•10 years ago
|
||
r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1037904#h8 GetScriptGlobalObject changed to GetGlobalObject.
Attachment #8456186 -
Attachment is obsolete: true
Attachment #8457935 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
r=bholley - from comment 11 GetScriptGlobalObject changed to GetGlobalObject.
Attachment #8456192 -
Attachment is obsolete: true
Attachment #8457936 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1037904#h12 GetScriptGlobalObject changed to GetGlobalObject.
Attachment #8456194 -
Attachment is obsolete: true
Attachment #8457937 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Seems like Bugzilla doesn't pick up the fragment identifier. Try push with the latest patches: https://tbpl.mozilla.org/?tree=Try&rev=70ca3f151742
Comment 20•10 years ago
|
||
Comment on attachment 8457934 [details] [diff] [review] Part 1: Replace nsCxPusher in nsJSObjWrapper::NP_HasMethod v2 Review of attachment 8457934 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Bob Owen (:bobowen) from comment #15) > From debugging it looks like this gives us the inner window, whereas before > we were getting the outer window. > > I think the inner window is actually what we want for AutoJSAPI and as > |GetContext()| (used in |GetJSContext|) delegates to the outer window the > result there should be the same. > > Sorry, to r? again, but I wanted to make sure that this made sense for > plugins. Yeah, I think we want the inner that's actually associated with the document here.
Attachment #8457934 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=70ca3f151742 Landing in part number order, patches 3 and 4 have been obsoleted. Thanks.
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/632730473b53 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7954ed4d0bd https://hg.mozilla.org/integration/mozilla-inbound/rev/53d7be299557 https://hg.mozilla.org/integration/mozilla-inbound/rev/9701d7726c62
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/632730473b53 https://hg.mozilla.org/mozilla-central/rev/b7954ed4d0bd https://hg.mozilla.org/mozilla-central/rev/53d7be299557 https://hg.mozilla.org/mozilla-central/rev/9701d7726c62
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•