Bug 131992 - Change Image Controls replacement to use selection and paste.
Summary: Change Image Controls replacement to use selection and paste.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on: 132025
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-22 09:47 PDT by Brady Eidson
Modified: 2014-04-29 14:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1 (16.40 KB, patch)
2014-04-22 09:57 PDT, Brady Eidson
thorton: review+
Details | Formatted Diff | Diff
Patch for landing maybe. (16.97 KB, patch)
2014-04-22 11:02 PDT, Brady Eidson
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing hopefully (17.05 KB, patch)
2014-04-22 11:47 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch with new approach - v1 (36.32 KB, patch)
2014-04-29 13:12 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
New approach, v2 (36.18 KB, patch)
2014-04-29 13:56 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-04-22 09:47:47 PDT
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>
Comment 1 Brady Eidson 2014-04-22 09:57:26 PDT
Created attachment 229893 [details]
Patch v1
Comment 2 Tim Horton 2014-04-22 10:04:57 PDT
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 ]
Comment 3 Brady Eidson 2014-04-22 10:39:45 PDT
(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.
Comment 4 Brady Eidson 2014-04-22 11:02:50 PDT
Created attachment 229897 [details]
Patch for landing maybe.
Comment 5 WebKit Commit Bot 2014-04-22 11:34:04 PDT
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
Comment 6 Brady Eidson 2014-04-22 11:47:21 PDT
Created attachment 229903 [details]
Patch for landing hopefully
Comment 7 WebKit Commit Bot 2014-04-22 12:22:56 PDT
Comment on attachment 229903 [details]
Patch for landing hopefully

Clearing flags on attachment: 229903

Committed r167674: <http://trac.webkit.org/changeset/167674>
Comment 8 WebKit Commit Bot 2014-04-22 12:22:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2014-04-22 14:57:33 PDT
Re-opened since this is blocked by bug 132025
Comment 10 Brady Eidson 2014-04-29 13:12:25 PDT
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.
Comment 11 Brady Eidson 2014-04-29 13:12:56 PDT
This is to avoid the normal "pasteboard-related sync IPC" that the first patch used.
Comment 12 Tim Horton 2014-04-29 13:24:24 PDT
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.
Comment 13 Brady Eidson 2014-04-29 13:37:55 PDT
(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.
Comment 14 Brady Eidson 2014-04-29 13:56:59 PDT
Created attachment 230411 [details]
New approach, v2
Comment 15 Tim Horton 2014-04-29 14:24:35 PDT
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 16 WebKit Commit Bot 2014-04-29 14:54:52 PDT
Comment on attachment 230411 [details]
New approach, v2

Clearing flags on attachment: 230411

Committed r167956: <http://trac.webkit.org/changeset/167956>
Comment 17 WebKit Commit Bot 2014-04-29 14:54:56 PDT
All reviewed patches have been landed.  Closing bug.