CRD iOS: Updating remoting service to use oauth and host list fetcher.

remoting/client/ios/facade/remoting_service.h

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef REMOTING_CLIENT_IOS_FACADE_REMOTING_SERCICE_H_

[joedow, 2017/04/05 22:30:55]

Can you fix this header guard (SERCICE is a typo):
REMOTING_CLIENT_IOS_FACADE_REMOTING_SERVICE_H_

[nicholss, 2017/04/07 18:16:15]

Done.

 #define REMOTING_CLIENT_IOS_FACADE_REMOTING_SERCICE_H_
 
+#include "base/memory/weak_ptr.h"
+#import "remoting/client/chromoting_client_runtime.h"
 #import "remoting/client/ios/domain/host_info.h"
 #import "remoting/client/ios/domain/user_info.h"
 
+// |RemotingAuthenticationDelegate|'s are interesed in authentication related

[joedow, 2017/04/05 22:30:55]

nit: remove apostrophe (non-possessive) and fix interesed (interested)

[nicholss, 2017/04/07 18:16:15]

Done.

+// notifications.
+@protocol RemotingAuthenticationDelegate<NSObject>
+
+// Notifies the delegate that the authentication status of the current user has
+// changed to a new state.
+- (void)nowAuthenticated:(BOOL)authenticated;
+
+@end
+
+// |RemotingHostListDelegate|'s are interested in notifications related to host

[joedow, 2017/04/05 22:30:55]

remove apostrophe

[nicholss, 2017/04/07 18:16:15]

Done.

+// list.
+@protocol RemotingHostListDelegate<NSObject>
+
+- (void)hostListUpdated;
+
+@end
+
+// |RemotingService| is the centralized place to ask for information about
+// authentication or query the remote services. It also helps deal with the
+// runtime and threading used in the application. |RemotingService| is a
+// singleton and should only be accessed via the |SharedInstance| method.
 @interface RemotingService : NSObject
 
-+ (RemotingService*)sharedInstance;
+// Access to the singleton shared instance from this method.
++ (RemotingService*)SharedInstance;
 
+// Access to the current |ChromotingClientRuntime| from this method.
+- (remoting::ChromotingClientRuntime*)runtime;
+
+// Register to be a |RemotingAuthenticationDelegate|.

[joedow, 2017/04/05 22:30:55]

Does the caller need to unregister at some point?  If that is a common  iOS
pattern then feel free to ignore otherwise a comment would be good. 

[nicholss, 2017/04/07 18:16:15]

No need to unregister, it is a pretty common iOS pattern to set a delegate and
never remove it. 

+- (void)setAuthenticationDelegate:(id<RemotingAuthenticationDelegate>)delegate;
+
+// A cached answer if there is a currently authenticated user.

[joedow, 2017/04/05 22:30:55]

Isn't this just the current authentication state (i.e. no authenticated user
returns false otherwise true)?  I think I'm getting thrown off by calling it the
cached answer instead of the current state.

[nicholss, 2017/04/07 18:16:15]

I said cache because it is not validating that the current auth token is still
valid. The user could have an expired token or something for example. I did not
want to imply that work would happen when you call this method.

 - (BOOL)isAuthenticated;
+
+// Provide an |authorizationCode| to authenticate a user as the first time user
+// of the applicaiton or OAuth Flow.

[joedow, 2017/04/05 22:30:55]

typo: applicaiton -> application

[nicholss, 2017/04/07 18:16:15]

Done.

 - (void)authenticateWithAuthorizationCode:(NSString*)authorizationCode;
+
+// Provide the |refreshToken| and |email| to authenticate a user as a returning
+// user of the application.
+- (void)authenticateWithRefreshToken:(NSString*)refreshToken
+                                emai:(NSString*)email;

[joedow, 2017/04/05 22:30:55]

typo: emai -> rename to email

[nicholss, 2017/04/07 18:16:15]

HA! I had not gotten to the point where I can use refresh tokens so I did not
catch this. :)

+
+// Returns the currently logged in user info from cache, or nil if no
+// currently authenticated user.
 - (UserInfo*)getUser;
+
+// Register to be a |RemotingHostListDelegate|. Side effect of setting this
+// delegate is the application will attempt to fetch a fresh host list.

[joedow, 2017/04/05 22:30:55]

Just curious, why is the host list fetched again?  If the host list is already
fresh (for some definition of fresh), you could just post a task which will call
the delegate with the current host list.  It's hard for me to tell if that would
be better without seeing the usage of this class.

[nicholss, 2017/04/07 18:16:15]

In reality there is only one object that will ever become the host list delegate
and this will happen once per application cycle, so there is not a host list yet
and setting this will cause the first time fetch of the host list and results in
the delegate getting called back a short time later with the host list.

It might be cleaner to set the delegate and then have them ask to refresh the
host list. I will keep this in-mind when I am working on keeping the host list
refreshed and that could rework this flow.

[joedow, 2017/04/10 16:09:59]

SGTM

+- (void)setHostListDelegate:(id<RemotingHostListDelegate>)delegate;
+
+// Returns the currently cached host list or nil if none exist.
 - (NSArray<HostInfo*>*)getHosts;
 
 @end
 
 #endif  // REMOTING_CLIENT_IOS_FACADE_REMOTING_SERCICE_H_