[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]

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.