Closed Bug 1276310 Opened 4 years ago Closed 4 years ago

Merge XPCContext into XPCJSRuntime

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files)

I think once bug 1276276 is done, we can just do this.
Need to remember to ask Bobby whether there are any issues he sees with this, but he's not accepting needinfo right now.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Seems fine to me.
Flags: needinfo?(bobbyholley)
Depends on: 1281276
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
The member is only set by SetException(), which is called with non-null in
precisely one place.  This place is always immediately followed by a call to
nsXPCWrappedJSClass::CheckForException which proceeds to grab the exception from
the XPCContext and call SetException(null).  So basically, it's all a weird
out-of-band argument to CheckForException.
Attachment #8764105 - Flags: review?(bobbyholley)
The XPCShellImpl change is needed because we're now unifying XPCShellImpl with
some file that does "using namespace xpc" at toplevel, so the "Atob" and "Btoa"
barewords in its JSFunctionSpecs become ambiguous.  Luckily, the file-static
versions of those in XPCShellImpl are completely identical to the xpc-namespaced
versions anyway.
Attachment #8764108 - Flags: review?(bobbyholley)
Attachment #8764103 - Flags: review?(bobbyholley) → review+
Comment on attachment 8764104 [details] [diff] [review]
part 2.  Get rid of Components.lastResult and the corresponding XPCContext member

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

\o/
Attachment #8764104 - Flags: review?(bobbyholley) → review+
Attachment #8764105 - Flags: review?(bobbyholley) → review+
Comment on attachment 8764107 [details] [diff] [review]
part 4.  Store an XPCJSRuntime* in XPCCallContext instead of storing an XPCContext*

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

This is all a bit overblown at this point but I'm fine with leaving it for now.
Attachment #8764107 - Flags: review?(bobbyholley) → review+
Comment on attachment 8764108 [details] [diff] [review]
part 5.  Remove the now-unused XPCContext

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

::: js/xpconnect/src/XPCContext.cpp
@@ -18,5 @@
> -        mJSContext(aJSContext)
> -{
> -    MOZ_COUNT_CTOR(XPCContext);
> -
> -    MOZ_ASSERT(!JS_GetSecondContextPrivate(mJSContext), "Must be null");

Can you file a followup to remove the second context private from JSAPI?
Attachment #8764108 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (busy) from comment #10)
> Can you file a followup to remove the second context private from JSAPI?

We can remove both context privates actually - I landed a patch that removes the context private from the JS shell. Removing both will help the JSContext/JSRuntime refactoring.
Blocks: 1281582
Filed bug 1281582 on removing JSContext privates.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa060d1404ad
part 1.  Move XPCContext's mPendingResult member into XPCJSRuntime.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d3a8915a3a
part 2.  Get rid of Components.lastResult and the corresponding XPCContext member.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a59ed6a08cb8
part 3.  Get rid of the mException member of XPCContext.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/15159f86c546
part 4.  Store an XPCJSRuntime* in XPCCallContext instead of storing an XPCContext*.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbb064a7bcb
part 5.  Remove the now-unused XPCContext.  r=bholley
You need to log in before you can comment on or make changes to this bug.