Bug 131992 – Change Image Controls replacement to use selection and paste.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 229893 [details]
Patch v1

[Tim Horton]

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 ]

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 229897 [details]
Patch for landing maybe.

[WebKit Commit Bot]

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

[Brady Eidson]

Created attachment 229903 [details]
Patch for landing hopefully

[WebKit Commit Bot]

Comment on attachment 229903 [details]
Patch for landing hopefully

Clearing flags on attachment: 229903

Committed r167674: <http://trac.webkit.org/changeset/167674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 132025

[Brady Eidson]

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.

[Brady Eidson]

This is to avoid the normal "pasteboard-related sync IPC" that the first patch used.

[Tim Horton]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 230411 [details]
New approach, v2

[Tim Horton]

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

[WebKit Commit Bot]

Comment on attachment 230411 [details]
New approach, v2

Clearing flags on attachment: 230411

Committed r167956: <http://trac.webkit.org/changeset/167956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.