[webkit-reviews] review granted: [Bug 49830] WebKit2: Add Trackpoint driver hack to support IBM trackpads : [Attachment 86809] [PATCH] Fix

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 49830: WebKit2: Add Trackpoint driver hack to support IBM trackpads
https://bugs.webkit.org/show_bug.cgi?id=49830

Attachment 86809: [PATCH] Fix
https://bugs.webkit.org/attachment.cgi?id=86809&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86809&action=review

> Source/WebKit2/UIProcess/win/WebView.cpp:294
> +	   // trackpoint scrollbars, so the WebView will receive WM_VSCROLl and
WM_HSCROLL messages.

They aren't "trackpoint scrollbars", they're just normal Windows scrollbars.

WM_VSCROLl should be WM_VSCROLL.

> Source/WebKit2/UIProcess/win/WebView.cpp:742
> +    const TCHAR trackPointKeys[][50] = {
TEXT("Software\\Lenovo\\TrackPoint"),
> +	   TEXT("Software\\Lenovo\\UltraNav"),
> +	   TEXT("Software\\Alps\\Apoint\\TrackPoint"),
> +	   TEXT("Software\\Synaptics\\SynTPEnh\\UltraNavUSB"),
> +	   TEXT("Software\\Synaptics\\SynTPEnh\\UltraNavPS2") };

I'd put the first key on its own line, and put the closing brace and semicolon
on their own line.

I think a better type for trackPointKeys would be: const wchar_t*
trackPointKeys[].

You should use L instead of TEXT().

> Source/WebKit2/UIProcess/win/WebView.cpp:744
> +    for (int i = 0; i < 5; ++i) {

WTF_ARRAY_LENGTH

> Source/WebKit2/UIProcess/win/WebView.cpp:746
> +	   int readKeyResult = ::RegOpenKeyEx(HKEY_CURRENT_USER,
trackPointKeys[i], 0, KEY_READ, &trackPointKey);

You should use ::RegOpenKeyExW.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:336
> +void WebPage::scrollBy(uint32_t scrollDirection, uint32_t scrollGranularity)

> +{
> +    scroll(m_page.get(), static_cast<ScrollDirection>(scrollDirection),
static_cast<ScrollGranularity>(scrollGranularity));
> +}

Why is this code duplicated in WebPageMac and WebPageWin? Just put it in
WebPage.cpp.

We should probably check that the uint32_ts we get are in the correct range.
You could use helper functions to do that (like toScrollDirection and
toScrollGranularity).