Route OnDragEvent through ViewAndroid tree

Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

Tested manually with target sdk set to 25.

BUG=622847
Review-Url: https://codereview.chromium.org/2896993002
Cr-Commit-Position: refs/heads/master@{#474947}
Committed: https://chromium.googlesource.com/chromium/src/+/da11b4dc274864786ae57bb4c006b9adfdbefb4e

[Jinsuk Kim, 2017-05-24 05:28:11]

Description was changed from

==========
OnDragEvent through ViewAndroid

BUG=622847
==========

to

==========
Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

BUG=622847
==========

[Jinsuk Kim, 2017-05-24 06:57:12]

Patchset #5 (id:80001) has been deleted

[Jinsuk Kim, 2017-05-24 06:57:31]

Patchset #4 (id:60001) has been deleted

[Jinsuk Kim, 2017-05-24 06:57:45]

Patchset #3 (id:40001) has been deleted

[Jinsuk Kim, 2017-05-24 06:57:52]

Patchset #2 (id:20001) has been deleted

[Jinsuk Kim, 2017-05-24 09:06:36]

jinsukkim@chromium.org changed reviewers:
+ aelias@chromium.org, boliu@chromium.org

[Jinsuk Kim, 2017-05-24 09:06:37]

boliu@chromium.org: Please review changes in content/ ui/android

aelias@chromium.org: Please review changes in ui/event/

[aelias_OOO_until_Jul13, 2017-05-24 22:25:41]

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/drag...
File ui/events/android/drag_event_android.cc (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/drag...
ui/events/android/drag_event_android.cc:31: new_location + (root_location_f() -
location_f());
Terminology and calculation seems weird here.  "screen" position typically
refers to location from the (0,0) coordinate of the physical screen (think
especially of floating window UIs).  root_location_f doesn't seem to refer to
that root, based on the comment for ui/events/event.h: "|location_| multiplied
by an optional transformation matrix for rotations, animations and skews.".  And
this isn't a pattern I see elsewhere.  So maybe just set this to be equal to
new_location?

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/moti...
File ui/events/android/motion_event_android.h (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/moti...
ui/events/android/motion_event_android.h:73: std::unique_ptr<MotionEventAndroid>
CreateFor(const gfx::Point& point) const;
gfx::PointF

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/moti...
ui/events/android/motion_event_android.h:76: gfx::Point GetPoint() const {
return gfx::Point(GetX(0), GetY(0)); }
gfx::PointF

[boliu, 2017-05-24 22:54:02]

I didn't find any existing drag-drop tests. Did you make sure to manually test
this doesn't break drag-drop?

https://codereview.chromium.org/2896993002/diff/100001/ui/android/event_forwa...
File ui/android/event_forwarder.cc (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/event_forwa...
ui/android/event_forwarder.cc:156: base::string16 drop_content =
ConvertJavaStringToUTF16(env, j_content);
depending on how big the payload is, that's potentially a *lot* of unnecessary
copying. just take a global ref to the java object, and convert lazily?

https://codereview.chromium.org/2896993002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/EventForwarder.java (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/EventForwarder.java:26: private final
boolean mIsDragDropEnabled;
nit: final above non-final member variables

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:106: bool ViewAndroid::HitBy(const gfx::Point& point)
{
just inline this one line method with a single caller. "HitBy" is a pretty
confusing name

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:366: bool ViewAndroid::HitTest(T send_to_client,
I think T doesn't need to be a template, just declare the correct base::Callback
type, using E if needed

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.h:75: bool HitBy(const gfx::Point& point) {
this should not be part of the public interface

probably should un-inline everything in this struct in a follow up

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.h:162: bool HitBy(const gfx::Point& point);
ditto does this need to be public?

[Jinsuk Kim, 2017-05-25 03:17:09]

Description was changed from

==========
Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

BUG=622847
==========

to

==========
Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

Tested manually with target sdk set to 25.

BUG=622847
==========

[Jinsuk Kim, 2017-05-25 03:18:19]

> I didn't find any existing drag-drop tests. Did you make
> sure to manually test
> this doesn't break drag-drop?

I made sure it works. Updated the CL description.

https://codereview.chromium.org/2896993002/diff/100001/ui/android/event_forwa...
File ui/android/event_forwarder.cc (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/event_forwa...
ui/android/event_forwarder.cc:156: base::string16 drop_content =
ConvertJavaStringToUTF16(env, j_content);
On 2017/05/24 22:54:02, boliu wrote:
> depending on how big the payload is, that's potentially a *lot* of unnecessary
> copying. just take a global ref to the java object, and convert lazily?

Done.

https://codereview.chromium.org/2896993002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/EventForwarder.java (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/EventForwarder.java:26: private final
boolean mIsDragDropEnabled;
On 2017/05/24 22:54:02, boliu wrote:
> nit: final above non-final member variables

Done.

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:106: bool ViewAndroid::HitBy(const gfx::Point& point)
{
On 2017/05/24 22:54:02, boliu wrote:
> just inline this one line method with a single caller. "HitBy" is a pretty
> confusing name

Done.

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:366: bool ViewAndroid::HitTest(T send_to_client,
On 2017/05/24 22:54:02, boliu wrote:
> I think T doesn't need to be a template, just declare the correct
base::Callback
> type, using E if needed

Done.

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.h:75: bool HitBy(const gfx::Point& point) {
On 2017/05/24 22:54:02, boliu wrote:
> this should not be part of the public interface
> 
> probably should un-inline everything in this struct in a follow up

Reverted (inlined it back).

https://codereview.chromium.org/2896993002/diff/100001/ui/android/view_androi...
ui/android/view_android.h:162: bool HitBy(const gfx::Point& point);
On 2017/05/24 22:54:02, boliu wrote:
> ditto does this need to be public?

Mistake. changed to private.

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/drag...
File ui/events/android/drag_event_android.cc (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/drag...
ui/events/android/drag_event_android.cc:31: new_location + (root_location_f() -
location_f());
On 2017/05/24 22:25:40, aelias wrote:
> Terminology and calculation seems weird here.  "screen" position typically
> refers to location from the (0,0) coordinate of the physical screen (think
> especially of floating window UIs).  root_location_f doesn't seem to refer to
> that root, based on the comment for ui/events/event.h: "|location_| multiplied
> by an optional transformation matrix for rotations, animations and skews.". 
And
> this isn't a pattern I see elsewhere.  So maybe just set this to be equal to
> new_location?

Agreed that storing screen location to root location is misleading. Let me store
the position directly in the class since the parent class doesn't provide a
place for it. Then the parent class is useful for |location_| only, not enough
reason to inherit it. Removed the inheritance.

|screen_location_| still needs adjusting to have it point to the new location
relative to the screen coordinate. I'm still doing the conversion.

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/moti...
File ui/events/android/motion_event_android.h (right):

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/moti...
ui/events/android/motion_event_android.h:73: std::unique_ptr<MotionEventAndroid>
CreateFor(const gfx::Point& point) const;
On 2017/05/24 22:25:41, aelias wrote:
> gfx::PointF

Done.

https://codereview.chromium.org/2896993002/diff/100001/ui/events/android/moti...
ui/events/android/motion_event_android.h:76: gfx::Point GetPoint() const {
return gfx::Point(GetX(0), GetY(0)); }
On 2017/05/24 22:25:41, aelias wrote:
> gfx::PointF

Done.

[boliu, 2017-05-25 03:49:34]

lgtm % comments

https://codereview.chromium.org/2896993002/diff/120001/ui/events/android/drag...
File ui/events/android/drag_event_android.h (right):

https://codereview.chromium.org/2896993002/diff/120001/ui/events/android/drag...
ui/events/android/drag_event_android.h:36: const gfx::Point location() const {
return gfx::ToFlooredPoint(location_); }
only direct pod type accessors should be inlined. this and screen_location don't
count

https://codereview.chromium.org/2896993002/diff/120001/ui/events/android/drag...
ui/events/android/drag_event_android.h:58: // The Java reference to the drop
content.
say this is holding a ref to avoid unnecessary copying

[aelias_OOO_until_Jul13, 2017-05-25 18:16:43]

lgtm

[Jinsuk Kim, 2017-05-25 23:12:14]

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

[Jinsuk Kim, 2017-05-25 23:12:15]

sky@chromium.org: Please review changes in ui/android/DEPS

** Presubmit ERRORS **
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
    '+ui/base/ui_base_switches_util.h',

Suggested missing target path OWNERS:
    sky@chromium.org

https://codereview.chromium.org/2896993002/diff/120001/ui/events/android/drag...
File ui/events/android/drag_event_android.h (right):

https://codereview.chromium.org/2896993002/diff/120001/ui/events/android/drag...
ui/events/android/drag_event_android.h:36: const gfx::Point location() const {
return gfx::ToFlooredPoint(location_); }
On 2017/05/25 03:49:34, boliu wrote:
> only direct pod type accessors should be inlined. this and screen_location
don't
> count

Done.

https://codereview.chromium.org/2896993002/diff/120001/ui/events/android/drag...
ui/events/android/drag_event_android.h:58: // The Java reference to the drop
content.
On 2017/05/25 03:49:34, boliu wrote:
> say this is holding a ref to avoid unnecessary copying

Done.

[sky, 2017-05-25 23:57:59]

https://codereview.chromium.org/2896993002/diff/140001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/2896993002/diff/140001/ui/android/DEPS#newcode16
ui/android/DEPS:16: "+ui/base/ui_base_switches_util.h",
Why not +ui/base? This target depends upon ui/base, so it seems silly to also
have deps like this. Maybe I'm missing why that is?

[Jinsuk Kim, 2017-05-26 00:16:48]

https://codereview.chromium.org/2896993002/diff/140001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/2896993002/diff/140001/ui/android/DEPS#newcode16
ui/android/DEPS:16: "+ui/base/ui_base_switches_util.h",
On 2017/05/25 23:57:59, sky wrote:
> Why not +ui/base? This target depends upon ui/base, so it seems silly to also
> have deps like this. Maybe I'm missing why that is?

Honestly no idea. I was just following what has been done - adding one header
file at a time. Replaced all ui/base/*.h with +ui/base. Same for ui/events/*.h
which this target also depend on.

[sky, 2017-05-26 03:01:34]

LGTM

[Jinsuk Kim, 2017-05-26 05:14:31]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-05-26 05:14:32]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2896993002/#ps160001
(title: "+ui/base")

[commit-bot: I haz the power, 2017-05-26 05:15:33]

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, 2017-05-26 06:44:47]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495775671935590,
"parent_rev": "69b92a95fb72a2d887e7863ae3e038455d45a00f", "commit_rev":
"da11b4dc274864786ae57bb4c006b9adfdbefb4e"}

[commit-bot: I haz the power, 2017-05-26 06:45:03]

Description was changed from

==========
Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

Tested manually with target sdk set to 25.

BUG=622847
==========

to

==========
Route OnDragEvent through ViewAndroid tree

Moves another event handling flow from CVC to ViewAndroid:

- Introduced a new native class for drag event (DragEventAndroid)
- Updated ViewAndroid to handle both drag/motion events
  in its hit testing logic.
- Clarify that layout in VA is in CSS unit, which was not
  clear and prone to error.

Tested manually with target sdk set to 25.

BUG=622847

Review-Url: https://codereview.chromium.org/2896993002
Cr-Commit-Position: refs/heads/master@{#474947}
Committed:
https://chromium.googlesource.com/chromium/src/+/da11b4dc274864786ae57bb4c006...
==========

[commit-bot: I haz the power, 2017-05-26 06:45:04]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/da11b4dc274864786ae57bb4c006...