Bug 134807 - Add a mechanism to notify the UIProcess when an editing command is done executing
Summary: Add a mechanism to notify the UIProcess when an editing command is done execu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-10 11:40 PDT by Enrica Casucci
Modified: 2014-07-10 16:26 PDT (History)
2 users (show)

See Also:


Attachments
Patch (17.83 KB, patch)
2014-07-10 11:50 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch2 (14.48 KB, patch)
2014-07-10 15:38 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch3 (14.39 KB, patch)
2014-07-10 16:20 PDT, Enrica Casucci
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2014-07-10 11:40:06 PDT
Some editing commands have an effect on some parts of the system that run inside the UIProcess. A good example are the cursor movement commands that require an update of the autocorrection/autosuggestion machinery.
We need a way to reliably know when the command has been executed in the WebProcess.
Comment 1 Enrica Casucci 2014-07-10 11:50:24 PDT
Created attachment 234714 [details]
Patch
Comment 2 Tim Horton 2014-07-10 11:59:48 PDT
mitz makes a good point! WebKit2 IPC has a nice callback mechanism you can use, and it makes sure that callbacks get invalidated e.g. when the process dies, stuff like that! You should probably be using it.
Comment 3 Enrica Casucci 2014-07-10 14:24:59 PDT
(In reply to comment #2)
> mitz makes a good point! WebKit2 IPC has a nice callback mechanism you can use, and it makes sure that callbacks get invalidated e.g. when the process dies, stuff like that! You should probably be using it.

I though about it and it seems heavier than just passing a parameter, but I agree it has the advantage of having the cleanup ready.
Thanks for the feedback, I'll do that.
Comment 4 Enrica Casucci 2014-07-10 15:38:52 PDT
Created attachment 234727 [details]
Patch2
Comment 5 WebKit Commit Bot 2014-07-10 15:41:28 PDT
Attachment 234727 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:380:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:362:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Tim Horton 2014-07-10 15:46:06 PDT
Comment on attachment 234727 [details]
Patch2

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2231
> +    _page->executeEditCommand(commandName, [self](const String& string, CallbackBase::Error error) {

I think VoidCallback is sufficient, isn't it? You don't need StringCallback.
Comment 7 Enrica Casucci 2014-07-10 15:50:39 PDT
(In reply to comment #6)
> (From update of attachment 234727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234727&action=review
> 
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2231
> > +    _page->executeEditCommand(commandName, [self](const String& string, CallbackBase::Error error) {
> 
> I think VoidCallback is sufficient, isn't it? You don't need StringCallback.

Probably yes. I don't need to check the command name. It ok with you with that change?
Comment 8 Tim Horton 2014-07-10 15:56:07 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 234727 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=234727&action=review
> > 
> > > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2231
> > > +    _page->executeEditCommand(commandName, [self](const String& string, CallbackBase::Error error) {
> > 
> > I think VoidCallback is sufficient, isn't it? You don't need StringCallback.
> 
> Probably yes. I don't need to check the command name. It ok with you with that change?

You ought never need to, because you already know which command it was by the state around the callback.

r=me with that change.
Comment 9 Enrica Casucci 2014-07-10 16:20:55 PDT
Created attachment 234730 [details]
Patch3
Comment 10 WebKit Commit Bot 2014-07-10 16:22:37 PDT
Attachment 234730 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:380:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:362:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Enrica Casucci 2014-07-10 16:26:48 PDT
Committed revision 170981.