[webkit-reviews] review granted: [Bug 56020] WebKit2: Pressing Tab in Web Inspector's console does not cycle through completion options : [Attachment 89235] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 12 11:56:12 PDT 2011
Adam Roben (:aroben) <aroben at apple.com> has granted Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 56020: WebKit2: Pressing Tab in Web Inspector's console does not cycle
through completion options
https://bugs.webkit.org/show_bug.cgi?id=56020
Attachment 89235: Patch
https://bugs.webkit.org/attachment.cgi?id=89235&action=review
------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=89235&action=review
> Tools/ChangeLog:12
> + WebKit2: Pressing Tab in Web Inspector's console does not cycle
through completion options
> + https://bugs.webkit.org/show_bug.cgi?id=56020
> +
> + Don't call TranslateMessage() in the MiniBrowser or TestWebKitAPI
for key messages destined for a WebKit2 view,
> + since WebKit will do this for us. If we didn't do this,
TranslateMessage() would be called twice,
> + which would generate two characters for every keypress (for
example).
> +
> + Add new WebKit2/TranslateMessageGeneratesWMChar test to test
expected TranslateMessage() behavior.
I think it's worth adding a comment about why we didn't bother to modify
WebKitTestRunner.
> Tools/MiniBrowser/win/main.cpp:44
> +static bool shouldTranslateMessage(MSG msg)
Can use a const MSG&.
> Tools/MiniBrowser/win/main.cpp:48
> + if ((msg.message == WM_KEYDOWN || msg.message == WM_SYSKEYDOWN ||
msg.message == WM_KEYUP || msg.message == WM_SYSKEYUP)) {
Making this an early return would be better.
> Tools/MiniBrowser/win/main.cpp:55
> + TCHAR className[256];
> + if (!::GetClassName(msg.hwnd, className, ARRAYSIZE(className)))
> + return true;
> +
> + // Don't call TranslateMessage() on key events destined for a
WebKit2 view, WebKit will do this if it doesn't handle the message.
> + // It would be nice to use some API here instead of hard-coding the
window class name.
> + return _tcscmp(className, _T("WebKit2WebViewWindowClass"));
In new code we've been avoiding TCHAR and using wchar_t directly. I think that
would be good to do here, too.
> Tools/TestWebKitAPI/PlatformUtilities.h:41
> +bool shouldTranslateMessage(MSG);
Could pass a const MSG&.
> Tools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:36
> +#include <tchar.h>
In
> Tools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:76
> +bool shouldTranslateMessage(MSG msg)
> +{
> + // Only these four messages are actually translated by
::TranslateMessage or ::TranslateAccelerator.
> + // It's useless (though harmless) to call those functions for other
messages, so we always allow other messages to be translated.
> + if ((msg.message == WM_KEYDOWN || msg.message == WM_SYSKEYDOWN ||
msg.message == WM_KEYUP || msg.message == WM_SYSKEYUP)) {
> + TCHAR className[256];
> + if (!::GetClassName(msg.hwnd, className, ARRAYSIZE(className)))
> + return true;
> +
> + // Don't call TranslateMessage() on key events destined for a
WebKit2 view, WebKit will do this if it doesn't handle the message.
> + // It would be nice to use some API here instead of hard-coding the
window class name.
> + return _tcscmp(className, _T("WebKit2WebViewWindowClass"));
> + }
> +
> + return true;
> +}
Same comments here as for MiniBrowser.
More information about the webkit-reviews
mailing list