[webkit-reviews] review granted: [Bug 14227] Scroll does not work with IBM Thinkpad : [Attachment 47983] Patch

[bugzilla-daemon at webkit.org]

Steve Falkenburg <sfalken at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 14227: Scroll does not work with IBM Thinkpad
https://bugs.webkit.org/show_bug.cgi?id=14227

Attachment 47983: Patch
https://bugs.webkit.org/attachment.cgi?id=47983&action=review

------- Additional Comments from Steve Falkenburg <sfalken at apple.com>

> +void WebView::initializeTrackpointHackIfNecessary(HWND parentWindow)
> +{
> +    const TCHAR trackpointKeys[][50] = {L"Software\\Lenovo\\TrackPoint",
> +	   L"Software\\Lenovo\\UltraNav",
> +	   L"Software\\Alps\\Apoint\\TrackPoint",
> +	   L"Software\\Synaptics\\SynTPEnh\\UltraNavUSB",
> +	   L"Software\\Synaptics\\SynTPEnh\\UltraNavPS2"};

Mixing TCHAR and L" isn't really correct (but won't cause any issues in
practice for us...) since TCHAR's size depends on whether UNICODE is defined,
while L" is always UTF-16.
You could switch from L" to TEXT( or switch from TCHAR to WCHAR. Since you're
passing these into Win32 APIs not suffixed with A or W, TCHAR seems more
appropriate.

> +    for (int i = 0; i < 5; i++) {

We use ++i in most places.

> +	   int readkeyResult = ::RegOpenKeyEx(HKEY_CURRENT_USER,
(LPCWSTR)&trackpointKeys[i], 0, KEY_READ, &trackpointKey);

We don't use C style casts in our code. Please change this to a C++ style cast.
And cast to LPCTSTR instead.

Strange casing of your variables here. How about:

trackPointKey
readKeyResult

> +	   if (readkeyResult == ERROR_SUCCESS) {
> +	       // If we detected a registry key belonging to a TrackPoint
driver, then create fake trackpoint
> +	       // scrollbars, so the WebView will receive WM_VSCROLL and
WM_HSCROLL messages. We create one
> +	       // vertical scrollbar and one horizontal to allow for receiving
both types of messages.
> +	       ::CreateWindow(L"SCROLLBAR", L"FAKETRACKPOINTHSCROLLBAR",
WS_CHILD | WS_VISIBLE | SBS_HORZ, 0, 0, 0, 0, parentWindow, 0, gInstance, 0);
> +	       ::CreateWindow(L"SCROLLBAR", L"FAKETRACKPOINTVSCROLLBAR",
WS_CHILD | WS_VISIBLE | SBS_VERT, 0, 0, 0, 0, parentWindow, 0, gInstance, 0);

More L"" use in non-suffixed Windows calls. Nitpicky, since it won't really
create issues unless somebody tried to build this without UNICODE.

>  HRESULT STDMETHODCALLTYPE WebView::initWithFrame( 

I don't believe the STDMETHODCALLTYPE is really required here.

>      /* [in] */ RECT frame,
> @@ -2404,6 +2495,8 @@ HRESULT STDMETHODCALLTYPE WebView::initW
>	   frame.left, frame.top, frame.right - frame.left, frame.bottom -
frame.top, m_hostWindow ? m_hostWindow : HWND_MESSAGE, 0, gInstance, 0);
>      ASSERT(::IsWindow(m_viewWindow));
>  
> +    initializeTrackpointHackIfNecessary(m_viewWindow);
> +

We should cache this code so we don't access the registry unnecessarily every
time we create a WebView! It is unlikely this driver will get installed while
we're running.