Add Aura handles to be used in unified touch selection

Add Aura handles to be used in unified touch selection

This patch is part of Aura side of unified touch selection. It adds an
implementation of touch handle drawables to be used on Aura. It also
adds some functions to the public interface of
ui::TouchSelectionController needed for unified touch selection.

COLLABORATOR=mfomitchev
BUG=399721
Committed: https://crrev.com/9143361454a599ac0882abcbd846b25ad7ce7945
Cr-Commit-Position: refs/heads/master@{#329191}

[mohsen, 2015-03-12 15:34:45]

mohsen@chromium.org changed reviewers:
+ jdduke@chromium.org, mfomitchev@chromium.org, sadrul@chromium.org

[mohsen, 2015-03-12 15:34:46]

This patch contains ui/touch_selection part of unified touch selection patch.
Please take a look...

[jdduke (slow), 2015-03-12 15:59:06]

https://codereview.chromium.org/996373002/diff/20001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/996373002/diff/20001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:120:
controller_->OnSelectionBoundsUpdated(SelectionBound(),
Should we get some test coverage for the return value? Either in some existing
tests (one or two is probably fine, just basic coverage), or a new basic test
that just verifies we get true/false when expected.

[mohsen, 2015-03-12 18:55:27]

https://codereview.chromium.org/996373002/diff/20001/ui/touch_selection/touch...
File ui/touch_selection/touch_selection_controller_unittest.cc (right):

https://codereview.chromium.org/996373002/diff/20001/ui/touch_selection/touch...
ui/touch_selection/touch_selection_controller_unittest.cc:120:
controller_->OnSelectionBoundsUpdated(SelectionBound(),
On 2015/03/12 15:59:06, jdduke wrote:
> Should we get some test coverage for the return value? Either in some existing
> tests (one or two is probably fine, just basic coverage), or a new basic test
> that just verifies we get true/false when expected.

Yeah. I added checks for the return value on all calls.

[mohsen, 2015-03-12 20:14:23]

Patchset #5 (id:80001) has been deleted

[mohsen, 2015-03-12 20:41:57]

Patchset #5 (id:100001) has been deleted

[jdduke (slow), 2015-03-13 15:41:27]

https://codereview.chromium.org/996373002/diff/120001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/996373002/diff/120001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:302:
&TouchSelectionControllerAura::ShowQuickMenu);
Why the delay here? What about fading it in or something like that?

If this is called at all frequently, consider using the Timer constructor that
takes all of these args. Then you need only call |Stop| and |Reset|, and you
avoid creating a new closure every time.

[mohsen, 2015-03-13 17:35:34]

https://codereview.chromium.org/996373002/diff/120001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller_aura.cc (right):

https://codereview.chromium.org/996373002/diff/120001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller_aura.cc:302:
&TouchSelectionControllerAura::ShowQuickMenu);
On 2015/03/13 15:41:27, jdduke wrote:
> Why the delay here? What about fading it in or something like that?
I don't know exactly what was the original intention for this delay. But, I
think I have seen places that the quick menu is shown but dismissed immediately
for some reason. Without this delay, menu would flicker for a moment. Another
case is when a window is resized, in which case, because of size change, quick
menu is hidden for a moment and reshown in the new place. without this delay,
menu would flicker while the user is resizing the window; this delay keeps it
hidden until the end of resize (or until the user pauses the resize for more
than 100ms). There is no easy resize-started and resize-ended events that we can
use to hide and then show the menu (sadrul@, please correct me if I'm wrong).

> If this is called at all frequently, consider using the Timer constructor that
> takes all of these args. Then you need only call |Stop| and |Reset|, and you
> avoid creating a new closure every time.
Done.

[mohsen, 2015-03-18 14:31:13]

jdduke@: Now that TouchSelectionController is changed to be a base class for
TouchSelectionControllerAndroid and TouchSelectionControllerAura, can you take
another look at this CL?

sadrul@: Please take a look and let me know of what you think about the new
design for TSC/TSCAura/TSCAndroid.

[jdduke (slow), 2015-03-23 15:20:55]

On 2015/03/18 14:31:13, mohsen wrote:
> jdduke@: Now that TouchSelectionController is changed to be a base class for
> TouchSelectionControllerAndroid and TouchSelectionControllerAura, can you take
> another look at this CL?

At first glance, I can't say I love the base class design here. It makes
reasoning about behavior more difficult than it was previously. Two alternatives
here:

1) Rename the current TouchSelectionController/Client to
TouchSelectionControllerImpl/Client. Then create a 
TouchSelectionControllerAura/TouchSelectionControllerAndroid that contain an
instance of that class, delegating to it as appropriate and implements its
client. Aura can then have a TouchSelectionControllerAuraClient, while Android
can just use ContentViewCore directly for delegation.

2) Per Mikhail's suggestion, create a pure TouchSelectionController interface
that is implemented by
TouchSelectionControllerImpl+TouchSelectionControllerAura+TouchSelectionControllerAura.
Both Aura/Android are again implemented in terms of
TouchSelectionControllerImpl.

I slightly prefer (1) as we really don't need polymorphism of the interface for
the Aura/Android implementations, and then we can vary the platform-specific
clients perhaps a bit more easily, but beyond that I'm fairly indifferent. If
others disagree, we can proceed with this version (also noting that these two
proposed approaches probably incur some additional boilerplate not found in the
latest design).

[chromium-reviews, 2015-03-23 15:42:07]

Jared, so the only difference between (1) and (2) is the presence of
TouchSelectionController
interface in (2)?

On Mon, Mar 23, 2015 at 11:20 AM <jdduke@chromium.org> wrote:

> On 2015/03/18 14:31:13, mohsen wrote:
> > jdduke@: Now that TouchSelectionController is changed to be a base class
> > for
> > TouchSelectionControllerAndroid and TouchSelectionControllerAura, can
> you
> > take
> > another look at this CL?
>
> At first glance, I can't say I love the base class design here. It makes
> reasoning about behavior more difficult than it was previously. Two
> alternatives
> here:
>
> 1) Rename the current TouchSelectionController/Client to
> TouchSelectionControllerImpl/Client. Then create a
> TouchSelectionControllerAura/TouchSelectionControllerAndroid that contain
> an
> instance of that class, delegating to it as appropriate and implements its
> client. Aura can then have a TouchSelectionControllerAuraClient, while
> Android
> can just use ContentViewCore directly for delegation.
>
> 2) Per Mikhail's suggestion, create a pure TouchSelectionController
> interface
> that is implemented by
> TouchSelectionControllerImpl+TouchSelectionControllerAura+
> TouchSelectionControllerAura.
> Both Aura/Android are again implemented in terms of
> TouchSelectionControllerImpl.
>
> I slightly prefer (1) as we really don't need polymorphism of the interface
> for
> the Aura/Android implementations, and then we can vary the
> platform-specific
> clients perhaps a bit more easily, but beyond that I'm fairly indifferent.
> If
> others disagree, we can proceed with this version (also noting that these
> two
> proposed approaches probably incur some additional boilerplate not found in
> the
> latest design).
>
> https://codereview.chromium.org/996373002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jdduke (slow), 2015-03-23 15:45:31]

On 2015/03/23 15:42:07, chromium-reviews wrote:
> Jared, so the only difference between (1) and (2) is the presence of
> TouchSelectionController
> interface in (2)?

Basically. There might be some client differences, i.e., if
TouchSelectionControllerAndroid is a TouchSelectionController, but doesn't take
a TouchSelectionControllerClient upon creation, it's a little misleading (but
not the end of the world).

[mfomitchev, 2015-03-23 22:10:03]

I had a quick chat with Sadrul and here's my understanding of his requirements
to the new design:
From a high level, he wants the design to be such that if one understands how
touch selection is used on one platform (e.g. Android), it should be very easy
for her to understand how it is used on the other platform (e.g. Aura), which
means that we should use the same abstractions, and the parts that are similar
should be done via the same shared abstractions. In concrete terms this means
that:
- TSCAura/Android share the same interface
- All interaction of external classes (like RWHVA or WCVA) with TSCA are done
through this common interface
- TSCAura/Android are created via the same factory method (with
platform-specific overrides), which means that the clients also need to have the
same interface. 

To accomplish this we need to make the following changes:
- Make TSC a pure interface class
- Add the following methods to TSC: 
  * void OnWindowMoved();
  * void OnOverscrollStarted();
  * void OnOverscrollCompleted();
  * void OnFlingCompleted();
- Add TSCImpl, implementing TSC - more or less the same as TSC in
crrev.com/698253004/, but with the above 4 methods implemented
- Make TSCCLient extend TouchSelectionMenuClient interface
- Also add the following methods to TSCClient:
  * gfx::Rect GetClientBounds()
  * gfx::NativeView GetNativeView()
- TSCAuraClient is no more
- Add a factory class (with platform-specific overrides) for creating a TSC. The
factory method would take a TSCClient
- RWHVAndroid, RWHVAura, WebContentsViewAura, Textfield would all use TSC
interface and the new factory class.
- Android's implementation of TSC will be TSCImpl
- Aura's implementation of TSC will be TSCAura, which will aggregate TSCImpl and
delegate a bunch of functionality to it.
- TSCAura will not implement TSCClient like it does now - some of the
functionality which is currently in TSCAura would have to be moved to
TouchSelectionControllerAuraClientImpl.
- In the future we will need to do something to share the common details of
Aura's client implementation between contents and Textfield. One option is to
create a TouchSelectioClientImplAura class, which could be aggregated on both
sides.

Thoughts?

[jdduke (slow), 2015-03-24 01:10:11]

On 2015/03/23 22:10:03, mfomitchev wrote:
> I had a quick chat with Sadrul and here's my understanding of his requirements
> to the new design:
> From a high level, he wants the design to be such that if one understands how
> touch selection is used on one platform (e.g. Android), it should be very easy
> for her to understand how it is used on the other platform (e.g. Aura), which
> means that we should use the same abstractions, and the parts that are similar
> should be done via the same shared abstractions. In concrete terms this means
> that:
> - TSCAura/Android share the same interface
> - All interaction of external classes (like RWHVA or WCVA) with TSCA are done
> through this common interface
> - TSCAura/Android are created via the same factory method (with
> platform-specific overrides), which means that the clients also need to have
the
> same interface. 
> 
> To accomplish this we need to make the following changes:
> - Make TSC a pure interface class
> - Add the following methods to TSC: 
>   * void OnWindowMoved();
>   * void OnOverscrollStarted();
>   * void OnOverscrollCompleted();
>   * void OnFlingCompleted();
> - Add TSCImpl, implementing TSC - more or less the same as TSC in
> crrev.com/698253004/, but with the above 4 methods implemented
> - Make TSCCLient extend TouchSelectionMenuClient interface
> - Also add the following methods to TSCClient:
>   * gfx::Rect GetClientBounds()
>   * gfx::NativeView GetNativeView()
> - TSCAuraClient is no more
> - Add a factory class (with platform-specific overrides) for creating a TSC.
The
> factory method would take a TSCClient
> - RWHVAndroid, RWHVAura, WebContentsViewAura, Textfield would all use TSC
> interface and the new factory class.
> - Android's implementation of TSC will be TSCImpl
> - Aura's implementation of TSC will be TSCAura, which will aggregate TSCImpl
and
> delegate a bunch of functionality to it.
> - TSCAura will not implement TSCClient like it does now - some of the
> functionality which is currently in TSCAura would have to be moved to
> TouchSelectionControllerAuraClientImpl.
> - In the future we will need to do something to share the common details of
> Aura's client implementation between contents and Textfield. One option is to
> create a TouchSelectioClientImplAura class, which could be aggregated on both
> sides.
> 
> Thoughts?

No real objections at this point. Hopefully not too much boilerplate, and I
suspect the global interface might get further simplified when Aura switches to
composited selection (no need to hide during movement) and we start coupling
selection + editability into a single packet
(https://codereview.chromium.org/929213004/).

[mohsen, 2015-03-26 12:53:55]

On 2015/03/23 22:10:03, mfomitchev wrote:
> I had a quick chat with Sadrul and here's my understanding of his requirements
> to the new design:
> From a high level, he wants the design to be such that if one understands how
> touch selection is used on one platform (e.g. Android), it should be very easy
> for her to understand how it is used on the other platform (e.g. Aura), which
> means that we should use the same abstractions, and the parts that are similar
> should be done via the same shared abstractions. In concrete terms this means
> that:
> - TSCAura/Android share the same interface
> - All interaction of external classes (like RWHVA or WCVA) with TSCA are done
> through this common interface
> - TSCAura/Android are created via the same factory method (with
> platform-specific overrides), which means that the clients also need to have
the
> same interface. 
> 
> To accomplish this we need to make the following changes:
> - Make TSC a pure interface class
> - Add the following methods to TSC: 
>   * void OnWindowMoved();
>   * void OnOverscrollStarted();
>   * void OnOverscrollCompleted();
>   * void OnFlingCompleted();
There is also OnGestureEvent(GestureEvent*) and OnTouchEvent(TouchEvent*) in
TSCAura overridden from EventHandler. This means that in Aura we'll use
OnGestureEvent() and OnTouchEvent() to send events to TSC, but in Android we'll
use OnTapEvent(), OnLongPressEvent(), and WillHandleTouchEvent(const
MotionEvent&).

This and other functions in the interface and in the client interface that are
only used in one platform mean that still if one understands how touch selection
is used on one platform (e.g. Android), it would not necessarily be very easy
for her to understand how it is used on the other platform (e.g. Aura).

> - Add TSCImpl, implementing TSC - more or less the same as TSC in
> crrev.com/698253004/, but with the above 4 methods implemented
> - Make TSCCLient extend TouchSelectionMenuClient interface
> - Also add the following methods to TSCClient:
>   * gfx::Rect GetClientBounds()
>   * gfx::NativeView GetNativeView()
> - TSCAuraClient is no more
> - Add a factory class (with platform-specific overrides) for creating a TSC.
The
> factory method would take a TSCClient
> - RWHVAndroid, RWHVAura, WebContentsViewAura, Textfield would all use TSC
> interface and the new factory class.
> - Android's implementation of TSC will be TSCImpl
> - Aura's implementation of TSC will be TSCAura, which will aggregate TSCImpl
and
> delegate a bunch of functionality to it.
> - TSCAura will not implement TSCClient like it does now - some of the
> functionality which is currently in TSCAura would have to be moved to
> TouchSelectionControllerAuraClientImpl.
> - In the future we will need to do something to share the common details of
> Aura's client implementation between contents and Textfield. One option is to
> create a TouchSelectioClientImplAura class, which could be aggregated on both
> sides.
> 
> Thoughts?

[jdduke (slow), 2015-03-26 15:10:18]

On 2015/03/26 12:53:55, mohsen wrote:
> > To accomplish this we need to make the following changes:
> > - Make TSC a pure interface class
> > - Add the following methods to TSC: 
> >   * void OnWindowMoved();
> >   * void OnOverscrollStarted();
> >   * void OnOverscrollCompleted();
> >   * void OnFlingCompleted();
> There is also OnGestureEvent(GestureEvent*) and OnTouchEvent(TouchEvent*) in
> TSCAura overridden from EventHandler. This means that in Aura we'll use
> OnGestureEvent() and OnTouchEvent() to send events to TSC, but in Android
we'll
> use OnTapEvent(), OnLongPressEvent(), and WillHandleTouchEvent(const
> MotionEvent&).
> 
> This and other functions in the interface and in the client interface that are
> only used in one platform mean that still if one understands how touch
selection
> is used on one platform (e.g. Android), it would not necessarily be very easy
> for her to understand how it is used on the other platform (e.g. Aura).

He has a point. There's a lot of value in having a common interface, but there's
also a danger when that interface becomes bloated with platform-specific
concerns and differences. It's not immediately clear that this new design will
be any better or more comprehensible than Mohsen's original design, but we could
probably debate that for months and not make any progress. At this point, I no
longer have a strong opinion; I think the design will evolve after it lands
regardless, so I'll just be happy if it lands, it works, and the design isn't
offensive (none of them so far have been...).

[jdduke (slow), 2015-03-26 15:11:36]

On 2015/03/26 15:10:18, jdduke wrote:
> On 2015/03/26 12:53:55, mohsen wrote:
> > > To accomplish this we need to make the following changes:
> > > - Make TSC a pure interface class
> > > - Add the following methods to TSC: 
> > >   * void OnWindowMoved();
> > >   * void OnOverscrollStarted();
> > >   * void OnOverscrollCompleted();
> > >   * void OnFlingCompleted();
> > There is also OnGestureEvent(GestureEvent*) and OnTouchEvent(TouchEvent*) in
> > TSCAura overridden from EventHandler. This means that in Aura we'll use
> > OnGestureEvent() and OnTouchEvent() to send events to TSC, but in Android
> we'll
> > use OnTapEvent(), OnLongPressEvent(), and WillHandleTouchEvent(const
> > MotionEvent&).
> > 
> > This and other functions in the interface and in the client interface that
are
> > only used in one platform mean that still if one understands how touch
> selection
> > is used on one platform (e.g. Android), it would not necessarily be very
easy
> > for her to understand how it is used on the other platform (e.g. Aura).
> 
> He has a point. There's a lot of value in having a common interface, but
there's
> also a danger when that interface becomes bloated with platform-specific
> concerns and differences. It's not immediately clear that this new design will
> be any better or more comprehensible than Mohsen's original design, but we
could
> probably debate that for months and not make any progress. At this point, I no
> longer have a strong opinion; I think the design will evolve after it lands
> regardless, so I'll just be happy if it lands, it works, and the design isn't
> offensive (none of them so far have been...).

As an aside, if we're concerned about the Aura-specific interface additions, we
can always guard them behind a define at which point it becomes immediately
clear to which platform those methods apply. There's precedent for this in many
of the common base interfaces in content/, so there's that.

[mfomitchev, 2015-03-26 17:03:32]

> TSCAura overridden from EventHandler. This means that in Aura we'll use
> OnGestureEvent() and OnTouchEvent() to send events to TSC, but in Android
we'll
> use OnTapEvent(), OnLongPressEvent(), and WillHandleTouchEvent(const
> MotionEvent&).

OnGestureEvent and OnTouchEvent will not be in the TSC interface, it's an
internal implementation detail. From the outside POV the difference is that you
need to feed events to the Android's implementation, while with Aura the
controller you don't need to worry about that as long as you return the
appropriate Window in GetNativeView(). Granted, such difference in behavior is
still sort of odd. We could factor this out into
TouchSelectionControllerAuraEventAdaptor and something like that and hook it up
from the outside of the controller.. 

> It's not immediately clear that this new design will
> be any better or more comprehensible than Mohsen's original design, but we
could
> probably debate that for months and not make any progress. At this point, I no
> longer have a strong opinion; I think the design will evolve after it lands
> regardless, so I'll just be happy if it lands, it works, and the design isn't
> offensive (none of them so far have been...).

I have a somewhat similar sentiment :)

[mohsen, 2015-05-01 18:12:37]

sadrul@, jdduke@: Now that the touch selection unification patch has gone
through a redesign, can you please take another look at this patch which is the
first piece I'm trying to land...

[mohsen, 2015-05-07 18:04:39]

Patchset #15 (id:320001) has been deleted

[mohsen, 2015-05-07 19:59:36]

Patchset #16 (id:360001) has been deleted

[mohsen, 2015-05-07 20:09:07]

Per sadrul@'s request, I've made to changes to the patch:
1. Removed menu runner code from this patch;
2. Created an enum instead of two booleans for active status of insertion and
selection.

Please take another look...

[jdduke (slow), 2015-05-07 20:38:15]

Most changes lgtm though I may not be the best reviewer for
touch_handle_drawable_aura.{cc,h}.

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:28: const int
kSelectionHandlePadding = 0;
Hmm, do we need this if it's always zero? Or you just like the flexibility?

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:139: bool visible =
!IsNearlyZero(alpha_);
Would a |IsVisible()| method that returns |enabled_ && !IsNearlyZero(alpha_))|
help?

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:222:
client_->OnSelectionEvent(SELECTION_MOVED);
Have we come to a conclusion on whether this is still necessary?

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:39: enum ActiveStatus {
Do we need to expose this publicly?

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/ui_t...
File ui/touch_selection/ui_touch_selection.gyp (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/ui_t...
ui/touch_selection/ui_touch_selection.gyp:43: 'dependencies!': [
Wouldn't it be simpler to reverse this condition and only add the
sources/dependencies here for Aura?

[mohsen, 2015-05-07 21:21:55]

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:28: const int
kSelectionHandlePadding = 0;
On 2015/05/07 at 20:38:15, jdduke wrote:
> Hmm, do we need this if it's always zero? Or you just like the flexibility?

This is something that we might want to tune later when we switch handles to use
aura targeting (currently, handles rely on the targeting performed in
TouchSelectionController; but we need to switch to aura targeting soon).

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:139: bool visible =
!IsNearlyZero(alpha_);
On 2015/05/07 at 20:38:15, jdduke wrote:
> Would a |IsVisible()| method that returns |enabled_ && !IsNearlyZero(alpha_))|
help?

Definitely, Done.

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:222:
client_->OnSelectionEvent(SELECTION_MOVED);
On 2015/05/07 at 20:38:15, jdduke wrote:
> Have we come to a conclusion on whether this is still necessary?

This is here for the case where user drags a window around and we want to hide
the quick menu during the drag.

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.h:39: enum ActiveStatus {
On 2015/05/07 at 20:38:15, jdduke wrote:
> Do we need to expose this publicly?

In a few places for Aura implementation, we'll need to know if touch selection
is active or not. That's why active_status() method (and this enum) is public.
For example, in the main patch (http://crrev.com/698253004), see line 2183 of
render_widget_host_view_aura.cc and line 167 of
touch_selection_controller_client_aura.cc. (they're still using
is_insertion_active()/is_selection_active() which after this patch would change
to active_status())

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/ui_t...
File ui/touch_selection/ui_touch_selection.gyp (right):

https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/ui_t...
ui/touch_selection/ui_touch_selection.gyp:43: 'dependencies!': [
On 2015/05/07 at 20:38:15, jdduke wrote:
> Wouldn't it be simpler to reverse this condition and only add the
sources/dependencies here for Aura?

GYP documentation suggests adding all sources first, and then removing
platform-specific ones later on; because "the sources list contains a useful
global list of all sources on all platforms with consistent sorting on all
platforms".

[jdduke (slow), 2015-05-07 21:25:39]

On 2015/05/07 21:21:55, mohsen wrote:
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> File ui/touch_selection/touch_selection_controller.cc (right):
> 
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> ui/touch_selection/touch_selection_controller.cc:222:
> client_->OnSelectionEvent(SELECTION_MOVED);
> On 2015/05/07 at 20:38:15, jdduke wrote:
> > Have we come to a conclusion on whether this is still necessary?
> 
> This is here for the case where user drags a window around and we want to hide
> the quick menu during the drag.

I understand why it's here now, I'm asking about our offline discussion where we
discussed making these methods Aura-specific (made possibly by allowing the
WebContents public reference).

[mohsen, 2015-05-07 21:44:14]

On 2015/05/07 21:25:39, jdduke wrote:
> On 2015/05/07 21:21:55, mohsen wrote:
> >
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> > File ui/touch_selection/touch_selection_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> > ui/touch_selection/touch_selection_controller.cc:222:
> > client_->OnSelectionEvent(SELECTION_MOVED);
> > On 2015/05/07 at 20:38:15, jdduke wrote:
> > > Have we come to a conclusion on whether this is still necessary?
> > 
> > This is here for the case where user drags a window around and we want to
hide
> > the quick menu during the drag.
> 
> I understand why it's here now, I'm asking about our offline discussion where
we
> discussed making these methods Aura-specific (made possibly by allowing the
> WebContents public reference).

I see. That discussion was actually about OnSelectionScroll* methods which are
to be called on TSCClientAura from RWHVAura. Here, the situation is different
(and actually better) since OnNativeViewMoved is to be called from WCVAura which
is in web_conents/ and has access to TSCClientAura. I'll remove this function
from TouchSelectionController.

[mohsen, 2015-05-07 21:44:17]

On 2015/05/07 21:25:39, jdduke wrote:
> On 2015/05/07 21:21:55, mohsen wrote:
> >
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> > File ui/touch_selection/touch_selection_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> > ui/touch_selection/touch_selection_controller.cc:222:
> > client_->OnSelectionEvent(SELECTION_MOVED);
> > On 2015/05/07 at 20:38:15, jdduke wrote:
> > > Have we come to a conclusion on whether this is still necessary?
> > 
> > This is here for the case where user drags a window around and we want to
hide
> > the quick menu during the drag.
> 
> I understand why it's here now, I'm asking about our offline discussion where
we
> discussed making these methods Aura-specific (made possibly by allowing the
> WebContents public reference).

I see. That discussion was actually about OnSelectionScroll* methods which are
to be called on TSCClientAura from RWHVAura. Here, the situation is different
(and actually better) since OnNativeViewMoved is to be called from WCVAura which
is in web_conents/ and has access to TSCClientAura. I'll remove this function
from TouchSelectionController.

[jdduke (slow), 2015-05-07 22:23:42]

On 2015/05/07 21:44:17, mohsen wrote:
> On 2015/05/07 21:25:39, jdduke wrote:
> > On 2015/05/07 21:21:55, mohsen wrote:
> > >
> >
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> > > File ui/touch_selection/touch_selection_controller.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/996373002/diff/380001/ui/touch_selection/touc...
> > > ui/touch_selection/touch_selection_controller.cc:222:
> > > client_->OnSelectionEvent(SELECTION_MOVED);
> > > On 2015/05/07 at 20:38:15, jdduke wrote:
> > > > Have we come to a conclusion on whether this is still necessary?
> > > 
> > > This is here for the case where user drags a window around and we want to
> hide
> > > the quick menu during the drag.
> > 
> > I understand why it's here now, I'm asking about our offline discussion
where
> we
> > discussed making these methods Aura-specific (made possibly by allowing the
> > WebContents public reference).
> 
> I see. That discussion was actually about OnSelectionScroll* methods which are
> to be called on TSCClientAura from RWHVAura. Here, the situation is different
> (and actually better) since OnNativeViewMoved is to be called from WCVAura
which
> is in web_conents/ and has access to TSCClientAura. I'll remove this function
> from TouchSelectionController.

Perfect, thanks!

[jdduke (slow), 2015-05-08 15:44:16]

On 2015/05/07 21:21:55, mohsen wrote:
> GYP documentation suggests adding all sources first, and then removing
> platform-specific ones later on; because "the sources list contains a useful
> global list of all sources on all platforms with consistent sorting on all
> platforms".

Hmm, OK then :) Latest changeset still lgtm.

[sadrul, 2015-05-11 16:06:57]

A couple of nits. lgtm

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:96: window_->Hide();
Are these Show()/Hide() animated?

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:127: };
If you haven't already, make sure to test for RTL as well.

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.h (right):

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.h:48: gfx::PointF focal_position_;
focal_position_ is in the parent's coord space? Can you clarify in the comment?

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:170: OnInsertionChanged();
Add braces in this block.

[mohsen, 2015-05-11 16:55:40]

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.cc (right):

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:96: window_->Hide();
On 2015/05/11 at 16:06:57, sadrul wrote:
> Are these Show()/Hide() animated?

Yes, they use the default animation for visibility change in aura.

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.cc:127: };
On 2015/05/11 at 16:06:57, sadrul wrote:
> If you haven't already, make sure to test for RTL as well.

Yeah, this is tested and works fine.

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
File ui/touch_selection/touch_handle_drawable_aura.h (right):

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_handle_drawable_aura.h:48: gfx::PointF focal_position_;
On 2015/05/11 at 16:06:57, sadrul wrote:
> focal_position_ is in the parent's coord space? Can you clarify in the
comment?

Yes, Done.

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/996373002/diff/420001/ui/touch_selection/touc...
ui/touch_selection/touch_selection_controller.cc:170: OnInsertionChanged();
On 2015/05/11 at 16:06:57, sadrul wrote:
> Add braces in this block.

Done.

[mohsen, 2015-05-11 18:02:48]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2015-05-11 18:02:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/996373002/#ps440001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2015-05-11 18:07:00]

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

[commit-bot: I haz the power, 2015-05-11 18:17:36]

Committed patchset #19 (id:440001)

[commit-bot: I haz the power, 2015-05-11 18:19:36]

Patchset 19 (id:??) landed as
https://crrev.com/9143361454a599ac0882abcbd846b25ad7ce7945
Cr-Commit-Position: refs/heads/master@{#329191}

[davve, 2015-05-13 15:58:57]

I think this change broke the component=shared_library build, at least for my
config (gcc, no debug fission, no bundled binutils).

See for https://codereview.chromium.org/1137323002 illustration "fix". Note that
I have actually no deep understanding on what's going on.

[sadrul, 2015-05-13 16:01:44]

On 2015/05/13 15:58:57, David Vest wrote:
> I think this change broke the component=shared_library build, at least for my
> config (gcc, no debug fission, no bundled binutils).
> 
> See for https://codereview.chromium.org/1137323002 illustration "fix". Note
that
> I have actually no deep understanding on what's going on.

The fix looks reasonable (should update BUILD.gn too). Any idea why the failure
is not showing up in the bots?

[mohsen, 2015-05-13 16:57:18]

On 2015/05/13 16:01:44, sadrul wrote:
> On 2015/05/13 15:58:57, David Vest wrote:
> > I think this change broke the component=shared_library build, at least for
my
> > config (gcc, no debug fission, no bundled binutils).
> > 
> > See for https://codereview.chromium.org/1137323002 illustration "fix". Note
> that
> > I have actually no deep understanding on what's going on.
> 
> The fix looks reasonable (should update BUILD.gn too). Any idea why the
failure
> is not showing up in the bots?

Is it related to compiler being GCC?

[davve, 2015-05-14 07:16:22]

On 2015/05/13 16:57:18, mohsen wrote:
> On 2015/05/13 16:01:44, sadrul wrote:
> > On 2015/05/13 15:58:57, David Vest wrote:
> > > I think this change broke the component=shared_library build, at least for
> my
> > > config (gcc, no debug fission, no bundled binutils).
> > > 
> > > See for https://codereview.chromium.org/1137323002 illustration "fix".
Note
> > that
> > > I have actually no deep understanding on what's going on.
> > 
> > The fix looks reasonable (should update BUILD.gn too). Any idea why the
> failure
> > is not showing up in the bots?
> 
> Is it related to compiler being GCC?

Maybe. Some people at Opera still use GCC still in order to build with icecc
(distributed compilation system). Testing with clang takes a bit of time, but I
can try it on Monday to see what falls out.