[CRD iOS] Hook the touch input feedback

[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position, which can simplify a
   few things.

BUG=718704
Review-Url: https://codereview.chromium.org/2879743002
Cr-Commit-Position: refs/heads/master@{#472337}
Committed: https://chromium.googlesource.com/chromium/src/+/7fbc8055f73d33056aafa74a39b8a0f3b5c99547

[Yuwei, 2017-05-12 19:08:39]

Description was changed from

==========
[CRD iOS] Hook the touch input feedback

WIP
==========

to

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hooks the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position can simplify a few
   things.

BUG=718704
==========

[Yuwei, 2017-05-12 19:08:51]

Description was changed from

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hooks the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position can simplify a few
   things.

BUG=718704
==========

to

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position can simplify a few
   things.

BUG=718704
==========

[Yuwei, 2017-05-12 19:09:14]

Description was changed from

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position can simplify a few
   things.

BUG=718704
==========

to

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position, which can simplify a
few
   things.

BUG=718704
==========

[Yuwei, 2017-05-12 19:10:00]

Description was changed from

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position, which can simplify a
few
   things.

BUG=718704
==========

to

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position, which can simplify a
   few things.

BUG=718704
==========

[Yuwei, 2017-05-12 19:10:42]

yuweih@chromium.org changed reviewers:
+ nicholss@chromium.org

[Yuwei, 2017-05-12 19:10:43]

PTAL thanks!

[nicholss, 2017-05-12 21:42:41]

Rather than add the the new "render stub" can you think of a away to use the
"Drawable" object to give the touch feedback? That was my intent when adding
those classes.

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/gesture...
File remoting/client/gesture_interpreter.cc (right):

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/gesture...
remoting/client/gesture_interpreter.cc:109: SetCursorPositionOnRenderStub();
I don't think "stub" is the correct term. See
https://en.wikipedia.org/wiki/Method_stub

Stub implies the real implementation has not been finished. You mean interface
or abstraction or something else.

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
File remoting/client/render_stub.h (right):

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
remoting/client/render_stub.h:13: class RenderStub {
Yeah stub is wrong. Stub would be the class you use to test with, not the
generic interface of a class.

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
remoting/client/render_stub.h:17: virtual void SetTransformation(const
ViewMatrix& transformation) = 0;
These methods need documentation please.

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
File remoting/client/render_stub_proxy.h (right):

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
remoting/client/render_stub_proxy.h:33: void RunTaskOnProperThread(const
base::Closure& task,
This method is doing magic task things and I don't understand it. Can you
document it here?

[Yuwei, 2017-05-13 00:28:42]

Patchset #4 (id:60001) has been deleted

[Yuwei, 2017-05-13 00:41:52]

PTAL

As discussed offline, the fact that:
1. GlRenderer still owns all the drawables, including the touch feedback
drawable, not the other way around
2. Currently there is no dependency between /client and /client/display, and not
even any common dependency, making it hard to maintain a Drawable in
GestureInterpreter.

So I could suggest using this approach until we get to the refactoring phase and
have some future discussions.

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/gesture...
File remoting/client/gesture_interpreter.cc (right):

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/gesture...
remoting/client/gesture_interpreter.cc:109: SetCursorPositionOnRenderStub();
On 2017/05/12 21:42:40, nicholss wrote:
> I don't think "stub" is the correct term. See
> https://en.wikipedia.org/wiki/Method_stub
> 
> Stub implies the real implementation has not been finished. You mean interface
> or abstraction or something else.

Done. Renamed to RendererFacade.  We did have ClipboardStub, HostStub, etc. so I
chose that name...

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
File remoting/client/render_stub.h (right):

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
remoting/client/render_stub.h:13: class RenderStub {
On 2017/05/12 21:42:40, nicholss wrote:
> Yeah stub is wrong. Stub would be the class you use to test with, not the
> generic interface of a class.

Fixed.

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
remoting/client/render_stub.h:17: virtual void SetTransformation(const
ViewMatrix& transformation) = 0;
On 2017/05/12 21:42:40, nicholss wrote:
> These methods need documentation please.

Done. Added a comment that points to GlRenderer :P

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
File remoting/client/render_stub_proxy.h (right):

https://codereview.chromium.org/2879743002/diff/40001/remoting/client/render_...
remoting/client/render_stub_proxy.h:33: void RunTaskOnProperThread(const
base::Closure& task,
On 2017/05/12 21:42:40, nicholss wrote:
> This method is doing magic task things and I don't understand it. Can you
> document it here?

Done.

[Yuwei, 2017-05-13 00:43:15]

*further discussions. But anyway O_o

[Yuwei, 2017-05-13 07:40:56]

Patchset #5 (id:100001) has been deleted

[nicholss, 2017-05-15 15:49:40]

Nits about the names of the interfaces.

Will you be doing a followup to integrate these changes into Android as well?

https://codereview.chromium.org/2879743002/diff/120001/remoting/client/BUILD.gn
File remoting/client/BUILD.gn (right):

https://codereview.chromium.org/2879743002/diff/120001/remoting/client/BUILD....
remoting/client/BUILD.gn:54: "renderer_facade_proxy.cc",
Please also add renderer_facade.h

https://codereview.chromium.org/2879743002/diff/120001/remoting/client/render...
File remoting/client/renderer_facade_proxy.h (right):

https://codereview.chromium.org/2879743002/diff/120001/remoting/client/render...
remoting/client/renderer_facade_proxy.h:20: class RendererFacadeProxy : public
RendererFacade {
I am not sure I understand the benefit to having both the virtual class
RenderFacade and RendererFacadeProxy.

Could you do the same thing with just a class called RendererProxy? Do you plan
to have other implementations of the proxy class?

https://codereview.chromium.org/2879743002/diff/120001/remoting/ios/display/g...
File remoting/ios/display/gl_display_handler.h (right):

https://codereview.chromium.org/2879743002/diff/120001/remoting/ios/display/g...
remoting/ios/display/gl_display_handler.h:51: -
(std::unique_ptr<remoting::RendererFacade>)CreateRendererFacade;
Facade is the wrong word also. 

Facade means to hide a complex interface or set of interfaces behind a simpler
interface. This seems to be more of a proxy to me.

[Yuwei, 2017-05-15 18:38:29]

I will eventually do a refactoring on GlRenderer for Android and iOS but I don't
have any plan yet.

We probably want GestureInterpreter to hold the Drawables and add their weak
references to GlRenderer. There will be more works to do.

The renderer proxy is for unblocking iOS development for now and I don't think
this is what we eventually want to do when we get to the refactoring phase.

https://codereview.chromium.org/2879743002/diff/120001/remoting/client/render...
File remoting/client/renderer_facade_proxy.h (right):

https://codereview.chromium.org/2879743002/diff/120001/remoting/client/render...
remoting/client/renderer_facade_proxy.h:20: class RendererFacadeProxy : public
RendererFacade {
On 2017/05/15 15:49:40, nicholss wrote:
> I am not sure I understand the benefit to having both the virtual class
> RenderFacade and RendererFacadeProxy.
> 
> Could you do the same thing with just a class called RendererProxy? Do you
plan
> to have other implementations of the proxy class?

I could like to have a proxy that simply wraps the GlRenderer but currently
/client doesn't depend on /client/display and I'm not sure whether we should add
that dependency...

I'm also thinking about moving the ui stuff into /client/ui and makes it depend
on /client and /client/display. WDYT?

[Yuwei, 2017-05-15 22:13:45]

Patchset #6 (id:140001) has been deleted

[Yuwei, 2017-05-15 22:37:27]

Patchset #6 (id:160001) has been deleted

[Yuwei, 2017-05-15 23:47:16]

I'm just thinking maybe we should do refactoring right now, before checking in
this code.

What we need to do is probably:
1. Move Drawables out of GlRenderer. Make GlRenderer just render a list of
Drawables.
2. Create and hold Drawables inside GestureInterpreter and add them to
GlRenderer.
3. Before the Android client uses client/ui stuff, we can wrap the changes
inside JniGlDisplayHandler::Core

We can discuss about this when you are back. Meanwhile I'll pull out the
refactoring code on InputStrategy.

[nicholss, 2017-05-16 18:17:24]

LGTM, nice refactoring.

https://codereview.chromium.org/2879743002/diff/220001/remoting/client/ui/inp...
File remoting/client/ui/input_strategy.h (right):

https://codereview.chromium.org/2879743002/diff/220001/remoting/client/ui/inp...
remoting/client/ui/input_strategy.h:20: LONG_PRESS_FEEDBACK,
iOS will have force touch but we need to figure out what that will do and maybe
even change the gestures for phones that support it. just fyi for future
features

[Yuwei, 2017-05-16 19:01:43]

Thanks!

I was talking about refactoring the GlRenderer. I took a look yesterday and
realized that it has so much work to do, so I'm reluctant to do it now :)

Sorry for changing mind back-and-forth :)

https://codereview.chromium.org/2879743002/diff/220001/remoting/client/ui/inp...
File remoting/client/ui/input_strategy.h (right):

https://codereview.chromium.org/2879743002/diff/220001/remoting/client/ui/inp...
remoting/client/ui/input_strategy.h:20: LONG_PRESS_FEEDBACK,
On 2017/05/16 18:17:24, nicholss wrote:
> iOS will have force touch but we need to figure out what that will do and
maybe
> even change the gestures for phones that support it. just fyi for future
> features

Good to mention :)
I think the concern is the function and member name of GestureInterpreter are
dependent on the actual gesture being done so changing the gesture mapping will
make the name inconsistent.

Anyway... We can discuss more when we get to that.

[Yuwei, 2017-05-16 19:02:02]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-16 19:03: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-16 19:11:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 19:11:51]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Yuwei, 2017-05-16 19:27:19]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-16 19:29:18]

CQ is trying da patch.

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

[Yuwei, 2017-05-16 19:48:50]

The CQ bit was unchecked by yuweih@chromium.org

[Yuwei, 2017-05-16 19:49:35]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-16 19:49:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org
Link to the patchset: https://codereview.chromium.org/2879743002/#ps240001
(title: "Fix NOTREACHED return")

[commit-bot: I haz the power, 2017-05-16 19:51:05]

CQ is trying da patch.

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

[Yuwei, 2017-05-16 21:35:05]

The CQ bit was unchecked by yuweih@chromium.org

[Yuwei, 2017-05-16 22:01:47]

Patchset #10 (id:260001) has been deleted

[Yuwei, 2017-05-16 22:02:15]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-16 22:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org
Link to the patchset: https://codereview.chromium.org/2879743002/#ps280001
(title: "Fix dependency and race condition")

[commit-bot: I haz the power, 2017-05-16 22:05:53]

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-16 23:30:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 23:30:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuwei, 2017-05-16 23:40:36]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-16 23:41:45]

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-17 05:23:54]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1494978036705170,
"parent_rev": "26d71b4f8cbf1fd071ae1817918857613d9338f7", "commit_rev":
"7fbc8055f73d33056aafa74a39b8a0f3b5c99547"}

[commit-bot: I haz the power, 2017-05-17 05:24:09]

Description was changed from

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position, which can simplify a
   few things.

BUG=718704
==========

to

==========
[CRD iOS] Hook the touch feedback animation and some refactoring

This CL:
1. Refactors a RenderStub interface to be used by GestureInterpreter to control
   the renderer directly.
2. Hook the touch feedback animation to GestureInterpreter.
3. Redesign the cursor tracking of the InputStrategy a little bit. Simply let
   the InputStrategy itself to track the cursor position, which can simplify a
   few things.

BUG=718704

Review-Url: https://codereview.chromium.org/2879743002
Cr-Commit-Position: refs/heads/master@{#472337}
Committed:
https://chromium.googlesource.com/chromium/src/+/7fbc8055f73d33056aafa74a39b8...
==========

[commit-bot: I haz the power, 2017-05-17 05:24:10]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/7fbc8055f73d33056aafa74a39b8...