[webkit-reviews] review granted: [Bug 28203] Mouse wheel scrolling on a page with expanded drop down list detaches drop down : [Attachment 34691] Little Cleaner Way

[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 28203: Mouse wheel scrolling on a page with expanded drop down list
detaches drop down
https://bugs.webkit.org/show_bug.cgi?id=28203

Attachment 34691: Little Cleaner Way
https://bugs.webkit.org/attachment.cgi?id=34691&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> From 2326797b9b9af58a1e28949e516be86d21e20a3a Mon Sep 17 00:00:00 2001
> From: Brian Weinstein <bweinstein at apple.com>
> Date: Wed, 12 Aug 2009 14:33:44 -0700
> Subject: [PATCH] 2009-08-12  Brian Weinstein	<bweinstein at apple.com>
> 
>	  Reviewed by NOBODY (OOPS!).
> 
>	  Fix of <rdar://6728361> Mouse wheel scrolling on a page with expanded
drop down
>	  list detaches drop down.
> 
>	  Added a check in mouseWheel to see if our focus is currently in a
popup, if so, close
>	  the popup (matches other browser behavior).
> 
>	  * WebView.cpp:
>	  (WebView::mouseWheel):
> 
> 2009-08-12  Brian Weinstein  <bweinstein at apple.com>
> 
>	  Reviewed by NOBODY (OOPS!).
> 
>	  Fix of <rdar://6728361> Mouse wheel scrolling on a page with expanded
drop down
>	  list detaches drop down.
> 
>	  Added a function for Windows PopupMenu's to return their class name.
> 
>	   * platform/PopupMenu.h:
>	   * platform/win/PopupMenuWin.cpp:
>	   (WebCore::PopupMenu::popupClassName):

Looks like your commit message got doubled.

> +    HWND focusHwnd;
> +    if ((focusHwnd = GetFocus()) && focusHwnd != m_viewWindow) {

I'd move the GetFocus() call up to the previous line.

HWND focusHwnd = GetFocus();
if (focusHwnd && focusHwnd != m_viewWindow) {

"focusedWindow" might be nicer than "focusHwnd".

> +	   // Our focus is on a different hwnd, see if it's a PopupMenu and if
so, set the focus back on us (which will hide the popup).
> +	   TCHAR className[256];
> +	   if (GetClassName(focusHwnd, className, ARRAYSIZE(className)) &&
!_tcscmp(className, PopupMenu::popupClassName())) {
> +	       SetFocus(m_viewWindow);
> +	       return true;
> +	   }
> +    }

It would be good to add an assert like ASSERT(ARRAYSIZE(className) >
_tcslen(PopupMenu::popupClassName()) so that we know the possible truncation of
the class name won't affect the comparison.

You should add a FIXME mentioning the other scrolling-related popup menu bug
and how we'd like to fix them both together.

You should add a comment about why we don't let the WebView scroll in response
to this event.

r=me