Updating Pin Entry View and creating Connection Status View.

Updating Pin Entry View and creating Connection Status View.

This CL connects the PIN entry view to the connection status view. It also adds a layer in the UI for connecting to the host, instead of going strait to the GLK view, we first end up on the new connection status view that will guide and inform the user about the state of their connection, prompting for PIN (or third party auth in time?). After a successful connection has been made the view is replaced with the HostViewController with the desktop rendering, etc.

Review-Url: https://codereview.chromium.org/2846603002
Cr-Commit-Position: refs/heads/master@{#468975}
Committed: https://chromium.googlesource.com/chromium/src/+/478229abf628ef29be1e14518ac991654e256605

[nicholss, 2017-05-01 16:58:37]

Description was changed from

==========
Updating Pin Entry View and creating Connection Status View.
==========

to

==========
Updating Pin Entry View and creating Connection Status View.

This CL connects the PIN entry view to the connection status view. It also adds
a layer in the UI for connecting to the host, instead of going strait to the GLK
view, we first end up on the new connection status view that will guide and
inform the user about the state of their connection, prompting for PIN (or third
party auth in time?). After a successful connection has been made the view is
replaced with the HostViewController with the desktop rendering, etc.
==========

[nicholss, 2017-05-01 16:58:37]

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

[nicholss, 2017-05-01 16:59:03]

PTAL thanks!

[Yuwei, 2017-05-01 23:01:16]

Please wrap the summary to at most 80 characters per line.

You may be able to get rid of unnecessary dependencies by rebasing with the
master :)

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:53:
[[UIBarButtonItem alloc] initWithTitle:@"CANCEL"
Is this a placeholder before we do i18n? Or is this a magic string that makes
iOS feed localized string for the word "cancel"?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:78: _navBar.frame =
CGRectMake(0.f, 0.f, self.view.frame.size.width, kBarHeight);
Shall these be placed in layoutSubviews? What will happen if you rotate the
screen?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:99:
_iconView.center =
Positioning code should be in layoutSubviews?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:121:
CGRectMake(kMargin, _activityIndicator.center.y + kIconRadius + kPadding,
ditto

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:126:
(self.view.frame.size.width - kPinEntryViewWidth) / 2.f,
ditto

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:202: _state =
state;
You might want to return here if the delegate (_remoteHostName) is not set yet,
otherwise you might see a flickering with the text "Connecting to NULL".

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:216: -
(void)setDelegate:(id<ClientConnectionViewControllerDelegate>)delegate {
Is this method not being used yet?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:265: //
TODO(nicholss): send event.
Isn't the code below already sending event?

Also could you add TODO for createPairing?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/host_view_controller.mm:12: #import
"remoting/client/ios/display/gl_display_handler.h"
What is this line for?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/remoting_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/remoting_view_controller.mm:180: [self
presentViewController:clientConnectionViewController
I'm not familiar with iOS' view navigation yet. When calling
presentViewController, will it just creates a new view on top of RemotingView
and RemotingView will always be alive?

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/do...
File remoting/client/ios/domain/client_session_details.h (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/do...
remoting/client/ios/domain/client_session_details.h:29: @property(nonatomic)
HostInfo* hostInfo;
Looks like HostInfo is a C++ struct so the property here seems to be just a raw
pointer?

Say if RemotingView gets replaced by the HostView (although for now it looks
like one stacking on top of another..) then it may cause segfault...

Since it's just a one-off copying so I think it's okay to just store HostInfo
rather than HostInfo*?

[nicholss, 2017-05-02 16:39:40]

Thanks for the comments and questions. I understand Obj-C is a bit strange and
feel free to keep asking questions.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:53:
[[UIBarButtonItem alloc] initWithTitle:@"CANCEL"
On 2017/05/01 23:01:15, Yuwei wrote:
> Is this a placeholder before we do i18n? Or is this a magic string that makes
> iOS feed localized string for the word "cancel"?

it is placeholder. There will have to be a i18n sweep later

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:78: _navBar.frame =
CGRectMake(0.f, 0.f, self.view.frame.size.width, kBarHeight);
On 2017/05/01 23:01:15, Yuwei wrote:
> Shall these be placed in layoutSubviews? What will happen if you rotate the
> screen?

Keeping me honest. thanks.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:99:
_iconView.center =
On 2017/05/01 23:01:15, Yuwei wrote:
> Positioning code should be in layoutSubviews?

Done.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:121:
CGRectMake(kMargin, _activityIndicator.center.y + kIconRadius + kPadding,
On 2017/05/01 23:01:15, Yuwei wrote:
> ditto

Done.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:126:
(self.view.frame.size.width - kPinEntryViewWidth) / 2.f,
On 2017/05/01 23:01:15, Yuwei wrote:
> ditto

Done.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:202: _state =
state;
On 2017/05/01 23:01:15, Yuwei wrote:
> You might want to return here if the delegate (_remoteHostName) is not set
yet,
> otherwise you might see a flickering with the text "Connecting to NULL".

I set a default of "" for _remoteHostName.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:216: -
(void)setDelegate:(id<ClientConnectionViewControllerDelegate>)delegate {
On 2017/05/01 23:01:15, Yuwei wrote:
> Is this method not being used yet?

This is a magic part of obj-c, in the calling class there is something like
clientConnectionViewControler.delegate = self, this wires into the @property
defined in the interface.

To override the @property auto-generated methods you can define them in the
class. For example, for @property foo, the get method is just -(type)foo; and
the set is -(void)setFoo:(type)foo.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:265: //
TODO(nicholss): send event.
On 2017/05/01 23:01:15, Yuwei wrote:
> Isn't the code below already sending event?
> 
> Also could you add TODO for createPairing?

Done.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/host_view_controller.mm:12: #import
"remoting/client/ios/display/gl_display_handler.h"
On 2017/05/01 23:01:15, Yuwei wrote:
> What is this line for?

Line 68 uses the display handler. In this CL I also removed the include for
gl_display_handler and used a forward declare. I am trying to clean that all up
now that I understand how to do it. So part of that work is including classes
where they are used in the m files.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/remoting_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/remoting_view_controller.mm:180: [self
presentViewController:clientConnectionViewController
On 2017/05/01 23:01:15, Yuwei wrote:
> I'm not familiar with iOS' view navigation yet. When calling
> presentViewController, will it just creates a new view on top of RemotingView
> and RemotingView will always be alive?

There are a few ways to present a view, (vs navigating to it). To present means
to pop over and start with a fresh nav controller stack. So pressing back would
mean to dismiss rather than shift over a screen.

The view will be presented on-top of the RemovingView, and RemovingView will
have it's state saved. So you can be some number of navigation in the view if it
had those and it would come back to the same view you left.

The catch is there can only be one presenting view controller. So I have to
dismiss the client connection view and present the remoting host view. The
animations need to be edited to make it not look derpy but that is for next
milestone.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/do...
File remoting/client/ios/domain/client_session_details.h (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/do...
remoting/client/ios/domain/client_session_details.h:29: @property(nonatomic)
HostInfo* hostInfo;
On 2017/05/01 23:01:15, Yuwei wrote:
> Looks like HostInfo is a C++ struct so the property here seems to be just a
raw
> pointer?
> 
> Say if RemotingView gets replaced by the HostView (although for now it looks
> like one stacking on top of another..) then it may cause segfault...
> 
> Since it's just a one-off copying so I think it's okay to just store HostInfo
> rather than HostInfo*?

No, HostInfo is a class that just has some data. See line 10, it is defined in
/domain/host_info

The object is managed by ARC so cleanup will happen when all objects that have a
hold on it let go.

[nicholss, 2017-05-02 17:12:18]

Added a note about viewWillLayoutSubviews.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:78: _navBar.frame =
CGRectMake(0.f, 0.f, self.view.frame.size.width, kBarHeight);
On 2017/05/02 16:39:40, nicholss wrote:
> On 2017/05/01 23:01:15, Yuwei wrote:
> > Shall these be placed in layoutSubviews? What will happen if you rotate the
> > screen?
> 
> Keeping me honest. thanks.

An update, only a view has layoutSubviews, a view controller uses
viewWillLayoutSubviews and viewDidLayoutSubviews

[Yuwei, 2017-05-02 18:24:44]

LGTM

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:78: _navBar.frame =
CGRectMake(0.f, 0.f, self.view.frame.size.width, kBarHeight);
On 2017/05/02 17:12:18, nicholss wrote:
> On 2017/05/02 16:39:40, nicholss wrote:
> > On 2017/05/01 23:01:15, Yuwei wrote:
> > > Shall these be placed in layoutSubviews? What will happen if you rotate
the
> > > screen?
> > 
> > Keeping me honest. thanks.
> 
> An update, only a view has layoutSubviews, a view controller uses
> viewWillLayoutSubviews and viewDidLayoutSubviews

Yes. I figured it out when working with the GLKView :)

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/client_connection_view_controller.mm:216: -
(void)setDelegate:(id<ClientConnectionViewControllerDelegate>)delegate {
On 2017/05/02 16:39:39, nicholss wrote:
> On 2017/05/01 23:01:15, Yuwei wrote:
> > Is this method not being used yet?
> 
> This is a magic part of obj-c, in the calling class there is something like
> clientConnectionViewControler.delegate = self, this wires into the @property
> defined in the interface.
> 
> To override the @property auto-generated methods you can define them in the
> class. For example, for @property foo, the get method is just -(type)foo; and
> the set is -(void)setFoo:(type)foo.

Acknowledged.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/host_view_controller.mm:12: #import
"remoting/client/ios/display/gl_display_handler.h"
On 2017/05/02 16:39:40, nicholss wrote:
> On 2017/05/01 23:01:15, Yuwei wrote:
> > What is this line for?
> 
> Line 68 uses the display handler. In this CL I also removed the include for
> gl_display_handler and used a forward declare. I am trying to clean that all
up
> now that I understand how to do it. So part of that work is including classes
> where they are used in the m files.

Acknowledged.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
File remoting/client/ios/app/remoting_view_controller.mm (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/ap...
remoting/client/ios/app/remoting_view_controller.mm:180: [self
presentViewController:clientConnectionViewController
On 2017/05/02 16:39:40, nicholss wrote:
> On 2017/05/01 23:01:15, Yuwei wrote:
> > I'm not familiar with iOS' view navigation yet. When calling
> > presentViewController, will it just creates a new view on top of
RemotingView
> > and RemotingView will always be alive?
> 
> There are a few ways to present a view, (vs navigating to it). To present
means
> to pop over and start with a fresh nav controller stack. So pressing back
would
> mean to dismiss rather than shift over a screen.
> 
> The view will be presented on-top of the RemovingView, and RemovingView will
> have it's state saved. So you can be some number of navigation in the view if
it
> had those and it would come back to the same view you left.
> 
> The catch is there can only be one presenting view controller. So I have to
> dismiss the client connection view and present the remoting host view. The
> animations need to be edited to make it not look derpy but that is for next
> milestone.

Acknowledged.

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/do...
File remoting/client/ios/domain/client_session_details.h (right):

https://codereview.chromium.org/2846603002/diff/120001/remoting/client/ios/do...
remoting/client/ios/domain/client_session_details.h:29: @property(nonatomic)
HostInfo* hostInfo;
On 2017/05/02 16:39:40, nicholss wrote:
> On 2017/05/01 23:01:15, Yuwei wrote:
> > Looks like HostInfo is a C++ struct so the property here seems to be just a
> raw
> > pointer?
> > 
> > Say if RemotingView gets replaced by the HostView (although for now it looks
> > like one stacking on top of another..) then it may cause segfault...
> > 
> > Since it's just a one-off copying so I think it's okay to just store
HostInfo
> > rather than HostInfo*?
> 
> No, HostInfo is a class that just has some data. See line 10, it is defined in
> /domain/host_info
> 
> The object is managed by ARC so cleanup will happen when all objects that have
a
> hold on it let go.

Gotcha. Then that's fine. I think I got confused by ios/facade/host_info.h O_o

[Jamie, 2017-05-02 23:44:09]

Rubber-stamp LGTM since OWNERS is pending for Yuwei anyway.

[nicholss, 2017-05-02 23:46:24]

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

[commit-bot: I haz the power, 2017-05-02 23:47:04]

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-03 01:51:17]

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

[commit-bot: I haz the power, 2017-05-03 01:51:18]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-05-03 15:07:44]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-05-03 15:08:06]

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-03 15:12:07]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493824064625780,
"parent_rev": "b1c9a6c795742460074554b098b85570a7fb0443", "commit_rev":
"478229abf628ef29be1e14518ac991654e256605"}

[commit-bot: I haz the power, 2017-05-03 15:12:15]

Description was changed from

==========
Updating Pin Entry View and creating Connection Status View.

This CL connects the PIN entry view to the connection status view. It also adds
a layer in the UI for connecting to the host, instead of going strait to the GLK
view, we first end up on the new connection status view that will guide and
inform the user about the state of their connection, prompting for PIN (or third
party auth in time?). After a successful connection has been made the view is
replaced with the HostViewController with the desktop rendering, etc.
==========

to

==========
Updating Pin Entry View and creating Connection Status View.

This CL connects the PIN entry view to the connection status view. It also adds
a layer in the UI for connecting to the host, instead of going strait to the GLK
view, we first end up on the new connection status view that will guide and
inform the user about the state of their connection, prompting for PIN (or third
party auth in time?). After a successful connection has been made the view is
replaced with the HostViewController with the desktop rendering, etc.

Review-Url: https://codereview.chromium.org/2846603002
Cr-Commit-Position: refs/heads/master@{#468975}
Committed:
https://chromium.googlesource.com/chromium/src/+/478229abf628ef29be1e14518ac9...
==========

[commit-bot: I haz the power, 2017-05-03 15:12:16]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/478229abf628ef29be1e14518ac9...