Adding error handling to the connection flow.

Adding error handling to the connection flow.

Adding a reconnect button and generic error display for reconnection flow.
Adding some logic around enabling the connect button based on pin length.
Adding a red circle to the error state view in connection flow.

Review-Url: https://codereview.chromium.org/2971903002
Cr-Commit-Position: refs/heads/master@{#484967}
Committed: https://chromium.googlesource.com/chromium/src/+/f5407a1825ca1557eed0f65a2aedb73d7aaec60e

[nicholss, 2017-07-05 20:58:02]

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

[nicholss, 2017-07-05 20:58:03]

PTAL

[Yuwei, 2017-07-05 21:43:42]

Thanks for fixing the sad path :) Just a few comments

https://codereview.chromium.org/2971903002/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_session.cc (right):

https://codereview.chromium.org/2971903002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_session.cc:281: remoting::SignalStrategy::Error err =
signaling_->GetError();
Where is err used?

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

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:163: _iconView.frame =
CGRectMake(0, 0, kIconRadius * 2, kIconRadius * 2.f);
FYI I think all these layout code need to be changed to constraint or something
at some point since currently the RTL layout is broken...

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:173:
_statusLabel.font.pointSize * _statusLabel.numberOfLines * 1.1);
What is this 1.1 doing?

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:274: [self
dismissViewControllerAnimated:YES completion:nil];
Since you are here could you change this to [self.navigationController
popViewControllerAnimated:YES]?

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
File remoting/ios/app/session_error_view.h (right):

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
remoting/ios/app/session_error_view.h:10: @protocol
SessionErrorViewDelegate<NSObject>
The name seems a bit confusing if what it does is just showing the reconnect
button. X_X If later we want to add the error text here then I think the name
will be fine...

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
File remoting/ios/app/session_error_view.mm (right):

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
remoting/ios/app/session_error_view.mm:14: // static const CGFloat kMargin =
5.f;
What are these commented numbers for?

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
remoting/ios/app/session_error_view.mm:50: CGRectMake((self.frame.size.width -
kReconnectButtonWidth) / 2.f, 0,
Will it be simpler to do self.center.x - kReconnectButtonWidth / 2.f? You can
also use the centerXAnchor if you like

[nicholss, 2017-07-06 20:49:02]

PTAL, I used a combo of VFL and anchors.

https://codereview.chromium.org/2971903002/diff/20001/remoting/client/chromot...
File remoting/client/chromoting_session.cc (right):

https://codereview.chromium.org/2971903002/diff/20001/remoting/client/chromot...
remoting/client/chromoting_session.cc:281: remoting::SignalStrategy::Error err =
signaling_->GetError();
On 2017/07/05 21:43:41, Yuwei wrote:
> Where is err used?

Sorry, this was debug, I had already removed it in patch 3.

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

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:163: _iconView.frame =
CGRectMake(0, 0, kIconRadius * 2, kIconRadius * 2.f);
On 2017/07/05 21:43:42, Yuwei wrote:
> FYI I think all these layout code need to be changed to constraint or
something
> at some point since currently the RTL layout is broken...

Fixed up the main view. Need to do the pin entry view still.

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:173:
_statusLabel.font.pointSize * _statusLabel.numberOfLines * 1.1);
On 2017/07/05 21:43:42, Yuwei wrote:
> What is this 1.1 doing?

The bottom of the g was getting cut off, I  guess point size does not include
the lower sections for g, q, j, p and y.

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:274: [self
dismissViewControllerAnimated:YES completion:nil];
On 2017/07/05 21:43:42, Yuwei wrote:
> Since you are here could you change this to [self.navigationController
> popViewControllerAnimated:YES]?

Done.

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
File remoting/ios/app/session_error_view.h (right):

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
remoting/ios/app/session_error_view.h:10: @protocol
SessionErrorViewDelegate<NSObject>
On 2017/07/05 21:43:42, Yuwei wrote:
> The name seems a bit confusing if what it does is just showing the reconnect
> button. X_X If later we want to add the error text here then I think the name
> will be fine...

I think I will rename it to SessonReconnectView and add an error message
component if needed later.

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
File remoting/ios/app/session_error_view.mm (right):

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
remoting/ios/app/session_error_view.mm:14: // static const CGFloat kMargin =
5.f;
On 2017/07/05 21:43:42, Yuwei wrote:
> What are these commented numbers for?

gone.

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/sessio...
remoting/ios/app/session_error_view.mm:50: CGRectMake((self.frame.size.width -
kReconnectButtonWidth) / 2.f, 0,
On 2017/07/05 21:43:42, Yuwei wrote:
> Will it be simpler to do self.center.x - kReconnectButtonWidth / 2.f? You can
> also use the centerXAnchor if you like

Used autolayout

[Yuwei, 2017-07-06 21:52:39]

LGTM once comments are resolved

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

https://codereview.chromium.org/2971903002/diff/20001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:173:
_statusLabel.font.pointSize * _statusLabel.numberOfLines * 1.1);
On 2017/07/06 20:49:01, nicholss wrote:
> On 2017/07/05 21:43:42, Yuwei wrote:
> > What is this 1.1 doing?
> 
> The bottom of the g was getting cut off, I  guess point size does not include
> the lower sections for g, q, j, p and y.

I think point is a unit different than pixels. UIFont.lineHeight will return the
height of a line, but that's still in points. I'm not quite sure how you could
convert pt into pixels though X_X

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
File remoting/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:180: [self.view
addConstraints:
The doc says [NSLayoutConstraint activateConstraints:] is preferred when
developing for iOS 8.0+

https://developer.apple.com/documentation/uikit/uiview/1622513-addconstraints...

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:260: [self.view
setNeedsUpdateConstraints];
You probably don't need this if you use activateConstraints?

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/sessio...
File remoting/ios/app/session_reconnect_view.mm (right):

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/sessio...
remoting/ios/app/session_reconnect_view.mm:58: [self
addConstraints:[NSLayoutConstraint
For simple constraints like this I would probably do
[_reconnectButton.widthAnchor constraintEqualToConstant:kButtonWidth]

[nicholss, 2017-07-06 22:15:06]

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
File remoting/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:180: [self.view
addConstraints:
On 2017/07/06 21:52:39, Yuwei wrote:
> The doc says [NSLayoutConstraint activateConstraints:] is preferred when
> developing for iOS 8.0+
> 
>
https://developer.apple.com/documentation/uikit/uiview/1622513-addconstraints...

arg. :(

It looks like people are all over the place on how to use this. Lets add a todo
in the tracking doc to look at how this should be.

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:260: [self.view
setNeedsUpdateConstraints];
On 2017/07/06 21:52:39, Yuwei wrote:
> You probably don't need this if you use activateConstraints?

no that is for bulk activation of an array of constraints.
https://developer.apple.com/documentation/uikit/nslayoutconstraint/1526955-ac...

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:260: [self.view
setNeedsUpdateConstraints];
On 2017/07/06 21:52:39, Yuwei wrote:
> You probably don't need this if you use activateConstraints?

I tried it out and you still need to call [self setNeedsUpdateConstraints];

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/sessio...
File remoting/ios/app/session_reconnect_view.mm (right):

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/sessio...
remoting/ios/app/session_reconnect_view.mm:58: [self
addConstraints:[NSLayoutConstraint
On 2017/07/06 21:52:39, Yuwei wrote:
> For simple constraints like this I would probably do
> [_reconnectButton.widthAnchor constraintEqualToConstant:kButtonWidth]

Done.

[Yuwei, 2017-07-06 22:36:28]

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
File remoting/ios/app/client_connection_view_controller.mm (right):

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:260: [self.view
setNeedsUpdateConstraints];
On 2017/07/06 22:15:06, nicholss wrote:
> On 2017/07/06 21:52:39, Yuwei wrote:
> > You probably don't need this if you use activateConstraints?
> 
> no that is for bulk activation of an array of constraints.
>
https://developer.apple.com/documentation/uikit/nslayoutconstraint/1526955-ac...

Okay... Sad...

I don't remember I have ever used setNeedsUpdateConstraints though...

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/sessio...
File remoting/ios/app/session_reconnect_view.mm (right):

https://codereview.chromium.org/2971903002/diff/80001/remoting/ios/app/sessio...
remoting/ios/app/session_reconnect_view.mm:58: [self
addConstraints:[NSLayoutConstraint
On 2017/07/06 22:15:06, nicholss wrote:
> On 2017/07/06 21:52:39, Yuwei wrote:
> > For simple constraints like this I would probably do
> > [_reconnectButton.widthAnchor constraintEqualToConstant:kButtonWidth]
> 
> Done.

FYI you could simplify these with array literal if you like

[nicholss, 2017-07-06 22:54:52]

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

[commit-bot: I haz the power, 2017-07-06 22:54:56]

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-07-06 23:01:03]

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

[commit-bot: I haz the power, 2017-07-06 23:01:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nicholss, 2017-07-07 16:03:37]

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

[commit-bot: I haz the power, 2017-07-07 16:03:51]

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-07-07 17:30:30]

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

[commit-bot: I haz the power, 2017-07-07 17:30:31]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-07-07 17:33:02]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-07-07 17:33:02]

The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org
Link to the patchset: https://codereview.chromium.org/2971903002/#ps100001
(title: "Trying out activateConstraints.")

[commit-bot: I haz the power, 2017-07-07 17:33:19]

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-07-07 17:38:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499448782496580,
"parent_rev": "c04dbb714b62253db69c04d7671b2c58af1fccf9", "commit_rev":
"f5407a1825ca1557eed0f65a2aedb73d7aaec60e"}

[commit-bot: I haz the power, 2017-07-07 17:38:39]

Description was changed from

==========
Adding error handling to the connection flow.

Adding a reconnect button and generic error display for reconnection flow.
Adding some logic around enabling the connect button based on pin length.
Adding a red circle to the error state view in connection flow.
==========

to

==========
Adding error handling to the connection flow.

Adding a reconnect button and generic error display for reconnection flow.
Adding some logic around enabling the connect button based on pin length.
Adding a red circle to the error state view in connection flow.

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

[commit-bot: I haz the power, 2017-07-07 17:38:44]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f5407a1825ca1557eed0f65a2aed...