Closed Bug 895957 Opened 7 years ago Closed 7 years ago

goDoCommand in e10s

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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)
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
The last part sounds like a reasonable approach and less of a hack. I am going to look at implementing that.
Attached patch problematic wip (obsolete) — Splinter Review
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.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Neil do you think we should just whitelist everything we might be able to handle in content?
Flags: needinfo?(neil)
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.
Attachment #783423 - Flags: review?(neil)
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.
Attachment #783423 - Flags: review?(neil) → feedback+
Attached patch v1 (obsolete) — Splinter Review
Thanks a lot Neil. With your last hint I could produce a much simpler implementation only in JS.
Attachment #783423 - Attachment is obsolete: true
Attachment #786970 - Flags: review?(neil)
Attached patch v1 (obsolete) — Splinter Review
Sorry, forgot to hg add RemoteController.jsm
Attachment #786970 - Attachment is obsolete: true
Attachment #786970 - Flags: review?(neil)
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!]
(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.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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.
(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.
Attached patch v2 (obsolete) — Splinter Review
Attachment #786974 - Attachment is obsolete: true
Attachment #787157 - Flags: review?(neil)
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.
Attachment #787157 - Flags: review?(neil) → feedback+
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?
Attachment #787157 - Flags: review?(felipc)
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 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
Attachment #787157 - Flags: review?(felipc)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attached patch v3Splinter Review
Please see my previous comment.
Attachment #787157 - Attachment is obsolete: true
Attachment #790920 - Flags: review?(gavin.sharp)
Comment on attachment 790920 [details] [diff] [review]
v3

Gavin said that my review by code inspection and build completion suffices.
Attachment #790920 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/1a17c66f7e91
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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
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.
You need to log in before you can comment on or make changes to this bug.