Third Party Authentication for Android Part I - TokenFetcherProxy

remoting/client/token_fetcher_proxy.h

-// Copyright 2013 The Chromium Authors. All rights reserved.
+// Copyright 2014 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_PLUGIN_PEPPER_TOKEN_FETCHER_H_
-#define REMOTING_CLIENT_PLUGIN_PEPPER_TOKEN_FETCHER_H_
+#ifndef REMOTING_PROTOCOL_TOKEN_FETCHER_PROXY_H_

[Lambros, 2014/06/04 22:20:19]

nit: s/PROTOCOL/CLIENT (same at bottom of file)

[kelvinp, 2014/06/05 23:53:14]

Done.

+#define REMOTING_PROTOCOL_TOKEN_FETCHER_PROXY_H_
 
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "remoting/protocol/third_party_client_authenticator.h"
 
 namespace remoting {
 
-class ChromotingInstance;
-
-class PepperTokenFetcher
+class TokenFetcherProxy
     : public protocol::ThirdPartyClientAuthenticator::TokenFetcher {
  public:
-  PepperTokenFetcher(base::WeakPtr<ChromotingInstance> plugin,
-                     const std::string& host_public_key);
-  virtual ~PepperTokenFetcher();
+  typedef base::Callback<void(
+      const GURL& token_url,
+      const std::string& scope,
+      base::WeakPtr<TokenFetcherProxy>)> TokenFetcherCallback;
+
+  TokenFetcherProxy(TokenFetcherCallback token_fetcher_impl,

[Sergey Ulanov, 2014/06/05 22:04:01]

nit: const reference 

[kelvinp, 2014/06/05 23:53:14]

Done.

+                    const std::string& host_public_key);

[Sergey Ulanov, 2014/06/05 22:06:18]

please mark constructor as explicit if you remove the second parameter per my
comment about |host_public_key_|.

[kelvinp, 2014/06/05 23:53:14]

Turns out |host_public_key| is needed.  See comment below.

+  virtual ~TokenFetcherProxy();
 
   // protocol::TokenClientAuthenticator::TokenFetcher interface.
   virtual void FetchThirdPartyToken(
       const GURL& token_url,
       const std::string& scope,
       const TokenFetchedCallback& token_fetched_callback) OVERRIDE;
 
-  // Called by ChromotingInstance when the webapp finishes fetching the token.
+  // Called by the token fetching implementation when the token is fetched.
   void OnTokenFetched(const std::string& token,
                       const std::string& shared_secret);
 
  private:
-  base::WeakPtr<ChromotingInstance> plugin_;
   std::string host_public_key_;

[Sergey Ulanov, 2014/06/05 22:04:01]

This is not used anywhere. Can it be removed (and the constructor argument)?

[kelvinp, 2014/06/05 23:53:14]

Good catch.  This should be used when calling to the TokenProxy

   TokenFetchedCallback token_fetched_callback_;
-  base::WeakPtrFactory<PepperTokenFetcher> weak_factory_;
+  TokenFetcherCallback token_fetcher_impl_;

[Sergey Ulanov, 2014/06/05 22:04:01]

nit: move this to top of the list, where plugin_ used to be, so that all
initialization arguments come before everything else.

[kelvinp, 2014/06/05 23:53:14]

Done.

+  base::WeakPtrFactory<TokenFetcherProxy> weak_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(PepperTokenFetcher);
+  DISALLOW_COPY_AND_ASSIGN(TokenFetcherProxy);
 };
 
 }  // namespace remoting
 
-#endif  // REMOTING_CLIENT_PLUGIN_PEPPER_TOKEN_FETCHER_H_
+#endif  // REMOTING_PROTOCOL_TOKEN_FETCHER_PROXY_H_