[Remoting iOS] Basic viewport manipulation support

[Remoting iOS] Basic viewport manipulation support

This CL adds some basic zoom and pan support (touch input mode only for now)
for the iOS client.

This is minimum code to make use of DesktopViewport. The client gesture stuff
may still need more refactoring or redesign to support trackpad input mode,
deceleration animation, etc.

BUG=714193
Review-Url: https://codereview.chromium.org/2856933007
Cr-Commit-Position: refs/heads/master@{#469414}
Committed: https://chromium.googlesource.com/chromium/src/+/a4d96d0d8bdcc688451ad0376a7304df5b41904a

[Yuwei, 2017-05-03 05:52:34]

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

[Yuwei, 2017-05-03 05:52:35]

PTAL Thanks!

Note that zooming is broken (pivot point shifts off the actual center of
gesture) until CL 2857113002 get landed.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/cli...
File remoting/client/ios/client_gestures.mm (right):

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/cli...
remoting/client/ios/client_gestures.mm:107: CGPoint pivot = [sender
locationInView:_view];
I'm planning on creating a new C++ class to be shared with Android to handle
touch events, so eventually these code may be moved to that class and this class
may just become a pure adapter.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:151: [gl_view_ display];
Turns out `setNeedsDisplay` is async and will throttle requests if it's called
too frequently. That broke the viewport experience.

`display` is synchronous and will block until rendering is done (takes ~3ms),
which is more appropriate in this case.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:205:
std::unique_ptr<remoting::QueuedTaskPoster> _uiTaskPoster;
FYI this object is for input synchronization. Basically all tasks posted using
this object inside one tick will be run on the other thread within the same
tick.

[nicholss, 2017-05-03 15:33:50]

I think we need to do a breakout session and talk about what the vision of the
architecture should be. I appreciate the work you are doing but this is not
really in line with what I have planned for this work (and it is not your fault,
I have not documented it well yet). So lets do a little diagramming and get on
the same page. 

Ping me when you are in. Thanks!

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:87: _viewport.reset(new
remoting::DesktopViewport());
I would rather see viewport be a part of the display handler and we ask the
display handler to translate and whatnot. I would like to minimize the amount of
logic in the native side as much as possible.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:113: CGRect surfaceFrame =
((GLKView*)self.view).frame;
No need to cast, frame comes from UIView.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:125: -
(void)canvasSizeChanged:(CGSize)size {
should have a space between the method and the pragma mark

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/cli...
File remoting/client/ios/client_gestures.mm (right):

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/cli...
remoting/client/ios/client_gestures.mm:107: CGPoint pivot = [sender
locationInView:_view];
On 2017/05/03 05:52:35, Yuwei wrote:
> I'm planning on creating a new C++ class to be shared with Android to handle
> touch events, so eventually these code may be moved to that class and this
class
> may just become a pure adapter.

Note this class is copy/paste from the original bad code.

[Yuwei, 2017-05-03 22:28:37]

PTAL. Thanks!

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:87: _viewport.reset(new
remoting::DesktopViewport());
On 2017/05/03 15:33:50, nicholss wrote:
> I would rather see viewport be a part of the display handler and we ask the
> display handler to translate and whatnot. I would like to minimize the amount
of
> logic in the native side as much as possible. 

I created a C++ GestureInterpreter in RemotingClient and moved the viewport
there. I put the viewport/GestureInterpreter in RemotingClient rather than the
display handler since it will also be used for injecting mouse clicks, etc.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:113: CGRect surfaceFrame =
((GLKView*)self.view).frame;
On 2017/05/03 15:33:50, nicholss wrote:
> No need to cast, frame comes from UIView.

Done.

https://codereview.chromium.org/2856933007/diff/20001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:125: -
(void)canvasSizeChanged:(CGSize)size {
On 2017/05/03 15:33:50, nicholss wrote:
> should have a space between the method and the pragma mark

Done.

[nicholss, 2017-05-03 22:47:35]

Looking closer! Few comments and questions

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/app...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:102: [_client.displayHandler
onSurfaceChanged:surfaceFrame];
Maybe we could tell the client that the surface changed and then the client can
tell the display handler and the gesture interpreter  the frame has changed size
rather than having the host view controller do this.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:116:
_client.gestureInterpreter->OnDesktopSizeChanged(size.width, size.height);
Do we still need the host view controller to tell the gesture interpreter this?
Seems like the _client can do it instead.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/cli...
File remoting/client/ios/client_gestures.mm (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/cli...
remoting/client/ios/client_gestures.mm:11: #include
"remoting/client/gesture_interpreter.h"
style says  #imports before #includes per level, just flip these fellas

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.h (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:10: #include <array>
new line between #import and include
 
Where are you using array in the .h file?

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:13: #import
"remoting/client/display/sys_opengl.h"
sort the includes, #imports first

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:28: @protocol
GlDisplayHandlerDelegate<NSObject>
Can you also add a comment describing the what the goal of the delegate is?

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:40: -
(void)setDelegate:(id<GlDisplayHandlerDelegate>)delegate;
Better to use @property delegate, for example:
https://cs.chromium.org/chromium/src/remoting/client/ios/key_input.h?type=cs&...

And then you can still call setDelegate but you really should use
object.delegate = someDelegate;

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:232: -
(void)setDelegate:(id<GlDisplayHandlerDelegate>)delegate {
You can keep this here, this will override the auto-generated.

Mark your property as write only and don't synthesize it. (I think that will
work...)

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:267: _uiTaskPoster->AddTask(
How do you choose to AddTask on the task poster vs PostTask on the task runner?

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:274: -
(void)glkView:(GLKView*)view drawInRect:(CGRect)rect {
Can you delete this? It is not used, just cruft I left around.

[Yuwei, 2017-05-04 00:28:07]

PTAL Thanks!

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/app...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:102: [_client.displayHandler
onSurfaceChanged:surfaceFrame];
On 2017/05/03 22:47:35, nicholss wrote:
> Maybe we could tell the client that the surface changed and then the client
can
> tell the display handler and the gesture interpreter  the frame has changed
size
> rather than having the host view controller do this.

Done.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:116:
_client.gestureInterpreter->OnDesktopSizeChanged(size.width, size.height);
On 2017/05/03 22:47:35, nicholss wrote:
> Do we still need the host view controller to tell the gesture interpreter
this?
> Seems like the _client can do it instead.

Nope. Move delegate to RemotingClient instead.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/cli...
File remoting/client/ios/client_gestures.mm (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/cli...
remoting/client/ios/client_gestures.mm:11: #include
"remoting/client/gesture_interpreter.h"
On 2017/05/03 22:47:35, nicholss wrote:
> style says  #imports before #includes per level, just flip these fellas 

Done.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.h (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:10: #include <array>
On 2017/05/03 22:47:35, nicholss wrote:
> new line between #import and include
>  
> Where are you using array in the .h file?

Removed. Not used anymore.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:13: #import
"remoting/client/display/sys_opengl.h"
On 2017/05/03 22:47:35, nicholss wrote:
> sort the includes, #imports first

Done.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:28: @protocol
GlDisplayHandlerDelegate<NSObject>
On 2017/05/03 22:47:35, nicholss wrote:
> Can you also add a comment describing the what the goal of the delegate is?

Done.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:40: -
(void)setDelegate:(id<GlDisplayHandlerDelegate>)delegate;
On 2017/05/03 22:47:35, nicholss wrote:
> Better to use @property delegate, for example:
>
https://cs.chromium.org/chromium/src/remoting/client/ios/key_input.h?type=cs&...
> 
> And then you can still call setDelegate but you really should use
> object.delegate = someDelegate;

Done. Manually synthesized with trick to disable the getter. ObjC doesn't
support write-only @property...

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:232: -
(void)setDelegate:(id<GlDisplayHandlerDelegate>)delegate {
On 2017/05/03 22:47:35, nicholss wrote:
> You can keep this here, this will override the auto-generated.
> 
> Mark your property as write only and don't synthesize it. (I think that will
> work...)

Done. Moved below.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:267: _uiTaskPoster->AddTask(
On 2017/05/03 22:47:35, nicholss wrote:
> How do you choose to AddTask on the task poster vs PostTask on the task
runner?

UITaskPoster is used to synchronize UI events that should be rendered in the
same frame.

We once ran into issue with synchronizing cursor and viewport movements. Say if
we want to do this sequence:
1. Move cursor by <1, 1>.
2. Move viewport in the same direction.

Without synchronizing, rendering can happen between step 1 and 2, and cause the
cursor to randomly jiggle.

I think cursor and viewport movements are the only things currently needed to be
synchronized.

https://codereview.chromium.org/2856933007/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:274: -
(void)glkView:(GLKView*)view drawInRect:(CGRect)rect {
On 2017/05/03 22:47:35, nicholss wrote:
> Can you delete this? It is not used, just cruft I left around.

Done.

[Yuwei, 2017-05-04 03:53:17]

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

[commit-bot: I haz the power, 2017-05-04 03:53:25]

Dry run: 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-04 04:56:27]

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

[commit-bot: I haz the power, 2017-05-04 04:56:28]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-05-04 18:34:59]

Thanks for the work! LGTM

[Yuwei, 2017-05-04 19:00:33]

On 2017/05/04 18:34:59, nicholss wrote:
> Thanks for the work! LGTM

Checking in. Thanks!

[Yuwei, 2017-05-04 19:00:37]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-04 19:01:18]

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-04 19:08:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493924437980460,
"parent_rev": "475352fde5d62050fa44d51e4cb2c740c9b6bc69", "commit_rev":
"a4d96d0d8bdcc688451ad0376a7304df5b41904a"}

[commit-bot: I haz the power, 2017-05-04 19:09:02]

Description was changed from

==========
[Remoting iOS] Basic viewport manipulation support

This CL adds some basic zoom and pan support (touch input mode only for now)
for the iOS client.

This is minimum code to make use of DesktopViewport. The client gesture stuff
may still need more refactoring or redesign to support trackpad input mode,
deceleration animation, etc.

BUG=714193
==========

to

==========
[Remoting iOS] Basic viewport manipulation support

This CL adds some basic zoom and pan support (touch input mode only for now)
for the iOS client.

This is minimum code to make use of DesktopViewport. The client gesture stuff
may still need more refactoring or redesign to support trackpad input mode,
deceleration animation, etc.

BUG=714193

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

[commit-bot: I haz the power, 2017-05-04 19:09:03]

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