Save the oauth client id used in App Notification setup

chrome/browser/extensions/app_notify_channel_setup.cc

 // Copyright (c) 2011 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/app_notify_channel_setup.h"
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
@@ skipping 39 matching lines @@
 static const char kCWSChannelServiceURL[] =
     "https://www.googleapis.com/chromewebstore/"
     "v1.1/channels/id";
 static const char* kRelevantGaiaServices[] = {
   GaiaConstants::kCWSService,
   GaiaConstants::kLSOService,
 };
 static const int kRelevantGaiaServicesCount = arraysize(kRelevantGaiaServices);
 }  // namespace.
 
+

[Munjal (Google), 2011/11/29 21:55:34]

Nit: remove extra newline.

[asargent_no_longer_on_chrome, 2011/11/30 06:06:20]

Done.

I also added spaces around the namespace opening/closing braces above to comply
with:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...

+static AppNotifyChannelSetup::InterceptorForTests* g_interceptor = NULL;

[Munjal (Google), 2011/11/29 21:55:34]

Would it make more sense for this to be in anonymous namespace or in a static
member of the class itself?

[asargent_no_longer_on_chrome, 2011/11/30 06:06:20]

Good idea - moved into the anonymous namespace above, and renamed slightly.

+
+// static
+void AppNotifyChannelSetup::SetInterceptorForTests(
+    AppNotifyChannelSetup::InterceptorForTests* interceptor) {
+  g_interceptor = interceptor;

[Munjal (Google), 2011/11/29 21:55:34]

Nit: should we check that g_interceptor is NULL to avoid unintentional chaining?

[asargent_no_longer_on_chrome, 2011/11/30 06:06:20]

Good idea, done.

+}
+
 AppNotifyChannelSetup::AppNotifyChannelSetup(
     Profile* profile,
     const std::string& extension_id,
     const std::string& client_id,
     const GURL& requestor_url,
     int return_route_id,
     int callback_id,
     AppNotifyChannelUI* ui,
     base::WeakPtr<AppNotifyChannelSetup::Delegate> delegate)
     : profile_(profile),
       extension_id_(extension_id),
       client_id_(client_id),
       requestor_url_(requestor_url),
       return_route_id_(return_route_id),
       callback_id_(callback_id),
       delegate_(delegate),
       ui_(ui),
       state_(INITIAL),
       fetch_token_success_count_(0),
       fetch_token_fail_count_(0) {}
 
 AppNotifyChannelSetup::~AppNotifyChannelSetup() {}
 
 void AppNotifyChannelSetup::Start() {
+  if (g_interceptor) {
+    std::string channel_id;
+    std::string error;
+    g_interceptor->DoIntercept(this, &channel_id, &error);
+    delegate_->AppNotifyChannelSetupComplete(channel_id, error, this);

[Munjal (Google), 2011/11/30 19:21:12]

I am actually thinking if there is a way to avoid this interceptor thing
completely.

Some way I can thinks of are:
- Use mock and mock out Start method.
- Add a private method to call the delegate and call that method explicitly in
tests.

I have seen both approaches used in various tests. And it would be nice to avoid
the interceptor.

[asargent_no_longer_on_chrome, 2011/11/30 19:45:21]

Because this is used by a browser test, the entire chrome binary is being run
and there isn't a convenient way to mock out the entire class without having a
global variable / static method somewhere. The reason I did it this way is that
I didn't want to make the call to
ExtensionService::SetAppNotificationSetupDone() from inside the
AppNotifyChannelSetup class directly, but rather from the ExtensionTabHelper
class which uses a AppNotifyChannelSetup object. The ExtensionTabHelper object
is glue that lies at the intersection of multiple processes and handles IPC and
method dispatch, so it isn't easily amenable to simple unit tests.

Instead of having an Interceptor, we could instead have something like
SetNextResultForTests(string code, string error) and then have the
AppNotifyChannelSetup just return those if they happen to be set. That would be
sufficient for the test I added and is slightly simpler than an interceptor.

[Munjal (Google), 2011/11/30 20:05:56]

Thanks for the explanation. The interceptor approach is fine then, don't worry
about changing it.

+    return;
+  }
   AddRef();  // Balanced in ReportResult.
   BeginLogin();
 }
 
 void AppNotifyChannelSetup::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_TOKEN_AVAILABLE) {
     TokenService::TokenAvailableDetails* tok_details =
@@ skipping 216 matching lines @@
   }
 }
 
 void AppNotifyChannelSetup::ReportResult(
     const std::string& channel_id,
     const std::string& error) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   CHECK(state_ == CHANNEL_ID_SETUP_DONE || state_ == ERROR_STATE);
 
   if (delegate_.get()) {
-    delegate_->AppNotifyChannelSetupComplete(
-        channel_id, error, return_route_id_, callback_id_);
+    delegate_->AppNotifyChannelSetupComplete(channel_id, error, this);
   }
   Release(); // Matches AddRef in Start.
 }
 
 // static
 GURL AppNotifyChannelSetup::GetCWSChannelServiceURL() {
   CommandLine* command_line = CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kAppNotifyChannelServerURL)) {
     std::string switch_value = command_line->GetSwitchValueASCII(
         switches::kAppNotifyChannelServerURL);
@@ skipping 51 matching lines @@
   DictionaryValue* dict = static_cast<DictionaryValue*>(value.get());
   if (!dict->Get("id", &channel_id_value))
     return false;
   if (channel_id_value->GetType() != base::Value::TYPE_STRING)
     return false;
 
   StringValue* channel_id = static_cast<StringValue*>(channel_id_value);
   channel_id->GetAsString(result);
   return true;
 }