Bug 49829 - WebKit2: Add WKViewRef API for executing edit commands
Summary: WebKit2: Add WKViewRef API for executing edit commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Jeff Miller
URL:
Keywords: InRadar, PlatformOnly
: 55710 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-11-19 14:01 PST by Sam Weinig
Modified: 2011-03-03 21:28 PST (History)
2 users (show)

See Also:


Attachments
Patch (3.40 KB, patch)
2011-03-03 14:54 PST, Jeff Miller
no flags Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2011-03-03 16:05 PST, Jeff Miller
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-11-19 14:01:21 PST
We need API for SelectAll, Undo and Redo on WKView. In WebKit1, this was handled by a custom WM_COMMAND, but there is no reason to copy that approach.
Comment 1 Sam Weinig 2010-11-29 09:29:41 PST
<rdar://problem/8705950>
Comment 2 Jeff Miller 2011-03-03 14:46:04 PST
*** Bug 55710 has been marked as a duplicate of this bug. ***
Comment 3 Jeff Miller 2011-03-03 14:47:54 PST
I believe the commands we need to support are cut/copy/paste, delete, select all, and undo/redo.
Comment 4 Jeff Miller 2011-03-03 14:54:10 PST
Created attachment 84632 [details]
Patch
Comment 5 Darin Adler 2011-03-03 15:38:57 PST
Comment on attachment 84632 [details]
Patch

I think this should be a WKPage API, not a WKView API. I also think this should take WebCore command names rather than a fixed set of command numbers.

I’m going to say review- for now, but you can put this back up for review if others disagree.
Comment 6 Jeff Miller 2011-03-03 15:47:29 PST
Because this is a Windows-specific API, both Sam and Anders agree it should be in WKView, since WKPage has no Windows-specific implementation.  I would rather use the WebCore Editor command name strings directly, but I'm not sure what the best way to expose these through the API (if we need to do that at all, we could just assume the client knows what the command names are, I suppose).
Comment 7 Jeff Miller 2011-03-03 15:50:21 PST
I suggested to Anders that we could just document the Editor command names in a comment block in WKView.h, and he agreed.  I'll submit another patch that uses the command names instead, but I'm going to leave the API in WKView.
Comment 8 Jeff Miller 2011-03-03 16:05:46 PST
Created attachment 84645 [details]
Patch
Comment 9 Anders Carlsson 2011-03-03 16:08:33 PST
Comment on attachment 84645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84645&action=review

> Source/WebKit2/UIProcess/API/C/win/WKView.h:54
> +    Valid command name strings include:

Maybe this could mention that it accepts all WebCore editing commands?

> Source/WebKit2/UIProcess/API/C/win/WKView.h:64
> +WK_EXPORT void WKViewExecuteCommand(WKViewRef view, WKStringRef command);

It's probably simpler if this takes a const char* since edit commands are all ASCII and that would make it easier for API clients.
Comment 10 Anders Carlsson 2011-03-03 16:09:25 PST
Comment on attachment 84645 [details]
Patch

Nah, it's fine to take a WKStringRef. I changed my mind.
Comment 11 Jeff Miller 2011-03-03 16:12:02 PST
Committed r80295: <http://trac.webkit.org/changeset/80295>
Comment 12 Darin Adler 2011-03-03 16:59:54 PST
(In reply to comment #6)
> Because this is a Windows-specific API

Why is this a Windows-specific API?
Comment 13 Darin Adler 2011-03-03 17:00:15 PST
I’d love to have this API on all platforms.
Comment 14 Jeff Miller 2011-03-03 17:38:06 PST
(In reply to comment #13)
> I’d love to have this API on all platforms.

There's a semantic mismatch between Mac and Windows in where menu items are set up (and executed).

On the Mac, we can do this all in the OpenSource side of the world in WKView.mm. On Windows, though, we have to do all this in the app.

According to Sam, we're not defining the final WebKit2 API set at this point, we're focused on making the API work for Safari specifically in the short term.  We can certainly revisit this when we are ready to think about an API design that will work for more clients.
Comment 15 Darin Adler 2011-03-03 18:07:55 PST
I still think this call should have gone on WKPage. I don’t see any argument here for why putting it on WKView is superior. The argument seems to be “there is code a little like this in the Mac’s WKView” and “we haven’t needed this on Mac” and neither convinces me. I’m slightly annoyed that we added this to what seems to me to be the wrong class.
Comment 16 Jeff Miller 2011-03-03 21:20:20 PST
OK, I will move this API to WKPage.  New bug coming soon.
Comment 17 Jeff Miller 2011-03-03 21:28:43 PST
Created bug 55744.