Closed
Bug 1297539
Opened 8 years ago
Closed 8 years ago
Handle eContentCommandPasteTransferable for remote targets
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jimicy, Assigned: jimicy)
References
Details
Attachments
(4 files)
EventStateManager::DoContentCommandEvent does not handle eContentCommandPasteTransferable for remote targets.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
mozreview-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/#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
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-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/#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
Assignee | ||
Comment 5•8 years ago
|
||
mozreview-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/#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
Assignee | ||
Updated•8 years ago
|
Attachment #8784197 -
Flags: review?(mrbkap)
Attachment #8784197 -
Flags: review?(enndeakin)
Attachment #8784198 -
Flags: review?(mrbkap)
Attachment #8784198 -
Flags: review?(enndeakin)
Comment 6•8 years ago
|
||
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•8 years ago
|
||
mozreview-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/#review72384
Updated•8 years ago
|
Attachment #8784198 -
Flags: review?(enndeakin) → review+
Comment 8•8 years ago
|
||
mozreview-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 9•8 years ago
|
||
mozreview-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-
Updated•8 years ago
|
Attachment #8784197 -
Flags: review?(enndeakin)
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-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 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 15•8 years ago
|
||
mozreview-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 16•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Attachment #8784197 -
Flags: review?(enndeakin) → review+
Updated•8 years ago
|
Attachment #8784198 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Try push looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=a391f0c2c19f
Keywords: checkin-needed
Comment 22•8 years ago
|
||
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
Comment 23•8 years ago
|
||
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.
![]() |
||
Comment 24•8 years ago
|
||
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
Flags: needinfo?(jimmyw22)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Attachment #8784197 -
Flags: review?(enndeakin) → review+
Updated•8 years ago
|
Attachment #8784198 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 28•8 years ago
|
||
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
Comment 29•8 years ago
|
||
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
Assignee | ||
Comment 30•8 years ago
|
||
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3bb42e644304 https://hg.mozilla.org/mozilla-central/rev/4e15e5606976 https://hg.mozilla.org/mozilla-central/rev/6f406d30eddb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•