Closed
Bug 1060138
Opened 10 years ago
Closed 10 years ago
Fix context-menu to work in e10s
Categories
(Add-on SDK Graveyard :: General, defect, P2)
Add-on SDK Graveyard
General
Tracking
(e10s+)
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: zombie, Assigned: mossop)
References
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 3 obsolete files)
as explained in bug 1058698, emitSync() is required for context-menu, but is unpossible with e10s. we need to figure something out.. Irakli had the idea to provide a different api, but i can't find the bug with those comments.
Assignee | ||
Updated•10 years ago
|
Comment 1•10 years ago
|
||
(In reply to Tomislav Jovanovic [:zombie] from comment #0) > Irakli had the idea to provide a different api, but i can't find the bug > with those comments. Irakli, do you remember what bug that was?
Flags: needinfo?(rFobic)
Assignee | ||
Comment 4•10 years ago
|
||
While a new context menu API will be nice I don't think we can just ditch the existing users of context-menu when e10s turns on.
Assignee: tomica+amo → dtownsend+bugmail
Summary: figure out what to do with content scripts for context-menu in e10s → Fix context-menu to work in e10s
Assignee | ||
Comment 5•10 years ago
|
||
I think a lot of add-ons are going to want to pass some data from the child process to the main process when the context menu is opened. This adds a hook for them to do so. An add-on just needs to listen to the observer notification then add something to the addonInfo property of its subject. The norm should be to create a property in that object for the add-on ID and then put whatever serializable data they want there.
Attachment #8517291 -
Flags: review?(mconley)
Assignee | ||
Comment 6•10 years ago
|
||
This updates the context-menu API to work in e10s. It works by creating child proxies for every menu item and context. When the context menu is opened these are called to pass some data through to the parent process (via the previous patch in this bug). The parent side then uses that data to figure out visibility. There is one API change. Previously we would only instantiate and call into content scripts when all the other contexts said to go ahead, now we do it for every context click. I don't expect this to have any impact on add-ons. The tests all pass in e10s and non-e10s modes (aside from two tests that had to be changed because of the above API change). There are some slight timing differences due to async message passing but nothing that should impact users.
Attachment #8517294 -
Flags: review?(tomica+amo)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8517291 [details] [diff] [review] platform patch There;s a better fix coming here once it's been through try.
Attachment #8517291 -
Attachment is obsolete: true
Attachment #8517291 -
Flags: review?(mconley)
Assignee | ||
Comment 8•10 years ago
|
||
Ok this is improved in that it always gives add-ons the opportunity to provide data regardless of whether the page is remote or not. I was working around this on the SDK side but that would get messy if many add-ons did it. I was tempted to just make all context menus use the message manager to open rather than having the inconsistent paths we have now but decided to leave that as-is for now. Frankly we could use this observer approach and make all the browser stuff use it instead and make the context menu stuff work from toolkit for all apps.
Attachment #8518613 -
Flags: review?(mconley)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8517294 [details] [review] SDK patch my main question is _why_ the global MM (and thus broadcastMessage) was used in a lot of places. is it that some parts can't be made to work with a targeted MM, or is it more efficient this way to broadcast remote item options to all tabs at once on startup (which i could maybe then argue against), or what? if the end response is that there are one or two places where we must use broadcastMessage, that should probably be a code comment, otherwise, an answer here is fine. and other than those few "whys", the code is pretty understandable as to "what" you are trying to do. it's just lots of it, so i'd like to go through it again after those fixes/answers..
Attachment #8517294 -
Flags: review?(tomica+amo) → review-
Comment 10•10 years ago
|
||
Comment on attachment 8518613 [details] [diff] [review] platform patch Review of attachment 8518613 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty sane - just a few nits and notes. Next iteration should do the trick. ::: browser/base/content/content.js @@ +121,5 @@ > + spellInfo = > + InlineSpellCheckerContent.initContextMenu(event, editFlags, this); > + } > + > + sendSyncMessage("contextmenu", { editFlags, spellInfo, addonInfo }, { event }); For some reason, it's only now that I've learned that { foo } syntax exists. Nice. @@ +129,5 @@ > + let mainWin = docShell.QueryInterface(Ci.nsIDocShellTreeItem) > + .rootTreeItem > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > + mainWin.gContextMenuContentData = { I know it's not your fault, but I'm not too jazzed by this global we're stashing here. :/ @@ +136,1 @@ > } Nit: Missing semicolon ::: browser/base/content/nsContextMenu.js @@ +534,3 @@ > let editFlags; > + this.isRemote = gContextMenuContentData && gContextMenuContentData.isRemote; > + if (this.isRemote) { if (this.isRemote) { this.isRemote = true; // ... } else { this.isRemote = false; } This looks wrong. I don't think we want to be setting isRemote in those blocks anymore.
Attachment #8518613 -
Flags: review?(mconley)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10) > Comment on attachment 8518613 [details] [diff] [review] > platform patch > > Review of attachment 8518613 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks pretty sane - just a few nits and notes. Next iteration should do > the trick. > > ::: browser/base/content/content.js > @@ +121,5 @@ > > + spellInfo = > > + InlineSpellCheckerContent.initContextMenu(event, editFlags, this); > > + } > > + > > + sendSyncMessage("contextmenu", { editFlags, spellInfo, addonInfo }, { event }); > > For some reason, it's only now that I've learned that { foo } syntax exists. > Nice. I live to teach > @@ +129,5 @@ > > + let mainWin = docShell.QueryInterface(Ci.nsIDocShellTreeItem) > > + .rootTreeItem > > + .QueryInterface(Ci.nsIInterfaceRequestor) > > + .getInterface(Ci.nsIDOMWindow); > > + mainWin.gContextMenuContentData = { > > I know it's not your fault, but I'm not too jazzed by this global we're > stashing here. :/ I have an idea for improving this but it should probably come in another bug. > @@ +136,1 @@ > > } > > Nit: Missing semicolon > > ::: browser/base/content/nsContextMenu.js > @@ +534,3 @@ > > let editFlags; > > + this.isRemote = gContextMenuContentData && gContextMenuContentData.isRemote; > > + if (this.isRemote) { > > if (this.isRemote) { > this.isRemote = true; > // ... > } else { > this.isRemote = false; > } > > This looks wrong. I don't think we want to be setting isRemote in those > blocks anymore. Good catch.
Assignee | ||
Comment 12•10 years ago
|
||
Addresses the comments and adds one extra fix from the SDK patch review. This now adds the browser property to gContextMenuContentData for non-child-process frames to match the property for child-process frames. This allows main process code to message the right frame when items are clicked on without broadcasting.
Attachment #8518613 -
Attachment is obsolete: true
Attachment #8520266 -
Flags: review?(mconley)
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8517294 [details] [review] SDK patch Updated from the comments. The broadcast cases are where a new item is created, changed or deleted. In this case we have to tell all the frame scripts to update their remote lists. Hopefully made that more clear in the comments.
Attachment #8517294 -
Flags: review- → review?(tomica+amo)
Assignee | ||
Comment 14•10 years ago
|
||
Hopefully the final change here. This adds a CPOW to the popupNode to gContextMenuContentData, this is the same as if you just did gContextMenuContentData.event.target but saves the CPOW calls. It allows us to pass the original popupNode back down to the frame script cheaply when necessary.
Attachment #8520266 -
Attachment is obsolete: true
Attachment #8520266 -
Flags: review?(mconley)
Attachment #8520773 -
Flags: review?(mconley)
Reporter | ||
Comment 15•10 years ago
|
||
Comment on attachment 8517294 [details] [review] SDK patch r=me with two comments addressed..
Attachment #8517294 -
Flags: review?(tomica+amo) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8520773 [details] [diff] [review] platform patch Review of attachment 8520773 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine for now, but let's get a bug on file for making gContextMenuContentData a bit better (or less necessary). Thanks for the patch!
Attachment #8520773 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Landed the platform piece: https://hg.mozilla.org/integration/fx-team/rev/89505d98a59c Filed bug 1097241 to make the menu handling saner.
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 18•10 years ago
|
||
And backed out because those final changes make us leak in the event that the context menu event doesn't actually trigger the main context menu, like in XUL pages. https://hg.mozilla.org/integration/fx-team/rev/33624e440cff
Assignee | ||
Comment 19•10 years ago
|
||
One simple changes fixes the platform side, by listening for the bubbling event we only get it if XUL or something else hasn't already cancelled the event.
Attachment #8523963 -
Flags: review?(mconley)
Comment 20•10 years ago
|
||
Comment on attachment 8523963 [details] [diff] [review] platform fixup Review of attachment 8523963 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense. Thanks Mossop.
Attachment #8523963 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Re-landed the platform pieces: https://hg.mozilla.org/integration/fx-team/rev/31722b673a74
Comment 23•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/ce54ed4dac6de9fd53ff30ee8218a0487c609fe2 Bug 1060138: Fix context-menu to work in e10s. r=zombie
Assignee | ||
Comment 24•10 years ago
|
||
Uplifted this direct to fx-team so add-ons are fixed a.s.a.p. https://hg.mozilla.org/integration/fx-team/rev/872f2f8a6c0d
Keywords: leave-open
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/872f2f8a6c0d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 27•8 years ago
|
||
The content-contextmenu observer notification was introduced here, it should be documented.
Keywords: dev-doc-needed
Comment 28•8 years ago
|
||
I've added a note on this: https://developer.mozilla.org/en-US/docs/Observer_Notifications#Context_menu.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•