Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11098077:
Grab mouse capture in the WM_POINTERDOWN message handler in the omnibox. (Closed)
|
Created:
8 years, 2 months ago by ananta Modified:
8 years, 2 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionGrab mouse capture in the WM_TOUCH message handler in the omnibox.
This message is received in Windows 8 mode when we touch the window/click on it. It looks there is a bug
in Windows 8 where in after touching the window, the generated WM_LBUTTONDOWN messages goes to the window which had focus
previously. For e.g. if the focus was previously with the RenderWidgetHost window then it would get this message. The
handling for this message sets focus back to the RenderWidgetHost window, which causes the OSK to show up and disappear
when we touch the omnixbox.
Fix is to register the omnibox window for touch and grab capture when we receive a touch down and release it on touch up.
This ensures that subsequent mouse messages are sent to the omnibox and focus remains in the omnibox.
I also removed the WM_POINTERDOWN/WM_POINTERUP handlers as they are no longer functional given that we don't support
automatically popping up the OSK in desktop mode.
BUG=155155
R=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=161893
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 6
Patch Set 12 : #
Total comments: 4
Patch Set 13 : #Patch Set 14 : #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/11098077/diff/1/chrome/browser/ui/views/omnib... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/1/chrome/browser/ui/views/omnib... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1454: SetCapture(); Do you need a corresponding ReleaseCapture somewhere now?
Please add comments about this.
On 2012/10/11 19:29:25, Peter Kasting wrote: > Please add comments about this. Done.
https://codereview.chromium.org/11098077/diff/1/chrome/browser/ui/views/omnib... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/1/chrome/browser/ui/views/omnib... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1454: SetCapture(); On 2012/10/11 19:24:25, sky wrote: > Do you need a corresponding ReleaseCapture somewhere now? Added a ReleaseCapture in OnPointerUp.
https://codereview.chromium.org/11098077/diff/6001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/6001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1473: if (m_hWnd == GetCapture()) Is this invoked as you release each finger? If so, do we only want to do this if there are no touch devices down?
https://codereview.chromium.org/11098077/diff/6001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/6001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1473: if (m_hWnd == GetCapture()) On 2012/10/11 21:28:12, sky wrote: > Is this invoked as you release each finger? If so, do we only want to do this if > there are no touch devices down? The only way to get info about touch points is to register the window for touch. That might bring about changes in behavior due to differences in some messages. I added a check to only set capture if the first button is down and to release it when it is up.
In principle should this window be registered for touch? If so, we should do that. What I am really saying here is I never want us to do some fix that is less theoretically correct just because it touches less code or is easier to test.
On 2012/10/11 22:01:23, Peter Kasting wrote: > In principle should this window be registered for touch? If so, we should do > that. > > What I am really saying here is I never want us to do some fix that is less > theoretically correct just because it touches less code or is easier to test. Fair enough. I registered the omnibox for touch events and added code to set and release capture in the touch handler. Did a bit of testing which seems to suggest that the omni box continues to work well with this change.
https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1376: bool touch_mode = RegisterTouchWindow(m_hWnd, TWF_WANTPALM) == TRUE; Nit: Just auto-convert to bool, don't explicitly check "== TRUE" https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1462: if (wparam == 1) { Shouldn't this also check "&& !model()->has_focus()"? https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: // capture here to ensure that the generated mouse messages come to us. Nit: If you move this comment to be at the beginning of the function, you can add something like: "...previously had focus. This means that if a user taps the omnibox to give it focus, we don't get the simulated WM_LBUTTONDOWN, and thus don't properly select all the text. To ensure we get this message, we capture the mouse when the user is doing a single-point tap on an unfocused model." https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1495: SetMsgHandled(false); Nit: If this is all this function does, can't we just remove the WM_POINTERUP macro from the header or something?
https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1376: bool touch_mode = RegisterTouchWindow(m_hWnd, TWF_WANTPALM) == TRUE; On 2012/10/11 23:13:34, Peter Kasting wrote: > Nit: Just auto-convert to bool, don't explicitly check "== TRUE" Replaced with !! as auto conversion gets a compiler warning. https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1462: if (wparam == 1) { On 2012/10/11 23:13:34, Peter Kasting wrote: > Shouldn't this also check "&& !model()->has_focus()"? We can't do that as the WM_POINTERDOWN handler which sets focus is invoked before the touch handler. https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: // capture here to ensure that the generated mouse messages come to us. On 2012/10/11 23:13:34, Peter Kasting wrote: > Nit: If you move this comment to be at the beginning of the function, you can > add something like: > > "...previously had focus. This means that if a user taps the omnibox to give it > focus, we don't get the simulated WM_LBUTTONDOWN, and thus don't properly select > all the text. To ensure we get this message, we capture the mouse when the user > is doing a single-point tap on an unfocused model." Done. https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1495: SetMsgHandled(false); On 2012/10/11 23:13:34, Peter Kasting wrote: > Nit: If this is all this function does, can't we just remove the WM_POINTERUP > macro from the header or something? Done.
https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1462: if (wparam == 1) { On 2012/10/11 23:48:41, ananta wrote: > On 2012/10/11 23:13:34, Peter Kasting wrote: > > Shouldn't this also check "&& !model()->has_focus()"? > > We can't do that as the WM_POINTERDOWN handler which sets focus is invoked > before the touch handler. I moved the SetFocus call from OnPointerDown to the TOUCHEVENTF_DOWN if below.
LGTM https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1376: bool touch_mode = RegisterTouchWindow(m_hWnd, TWF_WANTPALM) == TRUE; On 2012/10/11 23:48:41, ananta wrote: > On 2012/10/11 23:13:34, Peter Kasting wrote: > > Nit: Just auto-convert to bool, don't explicitly check "== TRUE" > > Replaced with !! as auto conversion gets a compiler warning. Suck. In that case, maybe it'd be even better to declare touch_mode as BOOL -- the DCHECK() will still work. https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1466: // that we get this message, we capture the mouse when the user is doing a Nit: Extra space https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: if (wparam == 1 && !model()->has_focus()) { Nit: Rest of this code puts parens around all binary subexpressions
On 2012/10/12 00:19:59, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... > File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): > > https://codereview.chromium.org/11098077/diff/12001/chrome/browser/ui/views/o... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1376: bool touch_mode = > RegisterTouchWindow(m_hWnd, TWF_WANTPALM) == TRUE; > On 2012/10/11 23:48:41, ananta wrote: > > On 2012/10/11 23:13:34, Peter Kasting wrote: > > > Nit: Just auto-convert to bool, don't explicitly check "== TRUE" > > > > Replaced with !! as auto conversion gets a compiler warning. > > Suck. In that case, maybe it'd be even better to declare touch_mode as BOOL -- > the DCHECK() will still work. > > https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... > File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): > > https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1466: // that we get this > message, we capture the mouse when the user is doing a > Nit: Extra space > > https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: if (wparam == 1 && > !model()->has_focus()) { > Nit: Rest of this code puts parens around all binary subexpressions Sadly I had to move the model()->has_focus() call back to OnPointerDown. On Windows 8 metro mode this handler is invoked for regular mouse messages as well. For e.g. left button click results in WM_POINTERDOWN/WM_LBUTTONDOWN/WM_LBUTTONUP/WM_POINTERUP etc. Caused some DCHECKS to fire in the omnibox code related to focus when I used the mouse to select the omnibox. PTAL. Thanks Ananta
The critical bit is not whether WM_POINTERDOWN is fired for mouse clicks but whether Windows automatically does WM_SETFOCUS the way it does for non-metro left clicks on the control. (Or at least I assume that's why we has a SetFocus() call in OnPointerDown() to begin with.) Are you saying that in metro mode, mouse clicks don't cause WM_SETFOCUS to happen automatically?
https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1466: // that we get this message, we capture the mouse when the user is doing a On 2012/10/12 00:19:59, Peter Kasting wrote: > Nit: Extra space Done. https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: if (wparam == 1 && !model()->has_focus()) { On 2012/10/12 00:19:59, Peter Kasting wrote: > Nit: Rest of this code puts parens around all binary subexpressions Do you mean to parenthesize point.dwFlags & TOUCHEVENTF_DOWN for e.g?
On 2012/10/12 00:37:18, ananta wrote: > https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: if (wparam == 1 && > !model()->has_focus()) { > On 2012/10/12 00:19:59, Peter Kasting wrote: > > Nit: Rest of this code puts parens around all binary subexpressions > Do you mean to parenthesize point.dwFlags & TOUCHEVENTF_DOWN > for e.g? No, because that's the only expression in the conditional and thus already has parens around it. I meant for the (wparam == 1) subexpression when it has "&& ..." next to it.
On 2012/10/12 00:40:06, Peter Kasting wrote: > On 2012/10/12 00:37:18, ananta wrote: > > > https://codereview.chromium.org/11098077/diff/18002/chrome/browser/ui/views/o... > > chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1468: if (wparam == 1 && > > !model()->has_focus()) { > > On 2012/10/12 00:19:59, Peter Kasting wrote: > > > Nit: Rest of this code puts parens around all binary subexpressions > > Do you mean to parenthesize point.dwFlags & TOUCHEVENTF_DOWN > > for e.g? > > No, because that's the only expression in the conditional and thus already has > parens around it. I meant for the (wparam == 1) subexpression when it has "&& > ..." next to it. Done. I removed the WM_POINTERDOWN handler as well as it is no longer functional. PTAL
https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1404: UnregisterTouchWindow(m_hWnd); Do we actually need to do this? It seems from the MSDN docs like this is only necessary if we want to mark the window as non-touch-friendly, it's not something where you're required to balance your calls. https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1473: SetFocus(); Is this SetFocus() necessary? I thought you said we don't need this. https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1699: if (!base::win::IsMetroProcess()) Seems like it'd be better to do: if (gaining_focus_) { DCHECK(base::win::IsMetroProcess()); return result; }
https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1404: UnregisterTouchWindow(m_hWnd); On 2012/10/12 21:16:31, Peter Kasting wrote: > Do we actually need to do this? It seems from the MSDN docs like this is only > necessary if we want to mark the window as non-touch-friendly, it's not > something where you're required to balance your calls. Removed this and the OnDestroy handler. https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1473: SetFocus(); On 2012/10/12 21:16:31, Peter Kasting wrote: > Is this SetFocus() necessary? I thought you said we don't need this. Good point. Artifact of the previous iteration. Will test it out before landing. https://codereview.chromium.org/11098077/diff/22004/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1699: if (!base::win::IsMetroProcess()) On 2012/10/12 21:16:31, Peter Kasting wrote: > Seems like it'd be better to do: > > if (gaining_focus_) { > DCHECK(base::win::IsMetroProcess()); > return result; > } Done.
LGTM https://codereview.chromium.org/11098077/diff/27002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/27002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1374: Nit: Either delete this or add a similar blank line below this next block. https://codereview.chromium.org/11098077/diff/27002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1696: if (!base::win::IsMetroProcess()) This conditional should go away entirely, the DCHECK in it is guaranteed to succeed.
https://codereview.chromium.org/11098077/diff/27002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11098077/diff/27002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1374: On 2012/10/12 21:36:57, Peter Kasting wrote: > Nit: Either delete this or add a similar blank line below this next block. Done. https://codereview.chromium.org/11098077/diff/27002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1696: if (!base::win::IsMetroProcess()) On 2012/10/12 21:36:57, Peter Kasting wrote: > This conditional should go away entirely, the DCHECK in it is guaranteed to > succeed. Yes. It was taken out in the patch I uploaded after this one.
As Peter is reviewing this one I'm removing myself as a reviewer. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
