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)

defect

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.
Blocks: 1060099
Assignee: nobody → tomica+amo
tracking-e10s: --- → +
Depends on: old-e10s-m2
Priority: -- → P2
(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)
bug 1070952 seems to be the thing
Flags: needinfo?(rFobic)
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
Attached patch platform patch (obsolete) — Splinter Review
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)
Attached file SDK patch
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)
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)
Attached patch platform patch (obsolete) — Splinter Review
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)
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 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)
(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.
Attached patch platform patch (obsolete) — Splinter Review
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)
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)
Attached patch platform patchSplinter Review
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)
Comment on attachment 8517294 [details] [review]
SDK patch

r=me with two comments addressed..
Attachment #8517294 - Flags: review?(tomica+amo) → review+
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+
Landed the platform piece: https://hg.mozilla.org/integration/fx-team/rev/89505d98a59c

Filed bug 1097241 to make the menu handling saner.
Keywords: leave-open
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
Attached patch platform fixupSplinter Review
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/872f2f8a6c0d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1130830
The content-contextmenu observer notification was introduced here, it should be documented.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.