Save the oauth client id used in App Notification setup

chrome/browser/extensions/app_notify_channel_setup.cc

Index: chrome/browser/extensions/app_notify_channel_setup.cc
diff --git a/chrome/browser/extensions/app_notify_channel_setup.cc b/chrome/browser/extensions/app_notify_channel_setup.cc
index 2e11a49ee14852d5afaab6d87cb96c10cc21b4c3..1253379a445f19b229858c4b6c51cba126174bb2 100644
--- a/chrome/browser/extensions/app_notify_channel_setup.cc
+++ b/chrome/browser/extensions/app_notify_channel_setup.cc
@@ -57,6 +57,15 @@ static const char* kRelevantGaiaServices[] = {
 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,
@@ -81,6 +90,13 @@ AppNotifyChannelSetup::AppNotifyChannelSetup(
 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();
 }
@@ -317,8 +333,7 @@ void AppNotifyChannelSetup::ReportResult(
   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.
 }