Closed
Bug 408539
Opened 17 years ago
Closed 16 years ago
Storing XPCContext inside JSContext
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 3 obsolete files)
95.16 KB,
patch
|
igor
:
review+
igor
:
superreview+
|
Details | Diff | Splinter Review |
Currently xpconnect uses a hash table with explicit synchronization calls to update it to maintain the mapping between XPCContext and JSContext. If instead XPCContext would be stored directly in JSContext, the map and the explicit update code would be unnecessary leading to smaller and faster code.
Assignee | ||
Comment 1•17 years ago
|
||
A prototype implementation. Ideally to store XPCContext the code should use context-private JSContext.data accessible via public API. But that is taken by the DOM branch callback to store the closure data. There are 2 possibilities to handle that. The first one is to store the branch callback closure in XPCConnect itself. That would mean that DOM must be aware about XPCConnect or at least some class that XPCConnect subclasses that stores the closure. The second possibility is to add to JSContext something like branchCallbackClosure and use that in the DOM branch callback, not the context-private. For me the first solution sounds better, but I am not sure if exposure of DOM to xpconnect implementation details is a good thing to do. For now the patch just adds JSContext.data2 as a hack pending the resolution of the above issues.
Assignee | ||
Comment 2•16 years ago
|
||
The patch uses the callback to always create XPCContext instance. Compared with the previous version the instance is created eagerly to avoid the problems with lazy creation. The current code pretty much assumes that js->xpc mapping is infallible (it is enforced with asserts) and the patch finally provides the guarantee. Another thing is that I moved the JS runtime initialization to XPJSRuntime constructor. It allowed to simplify the code. that exists in the current code
Attachment #293365 -
Attachment is obsolete: true
Attachment #342440 -
Flags: review?(mrbkap)
Assignee | ||
Comment 3•16 years ago
|
||
to mrbkap: do you have time to review the patch?
Assignee | ||
Comment 4•16 years ago
|
||
The new version fixes the context callback initialization issue in xpcshell.
Attachment #342591 -
Flags: review?(mrbkap)
Assignee | ||
Updated•16 years ago
|
Attachment #342440 -
Attachment is obsolete: true
Attachment #342440 -
Flags: review?(mrbkap)
Comment 5•16 years ago
|
||
Comment on attachment 342591 [details] [diff] [review] v2 >+nsXPConnect::GetRuntimeInstance( /*= nsnull*/) Nit: the comment doesn't mean anything anymore. This is pure win all around.
Attachment #342591 -
Flags: superreview+
Attachment #342591 -
Flags: review?(mrbkap)
Attachment #342591 -
Flags: review+
Assignee | ||
Comment 7•16 years ago
|
||
The new version addresses the nit and fixes the compilation problem on Windows - I missed to rename nsXPConnect::GetRuntime into nsXPConnect::GetRuntimeInstance in Windows-only XPCIDispatchExtension.cpp. The try server is a nice thing :)
Attachment #342591 -
Attachment is obsolete: true
Attachment #342728 -
Flags: superreview+
Attachment #342728 -
Flags: review+
Assignee | ||
Comment 8•16 years ago
|
||
landed - http://hg.mozilla.org/mozilla-central/rev/09fcb1bab6eb
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•16 years ago
|
||
(In reply to comment #9) > this broke jsd. Could you give more details?
Assignee | ||
Comment 11•16 years ago
|
||
Recording the presence of the patch on 1.9.1 branch.
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•