Save the oauth client id used in App Notification setup

Save the oauth client id used in App Notification setup

We're also syncing this value, which has the nice side effect of letting us get rid of the "setup done" boolean we were planning on using.

BUG=105682
TEST=Setting up app notifications should leave a "app_notif_client_id" key
in your Preferences file in the extensions section associated with the app
in question.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112357

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

Mostly nits.

One other thing: is there any other plumbing that is needed on sync side for
this field? I don't remember.

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:60: 
Nit: remove extra newline.

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:61: static
AppNotifyChannelSetup::InterceptorForTests* g_interceptor = NULL;
Would it make more sense for this to be in anonymous namespace or in a static
member of the class itself?

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:66: g_interceptor =
interceptor;
Nit: should we check that g_interceptor is NULL to avoid unintentional chaining?

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:595: std::string
ExtensionPrefs::AppNotificationClientId(
Nit: AppNotificationClientId to GetAppNotificationClientId?

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_sync_data.h (left):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sync_data.h:59: bool
notifications_initial_setup_done() const {
May be keep this method also in addition to the new method to check if initial
setup is done? Name it IsInitialSetupDone?

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

Addressed comments.

> One other thing: is there any other plumbing that is needed > on sync side for
> this field? I don't remember.

I'll check on this.

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:60: 
On 2011/11/29 21:55:34, munjal wrote:
> Nit: remove extra newline.

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...

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:61: static
AppNotifyChannelSetup::InterceptorForTests* g_interceptor = NULL;
On 2011/11/29 21:55:34, munjal wrote:
> Would it make more sense for this to be in anonymous namespace or in a static
> member of the class itself?

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

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:66: g_interceptor =
interceptor;
On 2011/11/29 21:55:34, munjal wrote:
> Nit: should we check that g_interceptor is NULL to avoid unintentional
chaining?

Good idea, done.

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:595: std::string
ExtensionPrefs::AppNotificationClientId(
On 2011/11/29 21:55:34, munjal wrote:
> Nit: AppNotificationClientId to GetAppNotificationClientId?

Done. 

I was ambivalent about this when writing the code initially because some other
functions in this file follow the pattern of Foo/SetFoo, but others follow
GetFoo/SetFoo.

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_sync_data.h (left):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sync_data.h:59: bool
notifications_initial_setup_done() const {
On 2011/11/29 21:55:34, munjal wrote:
> May be keep this method also in addition to the new method to check if initial
> setup is done? Name it IsInitialSetupDone?

I thought the idea was that the initial_setup_done field in the sync protobuffer
was deprecated and we were going to stop using it (that's what the comment in
app_specifics.proto that you added says :) ).

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

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:97:
delegate_->AppNotifyChannelSetupComplete(channel_id, error, this);
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]

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:97:
delegate_->AppNotifyChannelSetupComplete(channel_id, error, this);
On 2011/11/30 19:21:12, munjal wrote:
> 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.

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:55]

LGTM

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8727024/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:97:
delegate_->AppNotifyChannelSetupComplete(channel_id, error, this);
On 2011/11/30 19:45:21, Antony Sargent wrote:
> On 2011/11/30 19:21:12, munjal wrote:
> > 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.
> 
> 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.
> 

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

[commit-bot: I haz the power, 2011-11-30 22:27:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/8727024/8001

[commit-bot: I haz the power, 2011-12-01 00:41:11]

Change committed as 112357