Enable async mode for DownloadURL drags.

Enable async mode for DownloadURL drags.

BUG=390991
Committed: https://crrev.com/801c26a4d72811d0b59b2123cb8284db5f4a72bf
Cr-Commit-Position: refs/heads/master@{#324147}

[dcheng, 2015-04-06 21:11:25]

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

[dcheng, 2015-04-06 21:11:25]

https://msdn.microsoft.com/en-us/library/windows/desktop/bb776904(v=vs.85).as...
covers the details of using this interface.

Specifically, the async transfer mode isn't enabled if
IDataObjectAsyncCapability::GetAsyncMode() doesn't return VARIANT_TRUE. We lost
the call to enable async mode in the switch to Aura, so this patch restores it.

The original code in web_contents_view_win.cc (removed in
https://codereview.chromium.org/137403005) also does some crazy things with
handling the drag and drop on a background thread and forwarding messages to it.
The MSDN article implies that this isn't necessary (as it implies that the main
DnD operation is still taking place on the primary thread), and some testing
seems to confirm this (I wasn't able to deadlock Chrome).

[dcheng, 2015-04-06 21:12:26]

(Minor correction, the original code was actually in web_contents_drag_win.cc)

[dcheng, 2015-04-07 17:30:13]

dcheng@chromium.org changed reviewers:
+ sadrul@chromium.org

[dcheng, 2015-04-07 17:30:14]

+sadrul, mind taking a look at this so we can get this landed sooner? Maybe
merged back to previous releases too since it's pretty busted without it
(deadlocks are pretty easy to trigger without this patch).

[sadrul, 2015-04-07 17:39:14]

sadrul@chromium.org changed reviewers:
+ ananta@chromium.org, scottmg@chromium.org

[sadrul, 2015-04-07 17:39:14]

+ananta@ scottmg@

The change makes sense, but I don't know Windows well enough to review this.
Added ananta@ and scottmg@

[scottmg, 2015-04-07 19:38:05]

https://codereview.chromium.org/1062803003/diff/1/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/1/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data_provider_win.cc:536:
data_->SetAsyncMode(TRUE);
it looks like it should be VARIANT_TRUE from
https://msdn.microsoft.com/en-us/library/windows/desktop/bb776311(v=vs.85).aspx

i don't otherwise have any insight. i would just try dragging 2 files one after
the other, so if you tried that, then lgtm.

[dcheng, 2015-04-07 21:04:00]

PTAL

https://codereview.chromium.org/1062803003/diff/1/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/1/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data_provider_win.cc:536:
data_->SetAsyncMode(TRUE);
On 2015/04/07 19:38:05, scottmg wrote:
> it looks like it should be VARIANT_TRUE from
>
https://msdn.microsoft.com/en-us/library/windows/desktop/bb776311(v=vs.85).aspx
> 
> i don't otherwise have any insight. i would just try dragging 2 files one
after
> the other, so if you tried that, then lgtm.

Fixed this and several other instances.

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
ui/base/dragdrop/os_exchange_data_provider_win.cc:836: in_async_mode_ =
(do_op_async != VARIANT_FALSE);
Even though this is a bit harder to read, I think it's a bit more robust against
people passing in 'incorrect' values of true?

[sadrul, 2015-04-07 21:06:03]

rs lgtm

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
ui/base/dragdrop/os_exchange_data_provider_win.cc:836: in_async_mode_ =
(do_op_async != VARIANT_FALSE);
On 2015/04/07 21:04:00, dcheng wrote:
> Even though this is a bit harder to read, I think it's a bit more robust
against
> people passing in 'incorrect' values of true?

Maybe check for both FALSE and VARIANT_FALSE?

[dcheng, 2015-04-07 21:08:49]

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
ui/base/dragdrop/os_exchange_data_provider_win.cc:836: in_async_mode_ =
(do_op_async != VARIANT_FALSE);
On 2015/04/07 21:06:03, sadrul wrote:
> On 2015/04/07 21:04:00, dcheng wrote:
> > Even though this is a bit harder to read, I think it's a bit more robust
> against
> > people passing in 'incorrect' values of true?
> 
> Maybe check for both FALSE and VARIANT_FALSE?

My understanding is VARIANT_FALSE / FALSE / false have the same numerical value,
so this should be OK.

Alternatively, I could just do !!do_op_async. Do we have a pattern for this in
Chrome?

[scottmg, 2015-04-07 21:27:15]

lgtm

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
ui/base/dragdrop/os_exchange_data_provider_win.cc:836: in_async_mode_ =
(do_op_async != VARIANT_FALSE);
On 2015/04/07 21:08:49, dcheng wrote:
> On 2015/04/07 21:06:03, sadrul wrote:
> > On 2015/04/07 21:04:00, dcheng wrote:
> > > Even though this is a bit harder to read, I think it's a bit more robust
> > against
> > > people passing in 'incorrect' values of true?
> > 
> > Maybe check for both FALSE and VARIANT_FALSE?
> 
> My understanding is VARIANT_FALSE / FALSE / false have the same numerical
value,
> so this should be OK.
> 
> Alternatively, I could just do !!do_op_async. Do we have a pattern for this in
> Chrome?

!! is relatively common to convert from BOOL to bool. I think it's OK to rely on
the value of VARIANT_FALSE==FALSE==false==0. (I only mentioned it because
VARIANT_TRUE is -1, not 1, so conceivably something might whine)

[dcheng, 2015-04-07 21:31:00]

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
File ui/base/dragdrop/os_exchange_data_provider_win.cc (right):

https://codereview.chromium.org/1062803003/diff/20001/ui/base/dragdrop/os_exc...
ui/base/dragdrop/os_exchange_data_provider_win.cc:836: in_async_mode_ =
(do_op_async != VARIANT_FALSE);
On 2015/04/07 21:27:15, scottmg wrote:
> On 2015/04/07 21:08:49, dcheng wrote:
> > On 2015/04/07 21:06:03, sadrul wrote:
> > > On 2015/04/07 21:04:00, dcheng wrote:
> > > > Even though this is a bit harder to read, I think it's a bit more robust
> > > against
> > > > people passing in 'incorrect' values of true?
> > > 
> > > Maybe check for both FALSE and VARIANT_FALSE?
> > 
> > My understanding is VARIANT_FALSE / FALSE / false have the same numerical
> value,
> > so this should be OK.
> > 
> > Alternatively, I could just do !!do_op_async. Do we have a pattern for this
in
> > Chrome?
> 
> !! is relatively common to convert from BOOL to bool. I think it's OK to rely
on
> the value of VARIANT_FALSE==FALSE==false==0. (I only mentioned it because
> VARIANT_TRUE is -1, not 1, so conceivably something might whine)

I'll just use !! since I find it a bit easier to read personally.

[dcheng, 2015-04-07 22:06:08]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-04-07 22:06:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1062803003/#ps40001
(title: "!!")

[commit-bot: I haz the power, 2015-04-07 22:06:18]

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

[commit-bot: I haz the power, 2015-04-07 23:04:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-07 23:05:35]

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