Tracks the work required to be able to drag an attachment element out of a WebKit view and into a different target. rdar://problem/19982527
Created attachment 248886 [details] Patch
Attachment 248886 [details] did not pass style-queue: ERROR: Source/WebCore/page/DragController.cpp:51: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
I fixed the include order in DragController.cpp after posting the patch.
Comment on attachment 248886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248886&action=review > Source/WebCore/page/DragController.cpp:629 > + // Attachment elements can have DragSourceActionNone even if > + // selected, if the selection contains only the attachement element. This is not a 'why?' comment, but I wish it were. > Source/WebCore/page/DragController.cpp:759 > + m_draggingAttachmentURL = URL(); Vaguely interesting that you reset m_draggingAttachmentURL here, but this isn't where m_draggingImageURL is reset. But maybe there's a reason! > Source/WebCore/rendering/HitTestResult.cpp:330 > + return m_innerNonSharedNode->document().completeURL(attachmentFile->path()); This doesn't look right... is it? Is the file relative to the document's URL? Why are we completeURLing? > Source/WebKit2/UIProcess/mac/PageClientImpl.mm:410 > void PageClientImpl::setPromisedData(const String& pasteboardName, PassRefPtr<SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title, const String& url, const String& visibleUrl, PassRefPtr<SharedBuffer> archiveBuffer) It's pretty weird that this function takes an ImageBuffer and we call it even in the non-image case. Maybe it needs a rename and duplication? > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:413 > + RefPtr<SharedMemory> sharedMemoryImage = (!imageHandle.isNull()) ? SharedMemory::create(imageHandle, SharedMemory::ReadOnly) : nullptr; Ditto here, I think this should be written differently. Maybe split into setPromisedData and setPromisedImage (and setPromisedImage calls setPromisedData after extracting the data?) > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:275 > + NSMutableArray *types = [[NSMutableArray alloc] initWithObjects:NSFilesPromisePboardType, nil]; Isn't this leaking? > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:292 > + [paths.get() addObject:title]; no .get()
Created attachment 248893 [details] Patch that fixes builds and style
(In reply to comment #4) > Comment on attachment 248886 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248886&action=review > > > Source/WebCore/page/DragController.cpp:629 > > + // Attachment elements can have DragSourceActionNone even if > > + // selected, if the selection contains only the attachement element. > > This is not a 'why?' comment, but I wish it were. I don't understand your comment. This is exactly what we do. What am I missing? > > > Source/WebCore/page/DragController.cpp:759 > > + m_draggingAttachmentURL = URL(); > > Vaguely interesting that you reset m_draggingAttachmentURL here, but this > isn't where m_draggingImageURL is reset. But maybe there's a reason! Moved it up simply not to add another #if ENABLE(ATTACHMENT_ELEMENT) > > > Source/WebCore/rendering/HitTestResult.cpp:330 > > + return m_innerNonSharedNode->document().completeURL(attachmentFile->path()); > > This doesn't look right... is it? Is the file relative to the document's > URL? Why are we completeURLing? I believe you are right, copy paste mistake. > > > Source/WebKit2/UIProcess/mac/PageClientImpl.mm:410 > > void PageClientImpl::setPromisedData(const String& pasteboardName, PassRefPtr<SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title, const String& url, const String& visibleUrl, PassRefPtr<SharedBuffer> archiveBuffer) > > It's pretty weird that this function takes an ImageBuffer and we call it > even in the non-image case. Maybe it needs a rename and duplication? > > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:413 > > + RefPtr<SharedMemory> sharedMemoryImage = (!imageHandle.isNull()) ? SharedMemory::create(imageHandle, SharedMemory::ReadOnly) : nullptr; > > Ditto here, I think this should be written differently. Maybe split into > setPromisedData and setPromisedImage (and setPromisedImage calls > setPromisedData after extracting the data?) > I was not entirely convinced to duplicate the code, but maybe I should... > > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:275 > > + NSMutableArray *types = [[NSMutableArray alloc] initWithObjects:NSFilesPromisePboardType, nil]; > > Isn't this leaking? I only moved up an existing line. I've noticed that in many places where declareTypes is called we don't worry about the array we pass. > > > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:292 > > + [paths.get() addObject:title]; > > no .get() ok.
(In reply to comment #6) > (In reply to comment #4) > > Comment on attachment 248886 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=248886&action=review > > > > > Source/WebCore/page/DragController.cpp:629 > > > + // Attachment elements can have DragSourceActionNone even if > > > + // selected, if the selection contains only the attachement element. > > > > This is not a 'why?' comment, but I wish it were. > I don't understand your comment. This is exactly what we do. What am I > missing? Exactly. The comment is describing what the code does, but not why it does that. If I wanted to know what it does, I would read the code. If I wanted to know why it does it... I would come ask you, because the comment doesn't help me. > > > > > Source/WebCore/page/DragController.cpp:759 > > > + m_draggingAttachmentURL = URL(); > > > > Vaguely interesting that you reset m_draggingAttachmentURL here, but this > > isn't where m_draggingImageURL is reset. But maybe there's a reason! > Moved it up simply not to add another #if ENABLE(ATTACHMENT_ELEMENT) Ah! OK. > > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:413 > > > + RefPtr<SharedMemory> sharedMemoryImage = (!imageHandle.isNull()) ? SharedMemory::create(imageHandle, SharedMemory::ReadOnly) : nullptr; > > > > Ditto here, I think this should be written differently. Maybe split into > > setPromisedData and setPromisedImage (and setPromisedImage calls > > setPromisedData after extracting the data?) > > > I was not entirely convinced to duplicate the code, but maybe I should... It doesn't need to be much duplication (I think!) if you have setPromisedImage extract the image data and then just go ahead and call setPromisedData. > > > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:275 > > > + NSMutableArray *types = [[NSMutableArray alloc] initWithObjects:NSFilesPromisePboardType, nil]; > > > > Isn't this leaking? > I only moved up an existing line. I've noticed that in many places where > declareTypes is called we don't worry about the array we pass. Oh! No, there's an explicit -release sent there (it's now far away from the alloc/init, so I missed it). This should probably use RetainPtr, but I don't care if you fix it in this patch.
Does this patch introduce sandbox escape opportunities? Can malicious native code running inside WebContent process fool UI process into giving it full file system access? Files inside drag normally result in getting such access.
Created attachment 248979 [details] Patch3 This patch addresses the comments and the security concerns.
Created attachment 248983 [details] Patch4 Trying to make EWS happy.
Attachment 248983 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/PageClient.h:198: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 248983 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=248983&action=review > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:105 > + NSURL* nsURL = (NSURL *)url; one of the stars is on the wrong side > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:292 > + [paths.get() addObject:title]; no .get() > LayoutTests/ChangeLog:13 > +<<<<<<< .mine Something bad happened here. > LayoutTests/editing/pasteboard/drag-and-drop-attachment-contenteditable.html:1 > + blank line? > LayoutTests/editing/pasteboard/drag-and-drop-attachment-contenteditable.html:26 > + shouldBe('target.getElementsByTagName("attachment").length', '1'); indentation error > LayoutTests/editing/pasteboard/drag-and-drop-attachment-contenteditable.html:32 > +{ this { is inconsistent with the others.
Committed revision 181760.