Closed
Bug 1071562
Opened 9 years ago
Closed 9 years ago
[e10s] Support non-text content types for content process clipboard
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: bullionareboy, Assigned: enndeakin)
References
Details
Attachments
(3 files, 7 obsolete files)
40.77 KB,
patch
|
Details | Diff | Splinter Review | |
4.04 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0 Build ID: 20140922030204 Steps to reproduce: 1. Open either of these links: http://hotgeekz.com/wp-content/uploads/2014/01/BFHelmet.jpg http://static.comicvine.com/uploads/original/11/116475/4035926-boba+fett.png 2. Right click -> Copy image. 3. Try paste in Gimp - Image not copied to clipboard Note: e10s only, works perfect in non-e10s window Actual results: Image not getting copied to clipboard Expected results: image should have copied and user must be able to paste elsewhere
Comment 1•9 years ago
|
||
This image copy bug might be related to or a dupe of drag/drop bug 936092.
Oh, no, this is separate. When I added clipboard support, I only made it work for text.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [e10s] copy image not copying image to clipboard → [e10s] Support non-text content types for content process clipboard
Updated•9 years ago
|
Updated•9 years ago
|
Flags: firefox-backlog+
Updated•9 years ago
|
Assignee: nobody → ally
Comment 5•9 years ago
|
||
Good to see this having ownership! Thanks! I have to switch to Chrome every time I need to copy/paste in Google Docs.
Comment 6•9 years ago
|
||
No pressure. :p We're polishing off the m4 milestone and moving onto m5 bugs, which this is part of. We didn't forget it, I promise.
Comment 7•9 years ago
|
||
Comment 8•9 years ago
|
||
code in wip when run produces expected output. However, when the message manager sends cmd_copyImage, the child never gets the message, unlike cmd_copy which arrives in the child just fine.
Comment 9•9 years ago
|
||
gets msg in nsDocShell::DoCommand *
Comment 10•9 years ago
|
||
> xul.dll!nsXULPopupManager::GetLastTriggerNode(nsIDocument * aDocument, bool aIsTooltip) Line 1547 C++
xul.dll!nsXULPopupManager::GetLastTriggerPopupNode(nsIDocument * aDocument) Line 510 C++
xul.dll!nsDocumentViewer::GetPopupNode(nsIDOMNode * * aNode) Line 3410 C++
xul.dll!nsDocumentViewer::GetPopupImageNode(nsIImageLoadingContent * * aNode) Line 3469 C++
xul.dll!nsDocumentViewer::GetInImage(bool * aInImage) Line 3524 C++
xul.dll!nsClipboardImageCommands::IsClipboardCommandEnabled(const char * aCommandName, nsIContentViewerEdit * aEdit, bool * outCmdEnabled) Line 680 C++
xul.dll!nsSelectionCommand::IsCommandEnabled(const char * aCommandName, nsISupports * aCommandContext, bool * outCmdEnabled) Line 582 C++
xul.dll!nsControllerCommandTable::IsCommandEnabled(const char * aCommandName, nsISupports * aCommandRefCon, bool * aResult) Line 102 C++
xul.dll!nsBaseCommandController::IsCommandEnabled(const char * aCommand, bool * aResult) Line 105 C++
xul.dll!nsDocShell::IsCommandEnabled(const char * inCommand, bool * outEnabled) Line 12899 C++
For those playing along at home, this is as deep as the stack gets for cmd_copyImage.
GetLastTriggerNode has
nsMenuChainItem* item = aIsTooltip ? mNoHidePanels : mPopups;
item --> null.
parameter aIsTooltip is false. mNoHidePanels & mPopups are both NULL.
Comment 11•9 years ago
|
||
mNoHidePanels & mPopups are only set to nonnull values in nsXULPopupManager::ShowPopupCallback()
Comment 12•9 years ago
|
||
For those playing along at home: : the parent process's nsXULPopupManager has state matching single process nsXULPopupManager. However, the child receives none of that state, specifically around mPopups :the only thing used in nsXULPopupManager is getLastTriggerPopupNode, which is consumed by XULDocument.cpp & nsDocumentViewer only. Sending just the event doesn't cause the nsXULPopupManager to update state in the child To make this work at the nsXULPopupManager level, we'd need to keep the two copies in synch. However, code is triggered on every click, and we cannot afford a synchronous msg with every mouse click. New plan is to route around the nsXULPopupManager entirely in nsDocumentViewer and try to find an acceptable node in nsDocumentViewer.
Comment 13•9 years ago
|
||
Spent some time hacking around on this and trying different things, however none of them to seem to pan out around the popupnode. Spent some more time talking to dbaron & khuey about documentviewer. What we're actually looking for is the node that the popup was rooted in, not the popup node itself(the node we right clicked on), which in single process the popup system keeps track of. Since the child doesn't get any of that we're going to have to find another place to store & access that information in the child.
Comment 14•9 years ago
|
||
Largely in case Jim wants to play with it or view the inline notes.
Attachment #8534757 -
Attachment is obsolete: true
Just a note that bug 936092 has some code for dealing with transferables that would probably be useful here. It sounds like that's not the main problem though.
Comment 16•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #15) > Just a note that bug 936092 has some code for dealing with transferables > that would probably be useful here. It sounds like that's not the main > problem though. I had a solid look. It'll be helpful once we can get that far. First I have to convince firefox that we can execute the command, preferably while supplying the root node properly.
Comment 17•9 years ago
|
||
progress: The code progresses to nsDocShell::DoCommand (means the check is now passing). However controller->DoCommand() fails, so onto the next problem.
Comment 19•9 years ago
|
||
Enn, This bug hinges on the ability of the child process to find the parent root node of the popup(to figure out if it can be copied, to actually find the thing to copy, etc). In single process firefox that information is managed by the Popup node manager and updated every mouse move. In multiprocess, the popups are in the parent process, so the xul popup node manager in the child is always empty and therefore the child thinks there is never anything valid to copy. If I lie to the child, it is still unable to copy because of the missing information. In particular, I have been looking at cmd_CopyImage, whose commandEnabled's stack eventually ends up in nsDocumentViewer::GetInImage(bool* aInImage) which fails on nsresult rv = GetPopupImageNode(getter_AddRefs(node)); which boils down to a call to nsXULPopupManager::>GetLastTriggerPopupNode() What is the best way to reconcile dependencies on the nsXULPopupManager with multiprocess?
Updated•9 years ago
|
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 20•9 years ago
|
||
Bug 1058712 already has a patch for copy image, so the best way is to just apply that patch. :) This bug is supposed to be for implementing the copying of images across processes and onto the clipboard. I think smaug may have done something that can be built on top of in bug 936092 but I haven't looked at the patches there much yet.
Flags: needinfo?(enndeakin)
![]() |
||
Updated•9 years ago
|
Assignee: ally → nobody
Assignee: nobody → wmccloskey
Updated•9 years ago
|
Component: File Handling → DOM: Core & HTML
Product: Firefox → Core
Assignee | ||
Comment 22•9 years ago
|
||
Bill, are you working on this? I have a rough patch that handles copying images to the clipboard.
I was waiting for the drag and drop code to land. Please feel free to take this if you want.
Assignee | ||
Updated•9 years ago
|
Assignee: wmccloskey → enndeakin
Assignee | ||
Updated•9 years ago
|
Points: --- → 8
Assignee | ||
Comment 24•9 years ago
|
||
This patch allow copy and paste of text and html, any other text type and the copying of images. It doesn't yet support image pasting.
Attachment #8539570 -
Attachment is obsolete: true
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Flags: qe-verify?
Assignee | ||
Comment 25•9 years ago
|
||
Several ways to test this once complete: 1. Open data:text/html,<div contenteditable="true">Test <b>Bold</b> Test</div> Then, select some of bold text, copy it, then paste it in the same field. The bold text should be maintained. 2. When bug 1058712 is done, copy image from Firefox and then into the same data url from 1 3. Copy image from Chrome and then into the same area. (3 does not yet work with the attached patch.)
Comment 26•9 years ago
|
||
(one can also test by copying from non-e10s FF window)
Assignee | ||
Comment 27•9 years ago
|
||
This builds on top of the drag and drop patch in bug 936092. Two things I'm not sure about: 1. I added nsCString to IPCDataTransferData to handle images, but I'm not sure what the best approach is here. Is a blob better here and how would I go about creating it? 2. nsContentUtils::TransferableToIPCTransferable also handles files but I haven't tested this yet. There's limited support for cut/paste of files in the widget code.
Attachment #8568642 -
Attachment is obsolete: true
Attachment #8570459 -
Flags: review?(bugs)
Comment 28•9 years ago
|
||
1. Blob might be better, since I guess image data can be large. (dnd might want to use similar setup too in some cases) File::CreateMemoryFile perhaps? (Yes, it is odd that File implements Blob, but http://mxr.mozilla.org/mozilla-central/source/dom/webidl/File.webidl#18) 2. sounds like something to test. nsContentUtils::TransferableToIPCTransferable should just deal with Files, if such are being passed around. looking at the patch...
Comment 29•9 years ago
|
||
Comment on attachment 8570459 [details] [diff] [review] Support non-text copy/paste >@@ -0,0 +1,126 @@ >+// This test is used to check copy and paste in editable areas to enusre that non-text ensure >+// types (html and images) and copied to and pasted from the clipboard properly. s/and/are/ ? >+ // Images to be pasted on the clipboard are nsIInputStreams >+ nsCOMPtr<nsIInputStream> stream(do_QueryInterface(data)); >+ if (stream) { >+ IPCDataTransferItem* item = aIPCDataTransfer->items().AppendElement(); >+ item->flavor() = nsCString(flavorStr); >+ >+ nsCString imageData; >+ NS_ConsumeStream(stream, UINT32_MAX, imageData); >+ item->data() = imageData; >+ continue; Ah this way. I think we may want to have a new type of Blob implementation which can take inputstream as a param. Want to ask baku about it? But perhaps we could do that in a followup. >+ mozilla::RefPtr<mozilla::gfx::DataSourceSurface> dataSurface = >+ surface->GetDataSurface(); >+ mozilla::gfx::DataSourceSurface::MappedSurface map; >+ dataSurface->Map(mozilla::gfx::DataSourceSurface::MapType::READ, &map); >+ mozilla::gfx::IntSize size = dataSurface->GetSize(); >+ mozilla::CheckedInt32 requiredBytes = >+ mozilla::CheckedInt32(map.mStride) * mozilla::CheckedInt32(size.height); >+ size_t bufLen = requiredBytes.isValid() ? requiredBytes.value() : 0; >+ mozilla::gfx::SurfaceFormat format = dataSurface->GetFormat(); >+ bufLen = bufLen - map.mStride + (size.width * BytesPerPixel(format)); All this could use a comment the gfx APIs make it really hard to use the data. We might even want to add a helper method to nsContentUtils or something to get the image data. >+ContentParent::RecvSetClipboard(const IPCDataTransfer& dataTransfer, >+ const bool& isPrivateData, >+ const int32_t& whichClipboard) I wouldn't mind if you used mozilla coding style here. So, aFoo (I know ContentParent uses all sorts of styles) >- // If our data flavor has already been added, this will fail. But we don't care >- trans->AddDataFlavor(kUnicodeMime); >+ auto& items = dataTransfer.items(); I really would prefer the actual type name here. I have no idea about the type of items. (IMHO auto is great for writing code, but not at all great when one needs to read the code.) >+ if (item.data().type() == IPCDataTransferData::TnsString) { >+ nsCOMPtr<nsISupportsString> dataWrapper = >+ do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID, &rv); 2 spaces is enough for indentation >+ const gfxIntSize size(imageDetails.width(), imageDetails.height()); >+ if (!size.width || !size.height) >+ return true; always {} with 'if' >+ContentParent::RecvClipboardHasType(nsTArray<nsCString>&& types, >+ const int32_t& whichClipboard, >+ bool* hasType) aFoo Same also elsewhere. > union IPCDataTransferData > { > nsString; >+ nsCString; Perhaps add a comment that nsCString is for image data. Looks surprisingly simple to me :)
Attachment #8570459 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 30•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #29) > Ah this way. I think we may want to have a new type of Blob implementation > which can take inputstream as a param. > Want to ask baku about it? But perhaps we could do that in a followup. > Ah, there's a patch for this in bug 891247 (patch version 3). I'll look into adapting it. > >- // If our data flavor has already been added, this will fail. But we don't care > >- trans->AddDataFlavor(kUnicodeMime); > >+ auto& items = dataTransfer.items(); > I really would prefer the actual type name here. I have no idea about the > type of > items. I just copied this from your code :)
Assignee | ||
Comment 31•9 years ago
|
||
baku, see comment 29. Essentially, we want to transfer the image data stored in an nsIInputStream and send it to the child process. What is the best way to do this? Going the other way, we currently send images from child to parent using a nsCString.
Flags: needinfo?(amarchesini)
Comment 32•9 years ago
|
||
We are going to have something for Fetch. This is not landed yet. I'll leave this NI so that when that new feature lands, I remember to write a new comment here. Definitely a follow up.
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #8570459 -
Attachment is obsolete: true
Assignee | ||
Comment 34•9 years ago
|
||
Windows is using single-byte strings for html so it needs to be supported. This patch is included in the other one. I just pulled these changes out to make them easier to review.
Attachment #8574667 -
Flags: review?(bugs)
Assignee | ||
Comment 35•9 years ago
|
||
This patch, (also included in the main one), combines the two places where the image surface data is taken into a utility function.
Attachment #8574669 -
Flags: review?(bugs)
Comment 36•9 years ago
|
||
Comment on attachment 8574667 [details] [diff] [review] Extra patch - fix html copy on Windows So many different hacks and inconsistencies in our Transferable handling :/
Updated•9 years ago
|
Attachment #8574667 -
Flags: review?(bugs) → review+
Comment 37•9 years ago
|
||
Comment on attachment 8574669 [details] [diff] [review] Extra patch - merge both surface data handling cases Should the method return const uint8_t* to hint about the ownership? And please document whether caller should free the return value. Why we need nsCString dragImage(dragImagePart); in nsDragServiceProxy.cpp?
Attachment #8574669 -
Flags: review?(bugs) → review+
Updated•9 years ago
|
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Updated•9 years ago
|
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Assignee | ||
Comment 39•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a040986845d
Comment 40•9 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/40bc31482401 https://treeherder.mozilla.org/logviewer.html#?job_id=8711312&repo=mozilla-inbound
Comment 41•9 years ago
|
||
Looks like it angered mochitest-e10s as well. https://treeherder.mozilla.org/logviewer.html#?job_id=8716053&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=8717599&repo=mozilla-inbound
Assignee | ||
Comment 43•9 years ago
|
||
This is the original patch as checked in.
Attachment #8574666 -
Attachment is obsolete: true
Attachment #8574667 -
Attachment is obsolete: true
Attachment #8574669 -
Attachment is obsolete: true
Assignee | ||
Comment 44•9 years ago
|
||
Assignee | ||
Comment 45•9 years ago
|
||
Assignee | ||
Comment 46•9 years ago
|
||
Comment on attachment 8591822 [details] [diff] [review] Part 3 - fix assertions in drag code that doesn't handle nsCStrings. This fixes the assertion.
Attachment #8591822 -
Flags: review?(bugs)
Assignee | ||
Comment 47•9 years ago
|
||
Comment on attachment 8591819 [details] [diff] [review] Part 2 - fix widget build issues Try shows no build or test errors but I've no idea how to test this manually. Hopefully someone can test this out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0bc8959e456 Picking the same reviewer as the original code here.
Attachment #8591819 -
Flags: review?(fabrice)
Updated•9 years ago
|
Attachment #8591819 -
Flags: review?(fabrice) → review+
Updated•9 years ago
|
Attachment #8591822 -
Flags: review?(bugs) → review+
Updated•9 years ago
|
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Comment 48•9 years ago
|
||
Looks like we have reviews here, are we still waiting on something to land this?
Flags: needinfo?(enndeakin)
Comment 50•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/802512ee57ae https://hg.mozilla.org/integration/mozilla-inbound/rev/e812687a81cd https://hg.mozilla.org/integration/mozilla-inbound/rev/ace1964512f5
Comment 51•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/802512ee57ae https://hg.mozilla.org/mozilla-central/rev/e812687a81cd https://hg.mozilla.org/mozilla-central/rev/ace1964512f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•