895957 - goDoCommand in e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tom S [:evilpie]]

I made a fix for this that just sends the command into browser, when it's the focusedElement. Neil was about to tell me some problems with this approach, but he had to leave.

http://hg.mozilla.org/projects/larch/rev/762c00627de2
The new DoCommand method is unnecessary, I should just add the current one to idl. (By using the "string" type)

Comment 1

[[e-mail]]

The problem I spotted was with the detection of focus in a remote browser; just ignoring addons for now you still have to worry about the addons manager setting the "remote" attribute on various elements and you don't really want them to be mistaken for a browser.

Alternatively, if there is a whitelist of commands that needs to be sent to the message manager, then what you could do is to add a command controller to the element itself (presumably the remote browser binding would do this). Then when it's focused goDoCommand automatically invokes the command controller, which then sends the message to the message manager.

Comment 2

[Tom S [:evilpie]]

The last part sounds like a reasonable approach and less of a hack. I am going to look at implementing that.

Comment 3

[Tom S [:evilpie]]

Attachment: problematic wip

So I started working on setting up a Controller for the TabParent. I also got most stuff working, but there is one big problem that I only realized when I was almost finished. We can't claim to support every possible command, because that would break other command listeners higher up. But there are also certain commands that we can only support when the client is in a certain state. For example when a text-box is focused we should claim to support "undo". It's probably quite some work to maintain the correct state in the TabParent so that we can make a reasonable decision about what of the context depended things we claim support for. One easier, but also not very nice, solution would be to send a synchronous message to the child and ask if something is supported.

Comment 4

[Tom S [:evilpie]]

Neil do you think we should just whitelist everything we might be able to handle in content?

Comment 5

[Tom S [:evilpie]]

I would like to land either of these fixes by the of the week. I am going to flag for review in the hope that this moves forward.

Comment 6

[[e-mail]]

Comment on attachment 783423 [details] [diff] [review]
problematic wip

>@@ -11990,22 +11990,16 @@ nsDocShell::DoCommand(const char * inCom
I think you have to change the declaration from nsresult to NS_IMETHODIMP otherwise it won't link on Windows.

>+// From
>+// nsWindowCommandRegistration::RegisterWindowCommands
Do you really need to process of those commands?

>+  docShell->DoCommand(ToNewCString(aCommand));
This is a leak; if you can't make aCommand an nsCString& (so that you can use .get() directly) you need to use NS_LossyConvertUTF16toASCII(aCommand).get() instead.

>-      focused.messageManager.sendAsyncMessage("Content:DoCommand",
>-                                              {command: aCommand});
It looks as if you could actually implement this in remote-browser.xml instead of changing the C++. See for example browser/components/places/content/tree.xml for code that attaches a controller to a given XUL element.

Comment 7

[Tom S [:evilpie]]

Attachment: v1

Thanks a lot Neil. With your last hint I could produce a much simpler implementation only in JS.

Comment 8

[Tom S [:evilpie]]

Attachment: v1

Sorry, forgot to hg add RemoteController.jsm

Comment 9

[[e-mail]]

Comment on attachment 786974 [details] [diff] [review]
v1

>+  receiveMessage: function(message) {
>+    switch(message.name) {
>+      case "Content:DoCommand": {
>+        let controllers = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                  .getInterface(Ci.nsIDOMWindow)
>+                                  .controllers;
>+        let controller = controllers.getControllerForCommand(message.data);
>+        if (controller && controller.isCommandEnabled(message.data))
>+          controller.doCommand(message.data);
I think you'll find that (e.g.) Copy only works on page text, not in input fields.

Now from reading your other patch I notice that nsDocShell has a DoCommand(const char * inCommand) that almost does the right thing except that it doesn't check to see whether the command is enabled first. With that fixed you would be able to write docShell.doCommand(message.data) directly.

>+          {
[Why the extra block?]

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIController, Ci.nsISupports]),
IIRC generateQI automatically handles nsISupports for you.

>+  supportsCommand: function(aCommand) {
>+    if (aCommand.startsWith("places"))
>+      return false;
This check serves little use, so you might as well remove it.
[Maybe the places controller was a bad suggestion to copy!]

Comment 10

[[e-mail]]

(In reply to comment #9)
> Now from reading your other patch I notice that nsDocShell has a
> DoCommand(const char * inCommand) that almost does the right thing except
> that it doesn't check to see whether the command is enabled first. With that
> fixed you would be able to write docShell.doCommand(message.data) directly.

As I recall it's cmd_cut that needs you to check that it's enabled first.

Comment 11

[Tom S [:evilpie]]

(In reply to [e-mail] from comment #9)
> Comment on attachment 786974 [details] [diff] [review]
> v1
> 
> >+  receiveMessage: function(message) {
> >+    switch(message.name) {
> >+      case "Content:DoCommand": {
> >+        let controllers = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> >+                                  .getInterface(Ci.nsIDOMWindow)
> >+                                  .controllers;
> >+        let controller = controllers.getControllerForCommand(message.data);
> >+        if (controller && controller.isCommandEnabled(message.data))
> >+          controller.doCommand(message.data);
> I think you'll find that (e.g.) Copy only works on page text, not in input
> fields.
> 
You mean like selecting something in an input and then triggering goDoCommand("cmd_copy")? That actually works.
> Now from reading your other patch I notice that nsDocShell has a
> DoCommand(const char * inCommand) that almost does the right thing except
> that it doesn't check to see whether the command is enabled first. With that
> fixed you would be able to write docShell.doCommand(message.data) directly.
> 
> >+          {
> [Why the extra block?]
> 
I planned to add some other stuff her and I don't like when the lets leak outside the case.
> >+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIController, Ci.nsISupports]),
> IIRC generateQI automatically handles nsISupports for you.
> 
> >+  supportsCommand: function(aCommand) {
> >+    if (aCommand.startsWith("places"))
> >+      return false;
> This check serves little use, so you might as well remove it.
> [Maybe the places controller was a bad suggestion to copy!]
This was actually on purpose, because the "places" commands happen quite often.

Comment 12

[[e-mail]]

(In reply to Tom Schuster from comment #11)
> (In reply to from comment #9)
> > (From update of attachment 786974 [details] [diff] [review])
> > >+  receiveMessage: function(message) {
> > >+    switch(message.name) {
> > >+      case "Content:DoCommand": {
> > >+        let controllers = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > >+                                  .getInterface(Ci.nsIDOMWindow)
> > >+                                  .controllers;
> > >+        let controller = controllers.getControllerForCommand(message.data);
> > >+        if (controller && controller.isCommandEnabled(message.data))
> > >+          controller.doCommand(message.data);
> > I think you'll find that (e.g.) Copy only works on page text, not in input
> > fields.
> You mean like selecting something in an input and then triggering
> goDoCommand("cmd_copy")? That actually works.
Sorry, it turns out that Copy was a bad example, it's the only one that works even if you do the command on the window instead of the input.

> > >+          {
> > [Why the extra block?]
> I planned to add some other stuff her and I don't like when the lets leak
> outside the case.
Sorry, I didn't quote enough context, this is actually in the constructor, so there's nothing for the let to leak out of.

> > >+  supportsCommand: function(aCommand) {
> > >+    if (aCommand.startsWith("places"))
> > >+      return false;
> > This check serves little use, so you might as well remove it.
> > [Maybe the places controller was a bad suggestion to copy!]
> This was actually on purpose, because the "places" commands happen quite
> often.
Well if it's a bottleneck then perhaps checking whether the command starts with "cmd_" might be an idea, since all the interesting commands do.

Comment 13

[Tom S [:evilpie]]

Attachment: v2

Comment 14

[[e-mail]]

Comment on attachment 787157 [details] [diff] [review]
v2

>-    nsresult IsCommandEnabled(const char * inCommand, bool* outEnabled);
>-    nsresult DoCommand(const char * inCommand);
>+  boolean isCommandEnabled(in string command);
>+  void doCommand(in string command);
I think this changes the signature incompatibly on Windows; you can fix it by changing the nsresult to NS_IMETHODIMP in the appropriate places in nsDocShell.cpp.

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIController, Ci.nsISupports]),
I still don't think you need the nsISupports here.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 787157 [details] [diff] [review]
v2

Felipe can you give me the go ahead to land this, even though it's not a perfect solution and the edit menu doesn't work?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 787157 [details] [diff] [review]
v2

>diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl

>+    * Cherry picked parts of nsIController.
>+    * They are here, because we want to call these functions
>+    * from JS.
>+    */
>+  boolean isCommandEnabled(in string command);
>+  void doCommand(in string command);

This is pretty gross. Why not have docshell implement nsIController? Why only these parts?

>diff --git a/toolkit/content/browser-child.js b/toolkit/content/browser-child.js

>+Content.init()

As dao mentioned in another bug, "Content" is not a good name. ControllerCommands?

>diff --git a/toolkit/modules/RemoteController.jsm b/toolkit/modules/RemoteController.jsm

>+    // We can't synchronously ask content if a command is enabled,
>+    // so we always pretend is.

>+    // For now only support the commands used in "browser-context.inc"

These "just make it work" kind of changes bother me - we should either be clearly labeling them as incomplete/hacks and explaining what is needed to fix them. And if we intend to ship them, we at the very least need bugs on file to fix things properly.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 787157 [details] [diff] [review]
v2

Sorry, I don't think I'm the right person to give a final stamp for this one. (heh, no one wants to get close to this controller code).. Maybe the other Neil (Deakin) or Gavin

Comment 18

[Tom S [:evilpie]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #16)
> Comment on attachment 787157 [details] [diff] [review]
> v2
> 
> >diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl
> 
> >+    * Cherry picked parts of nsIController.
> >+    * They are here, because we want to call these functions
> >+    * from JS.
> >+    */
> >+  boolean isCommandEnabled(in string command);
> >+  void doCommand(in string command);
> 
> This is pretty gross. Why not have docshell implement nsIController? Why
> only these parts?
These were already there, also it seems to me that you usually wouldn't let such a "big class" implement nsIController, they seem to be more specialized. And we don't need most of the controller functions anyway.
> 
> >diff --git a/toolkit/content/browser-child.js b/toolkit/content/browser-child.js
> 
> >+Content.init()
> 
> As dao mentioned in another bug, "Content" is not a good name.
> ControllerCommands?
> 
This will change.
> >diff --git a/toolkit/modules/RemoteController.jsm b/toolkit/modules/RemoteController.jsm
> 
> >+    // We can't synchronously ask content if a command is enabled,
> >+    // so we always pretend is.
> 
> >+    // For now only support the commands used in "browser-context.inc"
> 
> These "just make it work" kind of changes bother me - we should either be
> clearly labeling them as incomplete/hacks and explaining what is needed to
> fix them. And if we intend to ship them, we at the very least need bugs on
> file to fix things properly.
I will file a bug, but I think it will end up with having to come up with a new design for nsIController. goDoCommand is already in a sense asynchronous (or can be made so), because we don't check if we actually did something in that call. All other instances of nsIController, that first check a command is enabled, should basically be removed and implemented differently.

Comment 19

[Tom S [:evilpie]]

Attachment: v3

Please see my previous comment.

Comment 20

[[e-mail]]

Comment on attachment 790920 [details] [diff] [review]
v3

Gavin said that my review by code inspection and build completion suffices.

Comment 21

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1a17c66f7e91

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1a17c66f7e91

Comment 23

[:Felipe Gomes (needinfo for replies!)]

I'm seeing this error on stdout:

JavaScript error: chrome://global/content/bindings/remote-browser.xml, line 122: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIControllers.removeController]

Seems like removeController only returns that when the controller to be removed was not found, so I wonder if by the time the destructor runs the controller has already cleared all controllers.

controller controller controller

Comment 24

[[e-mail]]

It might depend on when the destructor runs. You should try closing a remote tab and closing the window with a remote tab in it too see whether the error is consistent.