mus drag and drop: return the completed effect to the caller.

mus drag and drop: return the completed effect to the caller.

While implementing the views drag implementation, I had thought that you
didn't need to return this value, as nothing in views uses it. However,
it does get used in WebContents, so add it in.

BUG=614037
Committed: https://crrev.com/5ef4d91b5c0d124c73244b323d2d2913daae17f3
Cr-Commit-Position: refs/heads/master@{#419924}

[Elliot Glaysher, 2016-09-20 21:03:11]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 21:03:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-20 21:26:33]

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

[Elliot Glaysher, 2016-09-20 21:26:37]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 21:27:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-09-20 21:42:04]

https://codereview.chromium.org/2349973010/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2349973010/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:235: // return the tuple [success, action_taken]
to |callback|.
Document what action_take corresponds to.

https://codereview.chromium.org/2349973010/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2349973010/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1797:
client()->OnDragCompletedReturnValue(change_id, action_taken);
Can we only call OnDragCompletedReturnValue? By that I mean can you remove the
call to OnChangeCompleted on the next line?

[Elliot Glaysher, 2016-09-20 22:03:55]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 22:04:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2016-09-20 22:05:24]

erg@chromium.org changed reviewers:
+ tsepez@chromium.org

[Elliot Glaysher, 2016-09-20 22:05:25]

sky: ptal
tsepez: mojom change

https://codereview.chromium.org/2349973010/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2349973010/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:235: // return the tuple [success, action_taken]
to |callback|.
On 2016/09/20 21:42:03, sky wrote:
> Document what action_take corresponds to.

Done.

https://codereview.chromium.org/2349973010/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2349973010/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1797:
client()->OnDragCompletedReturnValue(change_id, action_taken);
On 2016/09/20 21:42:03, sky wrote:
> Can we only call OnDragCompletedReturnValue? By that I mean can you remove the
> call to OnChangeCompleted on the next line?

Removed OnChangeCompleted and made OnDragCompletedReturnValue take success. It
implicitly calls OnChangeCompleted on the other side.

[sky, 2016-09-20 22:20:26]

LGTM

https://codereview.chromium.org/2349973010/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2349973010/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:457:
OnDragCompletedReturnValue(uint32 window, bool success, uint32 effect_taken);
Please name this to match the call that initiated it. Perhaps
OnDragDropCompleted or perhaps OnPerformDragDropCompleted. I wouldn't bother
with the 'ReturnValue' part of it. That's implied by the args.

[Elliot Glaysher, 2016-09-20 22:29:57]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 22:30:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-09-20 23:08:40]

https://codereview.chromium.org/2349973010/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2349973010/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:457:
OnPerformDragDropCompleted(uint32 window, bool success, uint32 effect_taken);
Do you want an enum for effect_taken?  That way mojo could validate it for you
-- or am I misunderstanding?

[Tom Sepez, 2016-09-20 23:16:52]

LGTM otherwise.

https://codereview.chromium.org/2349973010/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2349973010/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:457:
OnPerformDragDropCompleted(uint32 window, bool success, uint32 effect_taken);
On 2016/09/20 23:08:40, Tom Sepez wrote:
> Do you want an enum for effect_taken?  That way mojo could validate it for you
> -- or am I misunderstanding?

NM. I see its a bit field?  Might more than one of these be set at a given time
in this method?  Should it be called actions_taken in that case?

[Elliot Glaysher, 2016-09-20 23:44:16]

https://codereview.chromium.org/2349973010/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2349973010/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:457:
OnPerformDragDropCompleted(uint32 window, bool success, uint32 effect_taken);
On 2016/09/20 23:16:52, Tom Sepez wrote:
> On 2016/09/20 23:08:40, Tom Sepez wrote:
> > Do you want an enum for effect_taken?  That way mojo could validate it for
you
> > -- or am I misunderstanding?
> 
> NM. I see its a bit field?  Might more than one of these be set at a given
time
> in this method?  Should it be called actions_taken in that case?

Here, it is a single value.

OnDragEnter() and OnDragOver() take a bitmask and return a bitmask.
OnCompleteDrop() takes a bitmask and returns an individual value.
OnPerformDragDropCompleted() returns this one individual value back to the
caller.

(Even if I were to make an enum for the contexts where it's an individual value,
I'd be casting back to their bitmask values immediately since those values are
fairly well entrenched in the views codebase. I added a bunch of static_asserts
in the views client library to assert that the mus and the views/aura versions
of these constants are the same.)

[Elliot Glaysher, 2016-09-20 23:44:34]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-09-20 23:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2349973010/#ps80001
(title: "Rename some effect_taken to action_taken for consistency.")

[commit-bot: I haz the power, 2016-09-20 23:45:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 00:49:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-21 00:51:12]

Description was changed from

==========
mus drag and drop: return the completed effect to the caller.

While implementing the views drag implementation, I had thought that you
didn't need to return this value, as nothing in views uses it. However,
it does get used in WebContents, so add it in.

BUG=614037
==========

to

==========
mus drag and drop: return the completed effect to the caller.

While implementing the views drag implementation, I had thought that you
didn't need to return this value, as nothing in views uses it. However,
it does get used in WebContents, so add it in.

BUG=614037

Committed: https://crrev.com/5ef4d91b5c0d124c73244b323d2d2913daae17f3
Cr-Commit-Position: refs/heads/master@{#419924}
==========

[commit-bot: I haz the power, 2016-09-21 00:51:13]

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