Add a couple of notifications related settings to app/extensions sync:

Add a couple of notifications related settings to app/extensions sync:
- Whether notifications setup is done once already.
- Whether the user has disabled notifications.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107692

[asargent_no_longer_on_chrome, 2011-10-27 16:52:21]

I have a bunch of naming suggestions along 2 lines:

-I'd like us to be calling these "app notifications" as opposed to
"notifications", to help reduce confusion with the web platform desktop
notifications. 

-In some places using singular rather than plural fits a little better with our
general conventions.

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:49: const char
kPrefNotificationsInitialSetup[] = "notif_initial_setup";
Name change suggestions:

kPrefNotificationsInitialSetup -> kPrefAppNotificationSetup

"notif_initial_setup" -> "app_notif_setup"

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:52: const char
kPrefNotificationsDisbaled[] = "notif_disabled";
Name change suggestion:

kPrefNotificationsDisbaled -> kPrefAppNotificationDisabled

"notif_disabled" -> "app_notif_disabled"

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:174: // Whether the notifications
initial setup is done for the given extension.
change "given extension" to "given app" in the comment

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:176: void
SetNotificationsInitialSetupDone(const std::string& extension_id,
Suggest changing names to:

IsAppNotificationSetupDone
SetAppNotificationSetupDone

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:179: // Whether notifications are
disabled for the given extension.
change "given extension" to "given app" in the comment

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:181: void
SetNotificationsDisabled(const std::string& extension_id, bool value);
Suggest changing names to:

IsAppNotificationDisabled
SetAppNotificationDisabled

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2055:
extension_prefs_->SetNotificationsInitialSetupDone(extension_id, value);
nit: probably not worth writing this into the prefs if the extension isn't
installed - perhaps move the check for the result of GetInstalledExtension up to
the top, and just exit this method if the extension isn't installed. (Perhaps
add NOTREACHED() too? Should anyone really call this on a non-installed
extension?)

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2071: if (extension)
same thing here about bailing out if the extension isn't installed

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:214: virtual bool
IsNotificationsInitialSetupDone(
Let's remove this IsNotificationsInitialSetupDone method in favor of just
calling through to the ExtensionPrefs method directly.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:216: virtual void
SetNotificationsInitialSetupDone(const std::string& extension_id,
Rename suggestion:

SetAppNotificationSetupDone

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:219: virtual bool
IsNotificationsDisabled(const std::string& extension_id) const;
Let's remove this IsNotificationsDisabled method in favor of just calling
through to the ExtensionPrefs method directly.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:220: virtual void
SetNotificationsDisabled(const std::string& extension_id,
Rename suggestion:

SetAppNotificationDisabled

[Munjal (Google), 2011-10-27 18:35:04]

Thanks for naming suggestions. When I named them, I didn't feel good about the
names I used and did think that notifications is probably too broad. So your
naming suggestions are perfect.

All done. Take a look.

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:49: const char
kPrefNotificationsInitialSetup[] = "notif_initial_setup";
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Name change suggestions:
> 
> kPrefNotificationsInitialSetup -> kPrefAppNotificationSetup
> 
> "notif_initial_setup" -> "app_notif_setup"

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:52: const char
kPrefNotificationsDisbaled[] = "notif_disabled";
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Name change suggestion:
> 
> kPrefNotificationsDisbaled -> kPrefAppNotificationDisabled
> 
> "notif_disabled" -> "app_notif_disabled"

Done.

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:174: // Whether the notifications
initial setup is done for the given extension.
On 2011/10/27 16:52:21, Antony Sargent wrote:
> change "given extension" to "given app" in the comment

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:176: void
SetNotificationsInitialSetupDone(const std::string& extension_id,
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Suggest changing names to:
> 
> IsAppNotificationSetupDone
> SetAppNotificationSetupDone

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:179: // Whether notifications are
disabled for the given extension.
On 2011/10/27 16:52:21, Antony Sargent wrote:
> change "given extension" to "given app" in the comment

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:181: void
SetNotificationsDisabled(const std::string& extension_id, bool value);
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Suggest changing names to:
> 
> IsAppNotificationDisabled
> SetAppNotificationDisabled
> 

Done.

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2055:
extension_prefs_->SetNotificationsInitialSetupDone(extension_id, value);
On 2011/10/27 16:52:21, Antony Sargent wrote:
> nit: probably not worth writing this into the prefs if the extension isn't
> installed - perhaps move the check for the result of GetInstalledExtension up
to
> the top, and just exit this method if the extension isn't installed. (Perhaps
> add NOTREACHED() too? Should anyone really call this on a non-installed
> extension?)

Done.

I have one question though - is it possible to have a race between an app being
installed and the webstie calling the JS API (and hte user is logged in) and we
detect that the website is actually an app? I guess it is unlikely but making
sure.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2071: if (extension)
On 2011/10/27 16:52:21, Antony Sargent wrote:
> same thing here about bailing out if the extension isn't installed

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:214: virtual bool
IsNotificationsInitialSetupDone(
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Let's remove this IsNotificationsInitialSetupDone method in favor of just
> calling through to the ExtensionPrefs method directly.

Done.
When I added this method, I thought it will be useful even though it is a single
line wrapper in case we need to mock it in testing (hence virtual). But turns
out testing was possible without that. So removed.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:216: virtual void
SetNotificationsInitialSetupDone(const std::string& extension_id,
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Rename suggestion:
> 
> SetAppNotificationSetupDone
> 

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:219: virtual bool
IsNotificationsDisabled(const std::string& extension_id) const;
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Let's remove this IsNotificationsDisabled method in favor of just calling
> through to the ExtensionPrefs method directly.

Done.

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:220: virtual void
SetNotificationsDisabled(const std::string& extension_id,
On 2011/10/27 16:52:21, Antony Sargent wrote:
> Rename suggestion:
> 
> SetAppNotificationDisabled

Done.

[asargent_no_longer_on_chrome, 2011-10-27 19:49:41]

extensions parts LGTM

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2055:
extension_prefs_->SetNotificationsInitialSetupDone(extension_id, value);
On 2011/10/27 18:35:05, munjal wrote:
> On 2011/10/27 16:52:21, Antony Sargent wrote:
> > nit: probably not worth writing this into the prefs if the extension isn't
> > installed - perhaps move the check for the result of GetInstalledExtension
up
> to
> > the top, and just exit this method if the extension isn't installed.
(Perhaps
> > add NOTREACHED() too? Should anyone really call this on a non-installed
> > extension?)
> 
> Done.
> 
> I have one question though - is it possible to have a race between an app
being
> installed and the webstie calling the JS API (and hte user is logged in) and
we
> detect that the website is actually an app? I guess it is unlikely but making
> sure.
A race condition is unlikely to happen - the way the code works for implementing
the browser process C++ side of the js method exposed in the renderer process,
we need to have a valid installed Extension already.

http://codereview.chromium.org/8399022/diff/13002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/8399022/diff/13002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:604:
DCHECK(Extension::IdIsValid(extension_id));
nit: UpdateExtensionPref already checks IdIsValid, so you can skip this here

http://codereview.chromium.org/8399022/diff/13002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:616:
DCHECK(Extension::IdIsValid(extension_id));
nit: UpdateExtensionPref already checks IdIsValid, so you can skip this here

[lipalani1, 2011-10-27 19:51:46]

lgtm for the protocol file and proto_value_conversion*.*.

[Munjal (Google), 2011-10-27 20:01:49]

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

http://codereview.chromium.org/8399022/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2055:
extension_prefs_->SetNotificationsInitialSetupDone(extension_id, value);
On 2011/10/27 19:49:41, Antony Sargent wrote:
> On 2011/10/27 18:35:05, munjal wrote:
> > On 2011/10/27 16:52:21, Antony Sargent wrote:
> > > nit: probably not worth writing this into the prefs if the extension isn't
> > > installed - perhaps move the check for the result of GetInstalledExtension
> up
> > to
> > > the top, and just exit this method if the extension isn't installed.
> (Perhaps
> > > add NOTREACHED() too? Should anyone really call this on a non-installed
> > > extension?)
> > 
> > Done.
> > 
> > I have one question though - is it possible to have a race between an app
> being
> > installed and the webstie calling the JS API (and hte user is logged in) and
> we
> > detect that the website is actually an app? I guess it is unlikely but
making
> > sure.
> A race condition is unlikely to happen - the way the code works for
implementing
> the browser process C++ side of the js method exposed in the renderer process,
> we need to have a valid installed Extension already.
> 

Great, good to know.