Obfuscated Gaia ID fetcher

chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h

+// Copyright (c) 2012 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.
+
+// This file has code to fetch the Obfuscated GAIA ID from the server
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_OBFUSCATED_GAIA_ID_FETCHER_H_
+#define CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_OBFUSCATED_GAIA_ID_FETCHER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/string16.h"
+#include "chrome/common/net/gaia/oauth2_api_call_flow.h"
+#include "chrome/browser/extensions/api/push_messaging/push_messaging_api.h"

[Munjal (Google), 2012/08/15 23:04:19]

Include ordering is wrong.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+
+class GoogleServiceAuthError;
+class ObfuscatedGaiaIdFetchTest;

[dcheng, 2012/08/16 19:12:43]

This forward declare is unnecessary.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+
+namespace base {
+class DictionaryValue;
+}
+
+namespace content {
+class URLFetcher;
+}
+
+namespace net {
+class URLRequestContextGetter;
+}
+
+namespace extensions {
+
+// Ideally we would use an interface instead of hardcoding to the function,
+// but we need to use a scoped_refptr to manager refcounts, and we
+// get an error if we try to put AddRef and Release into an interface,
+// since they are not virtual.
+class PushMessagingGetChannelIdFunction;

[Munjal (Google), 2012/08/15 23:04:19]

I am not sure I understand the situation here. If we define a delegate
interface, and make PushMessagingGetChannelIdFunction implement that delegate,
why doesn't it work? We should not try to make the Delegate class ref counted.

[Pete Williamson, 2012/08/20 18:38:42]

Done.

+
+// Fetches obfuscated gaia id of the Google Account that is logged in to Chrome.
+// This starts an asynchronous operation which reports success to the delegate.
+// Call "Start()" to start the async fetch.
+class ObfuscatedGaiaIdFetcher : public OAuth2ApiCallFlow {
+

[Munjal (Google), 2012/08/15 23:04:19]

Nit: remove this empty line.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+ public:
+

[Munjal (Google), 2012/08/15 23:04:19]

Nit: remove this empty line.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+  ObfuscatedGaiaIdFetcher(net::URLRequestContextGetter* context,
+                          extensions::PushMessagingGetChannelIdFunction*
+                              delegate,

[Munjal (Google), 2012/08/15 23:04:19]

We should try our best to remove this dependency. The fetcher class should not
need to know about the extension function that uses it. In other words, any
class should be able to use  this fetcher by becoming a delegate, not just
extension function.

[Pete Williamson, 2012/08/20 18:38:42]

Done.

+                          const std::string& refresh_token);
+  virtual ~ObfuscatedGaiaIdFetcher();
+

[Munjal (Google), 2012/08/15 23:04:19]

We need a Start() method. It is not a good idea to start fetching in
constructor.

[Pete Williamson, 2012/08/19 22:05:14]

We do have a start method, it is inherited from OAuth2ApiCallFlow

+ protected:
+  // Implementation of template methods in OAuth2ApiCallFlow.

[dcheng, 2012/08/16 19:12:43]

Nit: These aren't templated, so "template methods" is a little inaccurate.
Normally I'd just say "// OAuth2ApiCallFlow implementation." I'd also group all
the methods from the same interface together with no new lines as munjal has
suggested.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+  virtual GURL CreateApiCallUrl() OVERRIDE;
+  virtual std::string CreateApiCallBody() OVERRIDE;
+

[Munjal (Google), 2012/08/15 23:04:19]

Nit: remove this blank line since the next few methods are also template methods
in OAuth2ApiCallFlow

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+  virtual void ProcessApiCallSuccess(
+      const net::URLFetcher* source) OVERRIDE;
+  virtual void ProcessApiCallFailure(
+      const net::URLFetcher* source) OVERRIDE;
+  virtual void ProcessNewAccessToken(const std::string& access_token) OVERRIDE;
+  virtual void ProcessMintAccessTokenFailure(
+      const GoogleServiceAuthError& error) OVERRIDE;
+
+ private:
+  friend class ObfuscatedGaiaIdFetcherTest;

[dcheng, 2012/08/16 19:12:43]

This line is unnecessary.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+  FRIEND_TEST_ALL_PREFIXES(ObfuscatedGaiaIdFetcherTest, SetUp);
+  FRIEND_TEST_ALL_PREFIXES(ObfuscatedGaiaIdFetcherTest, ParseResponse);
+  FRIEND_TEST_ALL_PREFIXES(ObfuscatedGaiaIdFetcherTest, ProcessApiCallSuccess);
+  FRIEND_TEST_ALL_PREFIXES(ObfuscatedGaiaIdFetcherTest, ProcessApiCallFailure);
+
+  void ReportSuccess(const std::string& obfuscated_id);
+  void ReportFailure(const GoogleServiceAuthError& error);
+
+  static std::vector<std::string> Scopes();

[dcheng, 2012/08/16 19:12:43]

This method name should probably have a verb in it, if you choose to keep it. I
would prefer to pass in scopes as an argument though, since passing it in this
way looks clunky (in my opinion).

[Pete Williamson, 2012/08/20 18:38:42]

Done.

+
+  // Get the obfuscated GAIA ID out of the response body.
+  static bool ParseResponse(
+      const std::string& data, std::string* obfuscated_id);
+
+  scoped_refptr<extensions::PushMessagingGetChannelIdFunction> delegate_;
+  std::string obfuscated_gaia_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(ObfuscatedGaiaIdFetcher);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_PUSH_MESSAGING_OBFUSCATED_GAIA_ID_FETCHER_H_