Closed
Bug 564382
Opened 14 years ago
Closed 14 years ago
e10s: Remove CPOWs from PContentProcess tree
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jdm, Assigned: jdm)
References
Details
Attachments
(1 file, 10 obsolete files)
29.31 KB,
patch
|
Details | Diff | Splinter Review |
The word from those is the know is that they've got to go.
Assignee | ||
Comment 1•14 years ago
|
||
Right now I'm not just cutting CPOWs, but replacing them with Handles instead. benjamn, is this the right way? I'd also especially appreciate your input on the //XXX line in there, because I'm having trouble figuring out what is supposed to be used instead.
Assignee: nobody → josh
Attachment #444052 -
Flags: feedback?(mozilla+ben)
Comment 2•14 years ago
|
||
Comment on attachment 444052 [details] [diff] [review] WIP (In reply to comment #1) > Right now I'm not just cutting CPOWs, but replacing them with Handles instead. > benjamn, is this the right way? It's certainly not backwards compatible in any way, but no promises were made to that effect. I don't see the need for more than one Handle. Can you think of any? > I'd also especially appreciate your input on > the //XXX line in there, because I'm having trouble figuring out what is > supposed to be used instead. Hmm. This issue argues for bsmedberg's original design of letting handles have an .object property whose properties the client can modify. Then the obj you're passing could just become that property. I'm tempted to switch to that, since any further expansion of the API exposed by handle objects (.createHandle(), .isValid, .invalidate()) risks conflicting with client-defined object properties, anyway.
Attachment #444052 -
Flags: feedback?(mozilla+ben) → feedback+
Assignee | ||
Comment 3•14 years ago
|
||
(In reply to comment #2) > (From update of attachment 444052 [details] [diff] [review]) > (In reply to comment #1) > > Right now I'm not just cutting CPOWs, but replacing them with Handles instead. > > benjamn, is this the right way? > > It's certainly not backwards compatible in any way, but no promises were made > to that effect. > > I don't see the need for more than one Handle. Can you think of any? So instead of sendSyncMessageToParent taking an optional array of CPOWs, it just takes one optional handle instead?
Comment 4•14 years ago
|
||
In addition to coping with the renaming of js/src/ipc to js/ipc, this patch partially addresses the XXXjdm discussed above. That is, if the JSObject you're trying to pass isn't a Handle, it just comes out as null on the other side. In the future, we can be more forgiving (creating a new Handle and defining an .object property thereon).
What about throwing an exception in that case?
Comment 6•14 years ago
|
||
Comment on attachment 444718 [details] [diff] [review] Update to accomodate bug 565078, and use Handle::FromJSObject for passing Handles in messages. >- // GetOrCreateWrapper is null safe! >- aObjects.AppendElement(mContextWrapper->GetOrCreateWrapper(obj)); >+ aObjects.AppendElement(HandleChild::FromJSObject(ctx, obj)); This is the only substantive change with this patch. Basically, I'm proposing to delegate the conversion implementation to HandleChild, so you don't have to worry about it here.
Attachment #444718 -
Flags: feedback?(josh)
Assignee | ||
Comment 7•14 years ago
|
||
Comment on attachment 444718 [details] [diff] [review] Update to accomodate bug 565078, and use Handle::FromJSObject for passing Handles in messages. I'm a fan of sensible delegation.
Attachment #444718 -
Flags: feedback?(josh) → feedback+
Assignee | ||
Comment 8•14 years ago
|
||
Remove all traces of CPOWs from the PIFrameEmbedding tree and associated content elements.
Attachment #444052 -
Attachment is obsolete: true
Attachment #444718 -
Attachment is obsolete: true
Attachment #444783 -
Flags: review?(mozilla+ben)
Attachment #444783 -
Flags: review?(Olli.Pettay)
Comment 9•14 years ago
|
||
You should update iid when modifying interfaces.
Comment 10•14 years ago
|
||
Comment on attachment 444783 [details] [diff] [review] Patch This fixes bug 554913. https://wiki.mozilla.org/Content_Process_Event_Handlers needs to be updated after this. And http://mxr.mozilla.org/projects-central/source/electrolysis/content/base/public/nsIFrameMessageManager.idl?mark=54-54#47 needs to be updated when landing this. You should be able to remove ReportChildAlreadyBlocked() from RecvsendSyncMessageToParent. I'm not familiar with PHandle, but from messageManager point of view looks ok.
Attachment #444783 -
Flags: review?(Olli.Pettay) → review+
Comment 11•14 years ago
|
||
(Btw, is there a bug open to remove rpc createWindow() ?)
Comment 12•14 years ago
|
||
Comment on attachment 444783 [details] [diff] [review] Patch Thanks for going to the trouble of replicating the makeRoot/mRootHandle logic from ContextWrapper{Parent,Child}.h, but that's not going to be needed long-term. I'm planning to expose nsIFrameMessageManager.createHandle() and a similar method on TabChild::mCx::globalObject, rather than exposing global handle objects. If you'd prefer, you could try your hand at adding those methods yourself, if you want to learn about nsIVariants and defining functions using the JSAPI.
Attachment #444783 -
Flags: review?(mozilla+ben) → review+
Comment 13•14 years ago
|
||
(In reply to comment #12) > If you'd prefer, you could try your hand at adding those methods yourself, if > you want to learn about nsIVariants and defining functions using the JSAPI. If not, feel free just to remove the makeRoot/mRootHandle business :)
Assignee | ||
Comment 14•14 years ago
|
||
All requested changes made. I'm really interested in doing the createHandle work, as I'd like to learn more about all that, but that work can happen in another patch if I end up getting to it.
Attachment #444783 -
Attachment is obsolete: true
Comment 15•14 years ago
|
||
Could you still update test.xul a bit. It has some tests for messageManager+CPOW. The test works so that you click some link in the page, and you should get link url + PASS in the statusbar. If CPOWs are removed those test would break.
Assignee | ||
Comment 16•14 years ago
|
||
Good call. I've removed all traces of the CPOW tests for now, as I'm still not quite certain how they translate over to the handles.
Attachment #446178 -
Attachment is obsolete: true
Assignee | ||
Comment 17•14 years ago
|
||
Too much deletion from test.xul.
Attachment #446188 -
Attachment is obsolete: true
Assignee | ||
Comment 18•14 years ago
|
||
(In reply to comment #11) > (Btw, is there a bug open to remove rpc createWindow() ?) Bug 567058.
Comment 19•14 years ago
|
||
This removes js/ipc and related code. It does not add back handle support.
Updated•14 years ago
|
Attachment #451407 -
Flags: review?(Olli.Pettay)
Updated•14 years ago
|
Attachment #451407 -
Flags: review?(Olli.Pettay) → review-
Comment 20•14 years ago
|
||
Comment on attachment 451407 [details] [diff] [review] patch v.1 >+++ b/content/base/public/nsIFrameMessageManager.idl >@@ -47,17 +47,17 @@ interface nsIFrameMessageListener : nsIS > /** > * This is for JS only. > * receiveMessage is called with one parameter, which has the following > * properties: > * { > * name: %message name%, > * sync: %true or false%. > * json: %json object or null%, >- * objects: %array of cpow or null, always null if sync is false% >+ * objects: %array of handles or null, always null if sync is false% Since you're removing 'objects', remove this comment too. And also the "@note objects property isn't implemented yet." We can add those back if we start to use handles. > bool > ContentProcessChild::RecvPTestShellConstructor(PTestShellChild* actor) > { >- actor->SendPContextWrapperConstructor()->SendPObjectWrapperConstructor(true); >- return true; >+ return false; > } I don't know why this is needed. >- PContextWrapper(); >- >- rpc sendSyncMessageToParent(nsString aMessage, nsString aJSON, PObjectWrapper[] aObjects) >+ rpc sendSyncMessageToParent(nsString aMessage, nsString aJSON) > returns (nsString[] retval); The whole point of this bug is to make sendSyncMessageToParent 'sync'. > rpc protocol PTestShell > { > manager PContentProcess; > > manages PTestShellCommand; >- manages PContextWrapper; > > child: > __delete__(); > > ExecuteCommand(nsString aCommand); > > PTestShellCommand(nsString aCommand); > >-parent: >- PContextWrapper(); >- > }; I don't know about this change either. But I guess it is ok. >--- a/js/ipc/CPOWTypes.h >+++ /dev/null Uh, you're removing CPOWs. Is that what we want? This bug is not about that. Could you just remove CPOW usage from sendSencMessage/TabChild/TabParent and file a new bug for CPOW removal if that is what we want to do. (I thought we might want to use CPOWs with jetpacks or something, but I guess I'm wrong here.)
Comment 21•14 years ago
|
||
my misunderstanding... i was under the impression after our conversation that cpows were to be removed. Please ignore the noise. (and thanks for the review!)
Assignee | ||
Comment 22•14 years ago
|
||
All traces of handles removed from the patch. Make sure I haven't done anything stupid, please?
Attachment #451407 -
Attachment is obsolete: true
Attachment #451865 -
Flags: review?(Olli.Pettay)
Comment 23•14 years ago
|
||
Comment on attachment 451865 [details] [diff] [review] Patch without handles gfxMatrix matrix, Shmem buf); > > // @param matrix the transformation matrix the context we're going to draw into should have. > PDocumentRendererNativeID(PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h, nsString bgcolor, PRUint32 flags, bool flush, >- gfxMatrix matrix, PRUint32 nativeID); >+ gfxMatrix matrix, PRUint32 nativeID); > }; Don't add extra spaces. > TabParent::ReceiveMessage(const nsString& aMessage, > PRBool aSync, > const nsString& aJSON, >- const nsTArray<PObjectWrapperParent*>* aObjects, > nsTArray<nsString>* aJSONRetVal) > { > nsCOMPtr<nsIFrameLoaderOwner> frameLoaderOwner = > do_QueryInterface(mFrameElement); > if (frameLoaderOwner) { > nsRefPtr<nsFrameLoader> frameLoader = frameLoaderOwner->GetFrameLoader(); > if (frameLoader && frameLoader->GetFrameMessageManager()) { > nsFrameMessageManager* manager = frameLoader->GetFrameMessageManager(); > JSContext* ctx = manager->GetJSContext(); > JSAutoRequest ar(ctx); > jsval* dest; >- PRUint32 len = aObjects ? aObjects->Length() : 0; >+ PRUint32 len = 0; Could you add a comment here that once we have handles, the array length will be > 0 Looks ok to me.
Attachment #451865 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 24•14 years ago
|
||
Nits addressed. Let's land this sucker.
Attachment #446190 -
Attachment is obsolete: true
Attachment #451865 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [land in e10s]
Assignee | ||
Comment 25•14 years ago
|
||
Rebased; should hopefully apply cleanly.
Attachment #451951 -
Attachment is obsolete: true
Assignee | ||
Comment 26•14 years ago
|
||
Now rebased without the handles patch interfering.
Attachment #452074 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [land in e10s]
Assignee | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•