Change Image Controls replacement to use selection and paste. This is instead of the "replace the render image behind the scenes" implementation we use now. With this we gain a much more robust/tested code path (copy/paste) and undo support. We will end up modifying the DOM, which may or may not be a problem. But probably not. <rdar://problem/16302722>
Created attachment 229893 [details] Patch v1
Comment on attachment 229893 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=229893&action=review > Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:110 > + while (parent) { > + if (parent->isShadowRoot()) { Surprised there's not already something to do this. > Source/WebCore/page/ContextMenuController.cpp:-1456 > -void ContextMenuController::replaceControlledImage(PassRefPtr<Image> newImage) Yayyyyy! > Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:135 > + [pasteboard declareTypes:[NSArray arrayWithObject:NSPasteboardTypeTIFF] owner:nil]; @[ NSPasteboardTypeTIFF ] > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.h:60 > + WebPageProxy* page() const { return m_page; } it looks to me like this should return a reference, no? > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:235 > RetainPtr<CGImageSourceRef> source = adoptCF(CGImageSourceCreateWithData((CFDataRef)[items objectAtIndex:0], NULL)); > RetainPtr<CGImageRef> image = adoptCF(CGImageSourceCreateImageAtIndex(source.get(), 0, NULL)); why are we still doing this part? do you really need to make a CGImageRef here and then not use it? > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:241 > + [pasteboard declareTypes:[NSArray arrayWithObject:NSPasteboardTypeTIFF] owner:nil]; @[ NSPasteboardTypeTIFF ]
(In reply to comment #2) > (From update of attachment 229893 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229893&action=review > > > Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:110 > > + while (parent) { > > + if (parent->isShadowRoot()) { > > Surprised there's not already something to do this. I was too! There probably should be, but I couldn't find it! > > Source/WebCore/page/ContextMenuController.cpp:-1456 > > -void ContextMenuController::replaceControlledImage(PassRefPtr<Image> newImage) > > Yayyyyy! Yayyyy! > > > Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:135 > > + [pasteboard declareTypes:[NSArray arrayWithObject:NSPasteboardTypeTIFF] owner:nil]; > > @[ NSPasteboardTypeTIFF ] Indeed. > > > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.h:60 > > + WebPageProxy* page() const { return m_page; } > > it looks to me like this should return a reference, no? Probably! > > > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:235 > > RetainPtr<CGImageSourceRef> source = adoptCF(CGImageSourceCreateWithData((CFDataRef)[items objectAtIndex:0], NULL)); > > RetainPtr<CGImageRef> image = adoptCF(CGImageSourceCreateImageAtIndex(source.get(), 0, NULL)); > > why are we still doing this part? do you really need to make a CGImageRef here and then not use it? To verify the data actually represents an image. I think its necessary for now, can chat with you on IRC when you're around. > > > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:241 > > + [pasteboard declareTypes:[NSArray arrayWithObject:NSPasteboardTypeTIFF] owner:nil]; > > @[ NSPasteboardTypeTIFF ] Indeed.
Created attachment 229897 [details] Patch for landing maybe.
Comment on attachment 229897 [details] Patch for landing maybe. Rejecting attachment 229897 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ources/WebKit2/WebDatabaseManagerMessageReceiver.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit2.build/Objects-normal/x86_64/WebDatabaseManagerMessageReceiver.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit2.build/Objects-normal/x86_64/WebContextMenuProxyMac.o UIProcess/mac/WebContextMenuProxyMac.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.appspot.com/results/6463773594353664
Created attachment 229903 [details] Patch for landing hopefully
Comment on attachment 229903 [details] Patch for landing hopefully Clearing flags on attachment: 229903 Committed r167674: <http://trac.webkit.org/changeset/167674>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 132025
Created attachment 230407 [details] Patch with new approach - v1 This sends the data to the WebProcess along with the "replace selection" message, and uses a new WebProcess "pasteboard override" mechanism.
This is to avoid the normal "pasteboard-related sync IPC" that the first patch used.
Comment on attachment 230407 [details] Patch with new approach - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=230407&action=review > Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:127 > + > + Frame* frame = document().frame(); > + if (!frame) > + return; > + > + Page* page = document().page(); > + if (!page) > + return; these should be before the while, no? > Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:55 > +static NSString *imageControlPasteboardName = @"WebKitImageControlsPasteboard"; maybe add the s to the variable name > Source/WebKit2/WebProcess/WebCoreSupport/WebPasteboardOverrides.h:45 > + void overriddenTypes(const String& pasteboardName, Vector<String>& types); I think this should have a "get" prefix; or, return the types? why does it work this way? > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:392 > + Vector<char> overrideBuffer; maybe uint8_t here too? > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:394 > + printf("Returning an override buffer!\n"); no.
(In reply to comment #12) > (From update of attachment 230407 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230407&action=review > > > Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:127 > > + > > + Frame* frame = document().frame(); > > + if (!frame) > > + return; > > + > > + Page* page = document().page(); > > + if (!page) > > + return; > > these should be before the while, no? > Probably. > > Source/WebKit/mac/Misc/WebSharingServicePickerController.mm:55 > > +static NSString *imageControlPasteboardName = @"WebKitImageControlsPasteboard"; > > maybe add the s to the variable name Okay. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebPasteboardOverrides.h:45 > > + void overriddenTypes(const String& pasteboardName, Vector<String>& types); > > I think this should have a "get" prefix; or, return the types? why does it work this way? It works this way simply because the only client works this way, too. But that's not really a good reason. Will change. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:392 > > + Vector<char> overrideBuffer; > > maybe uint8_t here too? Either make it a Vector<char> here, or go add an additional constructor to SharedBuffer that takes Vector<uint8_t> (there isn't one). I prefer this way, but could be persuaded otherwise with a reason. > > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:394 > > + printf("Returning an override buffer!\n"); > > no. Awww.
Created attachment 230411 [details] New approach, v2
Comment on attachment 230411 [details] New approach, v2 View in context: https://bugs.webkit.org/attachment.cgi?id=230411&action=review > Source/WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:260 > + ASSERT(m_page); Pass it in as - and store it as - a reference instead :D
Comment on attachment 230411 [details] New approach, v2 Clearing flags on attachment: 230411 Committed r167956: <http://trac.webkit.org/changeset/167956>