Remove the CHECK from the DirectManipulationHelper::Activate function as it is firing in Canary on …

Remove the CHECK from the DirectManipulationHelper::Activate function as it is firing in Canary on Windows 10.

Crash dump analysis reveals the error code to be E_UNEXPECTED. Some disassembling into the Direct Manipulation Activate
function did not reveal a code path where the above error is returned. Based on the crash dump analysis it looks like
Activate is called on a window which is visible but has 0 dimensions which could possibly be triggering this.

In any case it is also possible that calling Activate multiple times on a window may be triggering this. I was unable to repro the crash though.

Proposed fixes include :-
1. Remove the CHECK from the DirectManipulationHelper::Activate function.
2. Add a new function DirectManipulationHelper::Deactivate which deactivates Direct manipulation processing on the window.
   This is called from views and content when the window is hidden.

BUG=520492
Committed: https://crrev.com/ee20880b00c6381d4393fd9f1927459034871c63
Cr-Commit-Position: refs/heads/master@{#343526}

[ananta, 2015-08-13 23:38:57]

ananta@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-08-14 14:55:26]

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
ui/views/win/hwnd_message_handler.cc:2512: void
HWNDMessageHandler::OnWindowPosChanged(WINDOWPOS* window_pos) {
Wouldn't it be safer to do the Activate/Deactivate here? Same comment for
LegacyRenderWidgetHostHWND. Also, do the activate/deactivate calls need to be
balanced?

[ananta, 2015-08-14 18:39:02]

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
ui/views/win/hwnd_message_handler.cc:2512: void
HWNDMessageHandler::OnWindowPosChanged(WINDOWPOS* window_pos) {
On 2015/08/14 14:55:26, sky wrote:
> Wouldn't it be safer to do the Activate/Deactivate here? Same comment for
> LegacyRenderWidgetHostHWND. Also, do the activate/deactivate calls need to be
> balanced?

It shouldn't matter I think. We are calling Activate/Deactivate after the
ShowWindow call. From what I could see in the disassembly Activate bails if the
window is already active and same with Deactivate. 
So I think we are fine.

[sky, 2015-08-14 20:03:36]

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
ui/views/win/hwnd_message_handler.cc:2512: void
HWNDMessageHandler::OnWindowPosChanged(WINDOWPOS* window_pos) {
On 2015/08/14 18:39:02, ananta wrote:
> On 2015/08/14 14:55:26, sky wrote:
> > Wouldn't it be safer to do the Activate/Deactivate here? Same comment for
> > LegacyRenderWidgetHostHWND. Also, do the activate/deactivate calls need to
be
> > balanced?
> 
> It shouldn't matter I think. We are calling Activate/Deactivate after the
> ShowWindow call. From what I could see in the disassembly Activate bails if
the
> window is already active and same with Deactivate. 
> So I think we are fine.

Yes, but might the system hide/show a window behind the scenes? Say you right
click on the window icon and hide?

[ananta, 2015-08-14 20:33:47]

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1295683003/diff/1/ui/views/win/hwnd_message_h...
ui/views/win/hwnd_message_handler.cc:2512: void
HWNDMessageHandler::OnWindowPosChanged(WINDOWPOS* window_pos) {
On 2015/08/14 20:03:36, sky wrote:
> On 2015/08/14 18:39:02, ananta wrote:
> > On 2015/08/14 14:55:26, sky wrote:
> > > Wouldn't it be safer to do the Activate/Deactivate here? Same comment for
> > > LegacyRenderWidgetHostHWND. Also, do the activate/deactivate calls need to
> be
> > > balanced?
> > 
> > It shouldn't matter I think. We are calling Activate/Deactivate after the
> > ShowWindow call. From what I could see in the disassembly Activate bails if
> the
> > window is already active and same with Deactivate. 
> > So I think we are fine.
> 
> Yes, but might the system hide/show a window behind the scenes? Say you right
> click on the window icon and hide?

Fair enough. Done

[sky, 2015-08-14 20:56:37]

https://codereview.chromium.org/1295683003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/1295683003/diff/20001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:409: return 0;
Do you need the SetMsgHandled(FALSE); ?

[ananta, 2015-08-14 21:04:30]

https://codereview.chromium.org/1295683003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/1295683003/diff/20001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:409: return 0;
On 2015/08/14 20:56:37, sky wrote:
> Do you need the SetMsgHandled(FALSE); ?

Done.

[sky, 2015-08-14 21:11:29]

LGTM

[ananta, 2015-08-14 21:53:40]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-08-14 21:54:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1295683003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1295683003/40001

[commit-bot: I haz the power, 2015-08-14 23:35:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-14 23:36:24]

Patchset 3 (id:??) landed as
https://crrev.com/ee20880b00c6381d4393fd9f1927459034871c63
Cr-Commit-Position: refs/heads/master@{#343526}