Fixing signed in message and disconnect/reconnect case.

This CL fixes the disconnection from an interruption during the session and lets you
reconnect. Also removes the "you are signed in" message each time you view the
host list.

Review-Url: https://codereview.chromium.org/2966243003
Cr-Commit-Position: refs/heads/master@{#485035}
Committed: https://chromium.googlesource.com/chromium/src/+/37d54e306cbf2723af12caac99dcfea8a42159e4

[nicholss, 2017-07-06 21:50:33]

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

[nicholss, 2017-07-06 21:51:52]

Description was changed from

==========
Fixed signedin as message.
==========

to

==========
This CL fixes the disconnection from an interruption during the session and lets
you reconnect. Also removes the "you are signed in" message each time you view
the host list.
==========

[nicholss, 2017-07-06 21:52:07]

Description was changed from

==========
This CL fixes the disconnection from an interruption during the session and lets
you reconnect. Also removes the "you are signed in" message each time you view
the host list.
==========

to

==========
This CL fixes the disconnection from an interruption during the session and lets
you
reconnect. Also removes the "you are signed in" message each time you view the
host list.
==========

[Yuwei, 2017-07-06 22:29:45]

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

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:455:
showMessage:[MDCSnackbarMessage messageWithText:@"Connection Closed."]];
Do we really need to show the message twice?

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
File remoting/ios/app/remoting_view_controller.mm (right):

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
remoting/ios/app/remoting_view_controller.mm:36: BOOL _showAccountSignIn;
Maybe change this to _showSignedInAccount? _showAccountSignIn sounds like it
will decide whether the account sign in flow should present...

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
remoting/ios/app/remoting_view_controller.mm:168: MDCSnackbarMessage* message =
[[MDCSnackbarMessage alloc] init];
If we already presentSignInFlow if the user hasn't signed in then I don't feel
this message is useful... WDYT?

Also I feel like we will need a dedicated "ask you to sign in" view rather than
just popping the sign in flow and force the user to sign in.

Anyway, UX concerns though...

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
remoting/ios/app/remoting_view_controller.mm:254: if (_isAuthenticated &&
_showAccountSignIn) {
I think we still want to show the message if the user switches account. You can
switch accounts with the SSO menu in internal build.

I kind of feel like it should be some singleton's responsibility (AppDelegate?)
to listen to the event and show the message. That could guarantee the message
will be shown exactly once when the user gets changed.

[nicholss, 2017-07-06 22:40:02]

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

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/client...
remoting/ios/app/client_connection_view_controller.mm:455:
showMessage:[MDCSnackbarMessage messageWithText:@"Connection Closed."]];
On 2017/07/06 22:29:45, Yuwei wrote:
> Do we really need to show the message twice?

yessss.

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
File remoting/ios/app/remoting_view_controller.mm (right):

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
remoting/ios/app/remoting_view_controller.mm:36: BOOL _showAccountSignIn;
On 2017/07/06 22:29:45, Yuwei wrote:
> Maybe change this to _showSignedInAccount? _showAccountSignIn sounds like it
> will decide whether the account sign in flow should present...

sounds fine. Done.

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
remoting/ios/app/remoting_view_controller.mm:168: MDCSnackbarMessage* message =
[[MDCSnackbarMessage alloc] init];
On 2017/07/06 22:29:45, Yuwei wrote:
> If we already presentSignInFlow if the user hasn't signed in then I don't feel
> this message is useful... WDYT?
> 
> Also I feel like we will need a dedicated "ask you to sign in" view rather
than
> just popping the sign in flow and force the user to sign in.
> 
> Anyway, UX concerns though...

Acknowledged.

https://codereview.chromium.org/2966243003/diff/40001/remoting/ios/app/remoti...
remoting/ios/app/remoting_view_controller.mm:254: if (_isAuthenticated &&
_showAccountSignIn) {
On 2017/07/06 22:29:45, Yuwei wrote:
> I think we still want to show the message if the user switches account. You
can
> switch accounts with the SSO menu in internal build.
> 
> I kind of feel like it should be some singleton's responsibility
(AppDelegate?)
> to listen to the event and show the message. That could guarantee the message
> will be shown exactly once when the user gets changed.

I think that sounds like a good refactor, pull all the account messaging out of
this class. Adding an item to the list.

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

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:40]

Dry run: CQ is trying da patch.

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

[Yuwei, 2017-07-06 22:54:41]

LGTM

[commit-bot: I haz the power, 2017-07-06 22:57:36]

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

[commit-bot: I haz the power, 2017-07-06 22:57:37]

Dry run: 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...)

[nicholss, 2017-07-07 20:16:44]

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

[commit-bot: I haz the power, 2017-07-07 20:17: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-07-07 20:28:02]

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

[commit-bot: I haz the power, 2017-07-07 20:28:03]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-07-07 20:35:00]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-07-07 20:35:00]

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/2966243003/#ps80001
(title: "Merged.")

[commit-bot: I haz the power, 2017-07-07 20:35:29]

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 20:43:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499459700073620,
"parent_rev": "b36f4aaf404427cef20af39e7b657f9afe942ae0", "commit_rev":
"37d54e306cbf2723af12caac99dcfea8a42159e4"}

[commit-bot: I haz the power, 2017-07-07 20:43:19]

Description was changed from

==========
This CL fixes the disconnection from an interruption during the session and lets
you
reconnect. Also removes the "you are signed in" message each time you view the
host list.
==========

to

==========
This CL fixes the disconnection from an interruption during the session and lets
you
reconnect. Also removes the "you are signed in" message each time you view the
host list.

Review-Url: https://codereview.chromium.org/2966243003
Cr-Commit-Position: refs/heads/master@{#485035}
Committed:
https://chromium.googlesource.com/chromium/src/+/37d54e306cbf2723af12caac99dc...
==========

[commit-bot: I haz the power, 2017-07-07 20:43:20]

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

[brucedawson, 2017-07-07 21:14:51]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2017-07-07 21:14:51]

Please make sure that your CL descriptions contain a one-line summary followed
by a blank line. The first line of the CL description is shown by "git log
--oneline" and "git branch" and if it is an entire paragraph then it makes the
output harder to read.

In this case there was an excellent title for the CL but that title was not
copied into the CL description so it did not get passed along to git when the CL
was committed.

This article gives some guidance: https://chris.beams.io/posts/git-commit/. We
are not required to follow this advice at Chrome but it offers a good start.

FWIW.