Obfuscated Gaia ID fetcher

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

+// 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.
+
+#include "chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h"
+
+#include <string>
+#include <vector>
+
+#include "base/json/json_reader.h"
+#include "base/values.h"
+#include "chrome/common/net/gaia/google_service_auth_error.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_status.h"
+
+using net::URLFetcher;
+using net::URLRequestContextGetter;
+using net::URLRequestStatus;
+
+namespace {
+
+const char kID[] = "id";

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

static.

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

This name is pretty undescriptive for a top-level constant. You can rename it...
or since this constant is only used in one place, you can just move it into the
function where it's used (I would probably do the latter here).

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

Done.

+
+}
+
+namespace {

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

Combine this anonymous namespace with the previous one.

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

Done.

+
+// URL of the service to get obfuscated GAIA ID (here misnamed channel ID).
+static const char kCWSChannelServiceURL[] =
+    "https://www.googleapis.com/chromewebstore/v1.1/channels/id";
+
+static GoogleServiceAuthError CreateAuthError(URLRequestStatus status) {
+  if (status.status() == URLRequestStatus::CANCELED) {
+    return GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED);
+  } else {
+    // TODO(munjal): Improve error handling. Currently we return connection
+    // error for even application level errors. We need to either expand the
+    // GoogleServiceAuthError enum or create a new one to report better
+    // errors.
+    DLOG(WARNING) << "Server returned error: errno " << status.error();
+    return GoogleServiceAuthError::FromConnectionError(status.error());
+  }
+}
+
+}  // namespace
+
+
+namespace extensions {
+
+ObfuscatedGaiaIdFetcher::ObfuscatedGaiaIdFetcher(
+    URLRequestContextGetter* context,
+    PushMessagingGetChannelIdFunction* delegate,
+    const std::string& refresh_token)
+    : OAuth2ApiCallFlow(context, refresh_token, "", Scopes()),

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

std::string() instead of "".

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

I'm not a fan of Scopes(), but I guess there's no great solution here since we
always pass in the same thing.

Scopes() shouldn't be a member function though.

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

Done.

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

Munjal suggested that it be a member function because it is an important
implementation detail of the class, and I agree.  I'm happy to change it to be a
non-member function if you and Munjal both prefer.

[dcheng, 2012/08/19 22:38:35]

It is an implementation detail. In fact, it's so much of one that it can live
entirely in the .cc =)

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

Done.

+      delegate_(delegate) {
+}
+
+ObfuscatedGaiaIdFetcher::~ObfuscatedGaiaIdFetcher() { }
+
+// We use this static function to encapsulate the scopes needed at ctor time.
+std::vector<std::string> ObfuscatedGaiaIdFetcher::Scopes() {
+  std::vector<std::string> scopes;
+  if (scopes.size() == 0) {

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

Why would the size of the vector not be 0? It is a local variable.

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

Prefer empty() over size() == 0

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

It is always 0, this is left over from when it was a member variable.  Check
removed.

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

check removed instead.

+    scopes.push_back(
+        "https://www.googleapis.com/auth/chromewebstore.notification");
+  }
+  return scopes;
+}
+
+void ObfuscatedGaiaIdFetcher::ReportSuccess(const std::string& obfuscated_id) {
+  if (delegate_)

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

Why do we allow null delegates?

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

It is because we are using the delegate to hold it alive, and the delegate needs
to be a pointer instead of a ref so that it is deleteable.  We'll revisit this
when we revisit the addref strategy as a comment below suggests.

+    delegate_->OnObfuscatedGaiaIdFetchSuccess(obfuscated_id);
+}
+
+void ObfuscatedGaiaIdFetcher::ReportFailure(
+    const GoogleServiceAuthError& error) {
+  if (delegate_)
+    delegate_->OnObfuscatedGaiaIdFetchFailure(error);
+}
+
+GURL ObfuscatedGaiaIdFetcher::CreateApiCallUrl() {
+  return GURL(kCWSChannelServiceURL);
+}
+
+std::string ObfuscatedGaiaIdFetcher::CreateApiCallBody() {
+  // Nothing to do here, we don't need a body for this request, the URL
+  // encodes all the proper arguments.
+  return std::string();
+}
+
+void ObfuscatedGaiaIdFetcher::ProcessApiCallSuccess(
+    const net::URLFetcher* source) {
+  // TODO(munjal): Change error code paths in this method to report an
+  // internal error.
+  std::string response_body;
+  source->GetResponseAsString(&response_body);

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

CHECK(source->GetResponseAsString(&response_body)) perhaps?

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

Done.

+
+  std::string channel_id;
+  if (ParseResponse(response_body, &channel_id))
+    ReportSuccess(channel_id);
+  else
+    ReportFailure(GoogleServiceAuthError::FromConnectionError(101));
+
+  // Clean up ourselves, and stop holding the Function object refcount too.
+  delete this;

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

We should definitely avoid this. Let us just define a delegate interface and let
the extension function do the right thing in terms of lifetime management.

here is another "delete this;" in ProcessApiCallFailure. We should remove both.

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

The problem is this:
The extension function (logically) ought to "own" its corresponding instance of
ObfuscatedGaiaIdFetcher. However, ObfuscatedGaiaIdFetcher also needs to keep a
ref count to PushMessagingGetChannelIdFunction to keep it alive (since we reply
asynchronously). We need to break the cycle somehow or find another way to fix
the ownership model here. I've been thinking about this problem and I have not
yet come to a great solution.

[Munjal (Google), 2012/08/16 21:45:12]

The ExtensionFunction should AddRef itself instead of relying on this class to
add a reference. That is what all extension functions do.

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

Daniel explicitly requested that I not call AddRef explicitly, and that my code
should never do it.  I'm happy with either solution, I'll try to get a 5 minute
chat with both of you so we can pick the best way to solve this issue.

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

After some clarification from Daniel, I settled on the AddRef method, which is
now implemented.

+}
+
+void ObfuscatedGaiaIdFetcher::ProcessApiCallFailure(
+    const net::URLFetcher* source) {
+  ReportFailure(CreateAuthError(source->GetStatus()));
+
+  // Clean up ourselves, and stop holding the Function object refcount too.
+  delete this;
+}
+
+void ObfuscatedGaiaIdFetcher::ProcessNewAccessToken(
+    const std::string& obfuscated_id)  {
+  // We generate a new access token every time instead of storing the access
+  // token since access token expire every hour and we expect to get
+  // obfuscated gaia id very infrequently.
+}
+
+void ObfuscatedGaiaIdFetcher::ProcessMintAccessTokenFailure(
+    const GoogleServiceAuthError& error)  {
+  // We failed to generate the token needed to call the API to get
+  // the obfuscated user ID, so report failure to the caller.
+  ReportFailure(error);
+}
+
+// static
+bool ObfuscatedGaiaIdFetcher::ParseResponse(
+    const std::string& data, std::string* result) {
+

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

Nit: remove empty line.

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

Done.

+  scoped_ptr<base::Value> value(base::JSONReader::Read(data));
+  if (!value.get() || value->GetType() != base::Value::TYPE_DICTIONARY)
+    return false;
+
+  DictionaryValue* dict = static_cast<DictionaryValue*>(value.get());

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

Instead of value->GetType() != base::Value::TYPE_DICTIONARY, just use
base::Value::GetAsDictionary.

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

Done.

+  return dict->GetString(kID, result);
+}
+
+} // namespace extensions

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

Nit: two spaces after }

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

Done.