Closed Bug 1297539 Opened 8 years ago Closed 8 years ago

Handle eContentCommandPasteTransferable for remote targets

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jimicy, Assigned: jimicy)

References

Details

Attachments

(4 files)

Bug 1297539 - Add IPC message, PasteTransferable, to call PasteTransferable via a controller on the content process to handle the command content event, "pasteTransferable".
58 bytes, text/x-review-board-request
mrbkap
: review+
enndeakin
: review+
Details
Bug 1297539 - Send message PasteTransferable if the focused element is on the content process, otherwise pass the transferable to the controller directly.
58 bytes, text/x-review-board-request
enndeakin
: review+
mrbkap
: review+
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.
58 bytes, text/x-review-board-request
mrbkap
: review+
Details
source-code-test-app-for-paste-from-services-menu.zip
76.58 KB, application/zip
Details 
EventStateManager::DoContentCommandEvent does not handle eContentCommandPasteTransferable for remote targets.
Assignee: nobody → jimmyw22
Blocks: 1261299
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 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 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
Attachment #8784197 - Flags: review?(mrbkap)
Attachment #8784197 - Flags: review?(enndeakin)
Attachment #8784198 - Flags: review?(mrbkap)
Attachment #8784198 - Flags: review?(enndeakin)
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 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
Attachment #8784198 - Flags: review?(enndeakin) → review+
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.
Attachment #8784198 - Flags: review?(mrbkap) → review+
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.
Attachment #8784197 - Flags: review?(mrbkap) → review-
Attachment #8784197 - Flags: review?(enndeakin)
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 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!
Attachment #8784197 - Flags: review?(mrbkap) → review+
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 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.
Attachment #8786534 - Flags: review+
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.
Attachment #8784197 - Flags: review?(enndeakin) → review+
Attachment #8784198 - Flags: review?(enndeakin) → review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/54efec27fb40
Add missing newline to end of browser_bug1297539.js to please ESLint.
Attachment #8784197 - Flags: review?(enndeakin) → review+
Attachment #8784198 - Flags: review?(enndeakin) → review+
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
Flags: needinfo?(jimmyw22)
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: