| Summary: | Support Appkit key bindings and custom key bindings in WebKit2 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||||||
| Component: | WebKit2 | Assignee: | Enrica Casucci <enrica> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abecsi, ap, darin, kbalazs, ossy, webkit-ews, zoltan | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Mac | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Enrica Casucci
2010-10-25 15:31:19 PDT
Created attachment 71808 [details]
patch
Attachment 71808 [details] did not build on qt: Build output: http://queues.webkit.org/results/4779010 Comment on attachment 71808 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=71808&action=review One thing not mentioned in the bug or ChangeLog is that there is a race condition here - the application may be in an entirely different state by the time it gets to re-process the Command-key combo. We should be on lookout for user-visible problems caused by that. Looks good to me. Please address the comments, and Qt build problems. > WebKit2/UIProcess/WebPageProxy.h:289 > + // Keyboard handling > + void interpretKeyEvent(uint32_t, String&); These arguments should have names. It's not at all clear what they are. > WebKit2/UIProcess/API/mac/PageClientImpl.mm:237 > + [m_wkView _resentEvent:nativeEvent]; The pattern is that selector name is a verb: resent (v) feel bitter or indignant about > WebKit2/UIProcess/API/mac/WKView.mm:90 > + NSEvent *_resentKeyDownEvent; // We keep here the event when resending it to the application. Maybe _keyDownEventBeingResent? With that name, the comment seems superfluous, but perhaps you could explain why it needs to be stored for future readers of the code. > WebKit2/UIProcess/API/mac/WKView.mm:91 > + NSString* _currentEditingCommand; Ditto, an explanation of why we need to store this could be helpful. I'm not 100% sure if I understand why this is one command, and not an array. > WebKit2/UIProcess/API/mac/WKView.mm:314 > +- (BOOL)_handleStyleKeyEquivalent:(NSEvent *)event You could add a comment about why we handle italic and bold, but not underline here. See <https://bugs.webkit.org/show_bug.cgi?id=24943#c8> and later comments for some discussion, but I think that boils down to "historic reasons". Comment on attachment 71808 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=71808&action=review > WebKit2/UIProcess/API/mac/WKView.mm:348 > + // Pass command-key combos through WebCore if there is a key binding available for > + // this event. This lets web pages have a crack at intercepting command-modified keypresses. Another thing that bothers me is reliance on the Command key - in comments if not in code. Not all menu commands have it - even Safari has Ctrl+Tab, for example. > One thing not mentioned in the bug or ChangeLog is that there is a race condition here - the application may be in an entirely different state by the time it gets to re-process the Command-key combo. We should be on lookout for user-visible problems caused by that. I will add a comment in the ChangeLog and address all the other comments as well as fixing the qt build. > Ditto, an explanation of why we need to store this could be helpful. I'm not 100% sure if I understand why this is one command, and not an array. You are correct, this should be an array. I will change this. > > > WebKit2/UIProcess/API/mac/WKView.mm:314 > > +- (BOOL)_handleStyleKeyEquivalent:(NSEvent *)event > > You could add a comment about why we handle italic and bold, but not underline here. See <https://bugs.webkit.org/show_bug.cgi?id=24943#c8> and later comments for some discussion, but I think that boils down to "historic reasons". I did not think we needed a comment since we were simply reimplementing the existing WebKit behavior, but I can definitely add one. (In reply to comment #4) > (From update of attachment 71808 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71808&action=review > > > WebKit2/UIProcess/API/mac/WKView.mm:348 > > + // Pass command-key combos through WebCore if there is a key binding available for > > + // this event. This lets web pages have a crack at intercepting command-modified keypresses. > > Another thing that bothers me is reliance on the Command key - in comments if not in code. Not all menu commands have it - even Safari has Ctrl+Tab, for example. I copied the comment from WebKit, but you are right, and I'll change it. Please don’t break the Qt build. ../../../WebKit2/UIProcess/API/qt/qwkpage.cpp:271: error: cannot allocate an object of abstract type ‘QWKPagePrivate’ ../../../WebKit2/UIProcess/API/qt/qwkpage_p.h:37: note: because the following virtual functions are pure within ‘QWKPagePrivate’: ../../../WebKit2/UIProcess/PageClient.h:62: note: virtual void WebKit::PageClient::interceptKeyEvent(const WebKit::NativeWebKeyboardEvent&, WTF::String&) I guess we’ll need to implement interceptKeyEvent for Qt or figure out why the ifdefs aren’t doing the right thing. (In reply to comment #7) > I guess we’ll need to implement interceptKeyEvent for Qt or figure out why the ifdefs aren’t doing the right thing. I cc-ed our WebKit2 guys. Could you check this Qt-WebKit2 break? I see now that Alexey already requested that you fix the Qt build problem, and I trust you will do so. Created attachment 71939 [details]
New patch
addressed the comments.
Comment on attachment 71939 [details] New patch View in context: https://bugs.webkit.org/attachment.cgi?id=71939&action=review r=me. We'll likely learn even more about all this while reimplementing input method support! > WebKit2/UIProcess/API/mac/PageClientImpl.mm:237 > + [m_wkView _setEventToResend:nativeEvent]; Can we say "_setEventBeingResent"? > WebKit2/UIProcess/API/mac/WKView.mm:92 > + NSEvent *_keyDownEventBeingResent; // We keep here the event when resending it to > + // the application to distinguish the case of a new event from one > + // that has been already sent to WebCore. We usually prefer to have a long comment before code, rather than to have to align it. Attachment 71939 [details] did not build on qt: Build output: http://queues.webkit.org/results/4751028 Will do. Thanks! I noticed that the qt build is still broken. I'll fix it before landing the patch. Created attachment 71945 [details]
Patch 3
Fixing the qt build (hopefully).
Comment on attachment 71945 [details]
Patch 3
Set r? for EWS bots
Comment on attachment 71945 [details]
Patch 3
Remove r?, because it can't be applied now. (WebCore/WebCore.xcodeproj/project.pbxproj)
Could you update to ToT, please?
Comment on attachment 71945 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=71945&action=review > WebKit2/UIProcess/API/qt/qwkpage_p.h:59 > + virtual void interceptKeyEvent(const WebKit::NativeWebKeyboardEvent& event, WTF:Vector<WTF:String>& commandName); Typo. You probably wanted to use scope operators (::) for WTF. After fixing these the patch also builds on Qt. Thank you very much for looking after the Qt build issue. Created attachment 72109 [details]
Patch4
I found a bug in my previous patch.
Comment on attachment 72109 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=72109&action=review > WebCore/dom/KeyboardEvent.h:36 > + KeypressCommand() { } I don't immediately see why a default constructor is now needed. Could you add a comment? > WebCore/dom/KeyboardEvent.h:38 > + KeypressCommand(const String& commandName, const String& text) : commandName(commandName), text(text) { ASSERT(commandName == "insertText:" || commandName == "insertText"); } This is surprising and fragile. How hard would it be to ensure that only one form is seen by KeypressCommand? Unless I'm mistaken, that would be a one byte change. > WebKit2/UIProcess/WebPageProxy.h:304 > +#if PLATFORM(MAC) > + // Keyboard handling > + void interpretKeyEvent(uint32_t eventType, Vector<WebCore::KeypressCommand>&); > +#endif How do we need to send the event back to application on other platforms for menu shortcut processing? > WebKit2/UIProcess/API/mac/WKView.mm:324 > + // This should not be changed, since it could break some Mac applications that > + // rely on this inherent behavior. > + // See https://bugs.webkit.org/show_bug.cgi?id=24943 That's a stronger comment than the one made by Darin in bug 24943. I'm not sure which is more accurate. > Could you add a comment?
In ChangeLog, of course.
> I don't immediately see why a default constructor is now needed. Could you add a comment? It is needed for the generated encoder/decoder class. I'll add a comment in ChangeLog. > > WebCore/dom/KeyboardEvent.h:38 > > + KeypressCommand(const String& commandName, const String& text) : commandName(commandName), text(text) { ASSERT(commandName == "insertText:" || commandName == "insertText"); } > > This is surprising and fragile. How hard would it be to ensure that only one form is seen by KeypressCommand? Unless I'm mistaken, that would be a one byte change. I did it this way to avoid changing WebKit code and I don't want to pass selector strings to the webprocess. I prefer to keep it this way. > > WebKit2/UIProcess/WebPageProxy.h:304 > > +#if PLATFORM(MAC) > > + // Keyboard handling > > + void interpretKeyEvent(uint32_t eventType, Vector<WebCore::KeypressCommand>&); > > +#endif > > How do we need to send the event back to application on other platforms for menu shortcut processing? This patch is only about supporting Mac. Other platforms will be supported later. > > WebKit2/UIProcess/API/mac/WKView.mm:324 > > + // This should not be changed, since it could break some Mac applications that > > + // rely on this inherent behavior. > > + // See https://bugs.webkit.org/show_bug.cgi?id=24943 > > That's a stronger comment than the one made by Darin in bug 24943. I'm not sure which is more accurate. Me neither. I'll stick with this. Committed revision 70730. > I did it this way to avoid changing WebKit code and I don't want to pass selector strings to the webprocess. I prefer to keep it this way.
Another way to reduce the variance would be changing WebCore code to use command name without the semicolon.
Editor command names start with an upper case letter. This patch introduces a third way to store and pass commands ("insertText:", "InsertText", and "insertText") - I don't think we had that many before.
|