1297539 - Handle eContentCommandPasteTransferable for remote targets

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jimmy Wang (:jimicy) - works on e10s stuff]

EventStateManager::DoContentCommandEvent does not handle eContentCommandPasteTransferable for remote targets.

Comment 1

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

Review commit: https://reviewboard.mozilla.org/r/73736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73736/

Comment 2

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

Review commit: https://reviewboard.mozilla.org/r/73738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73738/

Comment 3

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

https://reviewboard.mozilla.org/r/73736/#review71732

::: dom/ipc/TabChild.cpp:2168
(Diff revision 1)
> +bool
> +TabChild::RecvPasteTransferable(const IPCDataTransfer& aDataTransfer,
> +                                const bool& aIsPrivateData,
> +                                const IPC::Principal& aRequestingPrincipal)
> +{
> +  nsresult rv;

Modelled code for creating a nsITransferable from IPCDataTransfer from
https://dxr.mozilla.org/mozilla-central/rev/bd7645928990649c84609d3f531e803c2d41f269/dom/ipc/ContentParent.cpp#2495

::: dom/ipc/TabChild.cpp:2229
(Diff revision 1)
> +  }
> +
> +  trans->SetIsPrivateData(aIsPrivateData);
> +  trans->SetRequestingPrincipal(aRequestingPrincipal);
> +
> +  nsCOMPtr<nsIDocShell> ourDocShell = do_GetInterface(WebNavigation());

Use `nsDocShell::DoCommandWithParams` for cmd "cmd_pasteTransferable"
to call
`nsBaseCommandController::DoCommandWithParams`
nsControllerCommandTable::DoCommandParams
PasteTransferableCommand::DoCommandParams
HTMLEditor::PasteTransferable or TextEditor::PasteTransferable

Comment 4

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

https://reviewboard.mozilla.org/r/73738/#review71752

::: dom/events/EventStateManager.cpp:5207
(Diff revision 1)
>          case eContentCommandPasteTransferable: {
> +          nsFocusManager* fm = nsFocusManager::GetFocusManager();
> +          nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nullptr;
> +          RefPtr<TabParent> remote = TabParent::GetFrom(focusedContent);
> +          if (remote) {
> +            nsCOMPtr<nsITransferable> aTransferable = aEvent->mTransferable;

Modelled code for creating a IPCDataTransfer from nsITransferable
`nsClipboardProxy::SetData`
https://dxr.mozilla.org/mozilla-central/rev/bd7645928990649c84609d3f531e803c2d41f269/widget/nsClipboardProxy.cpp#26

Comment 5

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

https://reviewboard.mozilla.org/r/73738/#review71766

::: dom/events/EventStateManager.cpp:5197
(Diff revision 1)
>      // When GetControllerForCommand succeeded but there is no controller, the
>      // command isn't supported.
>      aEvent->mIsEnabled = false;
>    } else {
>      bool canDoIt;
>      rv = controller->IsCommandEnabled(cmd, &canDoIt);

In the remote and non-remote case, `IsCommandEnabled` calls the appropriate controller's `IsCommandEnabled`
remote case: RemoteController.jsm
non-remote case: nsBaseCommandController.cpp

Which calls
`PasteTransferableCommand::IsCommandEnabled`
That returns whether or not you are currently able to paste a transferable. This means IsCommandEnabled is only true when you're focused in a text editor/input

Comment 6

[Neil Deakin]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

Instead of copying the code from ContentParent::RecvSetClipboard, can you instead add a helper function in nsContentUtils (IPCTransferableToTransferable) and have both call it?

For extra points, nsClipboardProxy::GetData could also use this with say an extra flag to handle the slight differences.

Comment 7

[Neil Deakin]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

https://reviewboard.mozilla.org/r/73738/#review72384

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

https://reviewboard.mozilla.org/r/73738/#review72972

I have a couple of small comments here, but they're minor enough that I'll stamp r+ with them addressed.

::: dom/events/EventStateManager.cpp:5207
(Diff revision 1)
>          case eContentCommandPasteTransferable: {
> +          nsFocusManager* fm = nsFocusManager::GetFocusManager();
> +          nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nullptr;
> +          RefPtr<TabParent> remote = TabParent::GetFrom(focusedContent);
> +          if (remote) {
> +            nsCOMPtr<nsITransferable> aTransferable = aEvent->mTransferable;

Is there a need for the one-time-use nsCOMPtr here? Any reason to not just pass mTransferable through?

::: dom/events/EventStateManager.cpp:5213
(Diff revision 1)
> +            IPCDataTransfer ipcDataTransfer;
> +            nsContentUtils::TransferableToIPCTransferable(aTransferable,
> +                                                          &ipcDataTransfer,
> +                                                          false, nullptr,
> +                                                          remote->Manager()->
> +                                                          AsContentParent());

Two comments:
* Do we have to worry about the case where !remote->Manager()->IsContentParent()? It might be worth asserting that it returns true.
* I'd actually use a single-use `ContentParent *cp` to avoid the weird wrapping you get with this inline.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

https://reviewboard.mozilla.org/r/73736/#review72974

r- based on code duplication.

::: dom/ipc/TabChild.cpp:2175
(Diff revision 1)
> +    do_CreateInstance("@mozilla.org/widget/transferable;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, true);
> +  trans->Init(nullptr);
> +
> +  const nsTArray<IPCDataTransferItem>& items = aDataTransfer.items();
> +  for (const auto& item : items) {

I agree with Neil, this really needs to share the code with `TabParent` and with `nsClipboardProxy` as possible.

Comment 10

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

https://reviewboard.mozilla.org/r/73738/#review72972

> Is there a need for the one-time-use nsCOMPtr here? Any reason to not just pass mTransferable through?

I use mTransferable later hence the declaration. 
```
nsCOMPtr<nsITransferable> aTransferable = aEvent->mTransferable;
aTransferable->GetIsPrivateData(&isPrivateData);
aTransferable->GetRequestingPrincipal(getter_AddRefs(requestingPrincipal));
```

Comment 11

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1297539 - Write test for content event 'pasteTransferable' for the remote case. Ensures that EventStateManager dispatches the transferable correctly to TextEditor::PasteTransferable for plain text and HTMLEditor::PasteTransferable for html.

Review commit: https://reviewboard.mozilla.org/r/75450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75450/

Comment 12

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73736/diff/1-2/

Comment 13

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73738/diff/1-2/

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

https://reviewboard.mozilla.org/r/73736/#review73402

This looks better. I have a couple of small comments. r=me with them addressed.

::: dom/ipc/TabChild.cpp:2171
(Diff revision 2)
> +
> +  nsCOMPtr<nsIDocShell> ourDocShell = do_GetInterface(WebNavigation());
> +  if (NS_WARN_IF(!ourDocShell)) {
> +    return true;
> +  }
> +  RefPtr<nsDocShell> docShell = static_cast<nsDocShell*>(ourDocShell.get());

Do you actually need to downcast this? It looks like `DoCommandWithParams` already lives on `nsIDocShell`.

::: dom/ipc/TabChild.cpp:2179
(Diff revision 2)
> +  NS_ENSURE_SUCCESS(rv, true);
> +
> +  rv = params->SetISupportsValue("transferable", trans);
> +  NS_ENSURE_SUCCESS(rv, true);
> +
> +  rv = docShell->DoCommandWithParams("cmd_pasteTransferable", params);

No need to assign to `rv` if you're not going to check it.

::: dom/ipc/TabParent.cpp:2236
(Diff revision 2)
> +                                 const bool& aIsPrivateData,
> +                                 const IPC::Principal& aRequestingPrincipal)
> +{
> +  return PBrowserParent::SendPasteTransferable(aDataTransfer,
> +                                               aIsPrivateData,
> +                                               aRequestingPrincipal);

Is there any reason to do this? It looks like it might be leftover from debugging. If it's not needed, nuke it!

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

https://reviewboard.mozilla.org/r/73738/#review73404

::: dom/events/EventStateManager.cpp:5209
(Diff revision 2)
> +          nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nullptr;
> +          RefPtr<TabParent> remote = TabParent::GetFrom(focusedContent);
> +          if (remote) {
> +            NS_ENSURE_TRUE(remote->Manager()->IsContentParent(), NS_ERROR_FAILURE);
> +
> +            nsCOMPtr<nsITransferable> aTransferable = aEvent->mTransferable;

Oops, I missed the second use. That being said, aTransferable should be a parameter. Please call this `transferable` or something like that.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8786534 [details]
Bug 1297539 - Write test for content event 'pasteTransferable' for the remote case. Ensures that EventStateManager dispatches the transferable correctly to TextEditor::PasteTransferable for plain text and HTMLEditor::PasteTransferable for html.

https://reviewboard.mozilla.org/r/75450/#review73408

This looks good. I have a few stylistic nitpicks I noticed. Feel free to take these as suggestions and ignore them if you don't like them.

::: browser/base/content/test/general/browser_bug1297539.js:40
(Diff revision 1)
> +  var clip = Cc["@mozilla.org/widget/clipboard;1"].getService(Ci.nsIClipboard);
> +  clip.getData(trans, Ci.nsIClipboard.kGlobalClipboard);
> +  return trans;
> +}
> +
> +function* cutCurrentSelection(elementQueryString, property) {

I'd prefer to take the browser instead of acting on `gBrowser.selectedBrowser`.

::: browser/base/content/test/general/browser_bug1297539.js:48
(Diff revision 1)
> +                                       gBrowser.selectedBrowser);
> +
> +  // The editor should be empty after cut.
> +  yield ContentTask.spawn(gBrowser.selectedBrowser,
> +  {elementQueryString, property},
> +  function* (args){

The indentation here looks weird to me. I'd prefer:

```
yield ContentTask.spawn(browser,
  { elementQueryString, property },
  function* ({elementQueryString, property}) {
    ...
  });
```

Also note the destructuring trick that allows you to avoid having to say `args.` everywhere :-).

::: browser/base/content/test/general/browser_bug1297539.js:64
(Diff revision 1)
> +  let testPage =
> +    'data:text/html,' +
> +    '<textarea id="textarea">Write something here</textarea>';
> +  let DOMWindowUtils = EventUtils._getDOMWindowUtils(window);
> +
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, testPage);

Another place where you can use `BrowserTestUtils.withNewTab` if you want.

::: browser/base/content/test/general/browser_bug1297539.js:99
(Diff revision 1)
> +  let testPage =
> +    'data:text/html,' +
> +    '<div contenteditable="true"><b>Bold Text</b><i>italics</i></div>';
> +  let DOMWindowUtils = EventUtils._getDOMWindowUtils(window);
> +
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, testPage);

Totally stylistic, but you could totally use `BrowerTestUtils.withNewTab` here if you want.

Comment 17

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

https://reviewboard.mozilla.org/r/73736/#review73402

> Is there any reason to do this? It looks like it might be leftover from debugging. If it's not needed, nuke it!

Added by
https://reviewboard.mozilla.org/r/61726/diff/4#index_header
as an arg to `ContentParent::RecvSetClipboard`.

Seems to be needed to copy (by context menu) a gif and paste on Windows. So I'm assuming it's needed to setup a transferable correctly.

Comment 18

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73736/diff/2-3/

Comment 19

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73738/diff/2-3/

Comment 20

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8786534 [details]
Bug 1297539 - Write test for content event 'pasteTransferable' for the remote case. Ensures that EventStateManager dispatches the transferable correctly to TextEditor::PasteTransferable for plain text and HTMLEditor::PasteTransferable for html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75450/diff/1-2/

Comment 21

[Jimmy Wang (:jimicy) - works on e10s stuff]

Try push looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a391f0c2c19f

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53295d9cc8ef
Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable". New method nsContentUtils::IPCTransferableToTransferable since ContentParent::RecvSetClipboard and TabChild::RecvPasteTransferable both require the same setup to make a transferable. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/b7c49e3bcf03
Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/f2cd09361916
Write test for content event 'pasteTransferable' for the remote case. Ensures that EventStateManager dispatches the transferable correctly to TextEditor::PasteTransferable for plain text and HTMLEditor::PasteTransferable for html. r=mrbkap

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54efec27fb40
Add missing newline to end of browser_bug1297539.js to please ESLint.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing own test:

https://hg.mozilla.org/integration/autoland/rev/43a0ed761011fd5f569b7baaeaf42a040698cc4e
https://hg.mozilla.org/integration/autoland/rev/512f3b8b736e23d6f2140aee174b82eb49c6a244
https://hg.mozilla.org/integration/autoland/rev/01968bed4e17924c37e335e776cb04200d7a2c4a
https://hg.mozilla.org/integration/autoland/rev/5d8d8e09acfa0ca1ccef77ede3d0095ac0a4b2f1

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=2895203&repo=autoland

Comment 25

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784197 [details]
Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73736/diff/3-4/

Comment 26

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8784198 [details]
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73738/diff/3-4/

Comment 27

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8786534 [details]
Bug 1297539 - Write test for content event 'pasteTransferable' for the remote case. Ensures that EventStateManager dispatches the transferable correctly to TextEditor::PasteTransferable for plain text and HTMLEditor::PasteTransferable for html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75450/diff/2-3/

Comment 28

[Jimmy Wang (:jimicy) - works on e10s stuff]

Pushed new commits to run test for mac OS only.

Try push. Test doesn't fail on windows.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ccca26d55cc

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bb42e644304
Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable". New method nsContentUtils::IPCTransferableToTransferable since ContentParent::RecvSetClipboard and TabChild::RecvPasteTransferable both require the same setup to make a transferable. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/4e15e5606976
Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/6f406d30eddb
Write test for content event 'pasteTransferable' for the remote case. Ensures that EventStateManager dispatches the transferable correctly to TextEditor::PasteTransferable for plain text and HTMLEditor::PasteTransferable for html. r=mrbkap

Comment 30

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: source-code-test-app-for-paste-from-services-menu.zip

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3bb42e644304
https://hg.mozilla.org/mozilla-central/rev/4e15e5606976
https://hg.mozilla.org/mozilla-central/rev/6f406d30eddb