Factor Google Now welcome notifications out of DesktopNotificationService.

Factor Google Now welcome notifications out of DesktopNotificationService.

This changes the ExtensionWelcomeNotification to be its own keyed service,
which seperates it from the DesktopNotificationService.

BUG=

Committed: https://crrev.com/7015281c5501b8dffe74ed2b825a7a3c4edef34a
Cr-Commit-Position: refs/heads/master@{#296494}

[Peter Beverloo, 2014-09-18 15:47:25]

peter@chromium.org changed reviewers:
+ dewittj@chromium.org, mukai@chromium.org

[Peter Beverloo, 2014-09-18 15:47:26]

While this *technically* could be part of the DesktopNotificationService per its
definition, I think it's significantly cleaner as a separate sub-system since it
serves a very specific purpose.

Jun, moving files to an extensions/ sub-folder is a first step towards the new
folder structure in the component. WDYT?

[dewittj, 2014-09-18 17:48:03]

dewittj@chromium.org changed reviewers:
+ robliao@chromium.org

[dewittj, 2014-09-18 17:48:15]

+ robliao for chrome now

[Jun Mukai, 2014-09-18 17:53:48]

I am not sure about creating a new directory
chrome/browser/notifications/extensions, it can be simply in
chrome/browser/notifications directly.

Otherwise, it lgtm with the following nits.

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/extensions/extension_welcome_notification.cc
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification.cc:143:
ExtensionWelcomeNotification::kChromeNowExtensionID),
You can just say kChromeNowExtensionID because it's in the scope of
ExtensionWelcomeNotification ??
Then it would fit into the previous line.

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc:31:
BrowserContextDependencyManager::GetInstance()) {}
DependsOn(DesktopNotificationServiceFactory::GetInstance());

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc:38:
ExtensionWelcomeNotification::Create(static_cast<Profile*>(context));
It's better to cleanup the initialization -- change Create() to return simply
the pointer, or simply new ExtensionWelcomeNotification() could be okay.

[robliao, 2014-09-18 19:44:52]

On 2014/09/18 17:53:48, Jun Mukai wrote:
> I am not sure about creating a new directory
> chrome/browser/notifications/extensions, it can be simply in
> chrome/browser/notifications directly.
> 
> Otherwise, it lgtm with the following nits.
> 
>
https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
> File chrome/browser/notifications/extensions/extension_welcome_notification.cc
> (right):
> 
>
https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
> chrome/browser/notifications/extensions/extension_welcome_notification.cc:143:
> ExtensionWelcomeNotification::kChromeNowExtensionID),
> You can just say kChromeNowExtensionID because it's in the scope of
> ExtensionWelcomeNotification ??
> Then it would fit into the previous line.
> 
>
https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
> File
>
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc
> (right):
> 
>
https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
>
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc:31:
> BrowserContextDependencyManager::GetInstance()) {}
> DependsOn(DesktopNotificationServiceFactory::GetInstance());
> 
>
https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
>
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc:38:
> ExtensionWelcomeNotification::Create(static_cast<Profile*>(context));
> It's better to cleanup the initialization -- change Create() to return simply
> the pointer, or simply new ExtensionWelcomeNotification() could be okay.

Adding the Chrome Now team as an FYI.

[robliao, 2014-09-18 20:10:01]

Overall LGTM and +1 to adding the DependsOn

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/extensions/extension_welcome_notification.cc
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification.cc:30:
const char ExtensionWelcomeNotification::kChromeNowExtensionID[]=
Space between [] and =

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/extensions/extension_welcome_notification.h
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification.h:37: //
hiding a welcome notification for built-in components which show
Nit: s/which/that

The first clause is non-restrictive, so 'which' works. The
ExtensionWelcomeNotification is indeed a KeyedService.

The second clause is restrictive. The ExtensionWelcomeNotification only manages
the welcome notification for built-in components that show notifications, not
all built-in components.

[robliao, 2014-09-18 20:13:06]

https://codereview.chromium.org/580093003/diff/1/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:430:
ExtensionWelcomeNotification::RegisterProfilePrefs(registry);
And a quick question here. What happens if there is a
MessageCenterNotificationManager Registration but no
ExtensionWelcomeNotification registration?

[Peter Beverloo, 2014-09-19 13:01:21]

peter@chromium.org changed reviewers:
+ bauerb@chromium.org

[Peter Beverloo, 2014-09-19 13:01:23]

All comments addressed, thank you for the quick reviews!

Adding Bernhard for //c/b/prefs/ OWNERS.

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/extensions/extension_welcome_notification.cc
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification.cc:143:
ExtensionWelcomeNotification::kChromeNowExtensionID),
On 2014/09/18 17:53:48, Jun Mukai wrote:
> You can just say kChromeNowExtensionID because it's in the scope of
> ExtensionWelcomeNotification ??
> Then it would fit into the previous line.

I removed the prefix, but since still end up with an 84-character line the break
needs to stay.

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/extensions/extension_welcome_notification.h
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification.h:37: //
hiding a welcome notification for built-in components which show
On 2014/09/18 20:10:01, robliao wrote:
> Nit: s/which/that
> 
> The first clause is non-restrictive, so 'which' works. The
> ExtensionWelcomeNotification is indeed a KeyedService.
> 
> The second clause is restrictive. The ExtensionWelcomeNotification only
manages
> the welcome notification for built-in components that show notifications, not
> all built-in components.

Insightful, thanks :-).

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
File
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc
(right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc:31:
BrowserContextDependencyManager::GetInstance()) {}
On 2014/09/18 17:53:48, Jun Mukai wrote:
> DependsOn(DesktopNotificationServiceFactory::GetInstance());

Done.

https://codereview.chromium.org/580093003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/extensions/extension_welcome_notification_factory.cc:38:
ExtensionWelcomeNotification::Create(static_cast<Profile*>(context));
On 2014/09/18 17:53:48, Jun Mukai wrote:
> It's better to cleanup the initialization -- change Create() to return simply
> the pointer, or simply new ExtensionWelcomeNotification() could be okay.

I'm inclined to disagree. Returning a scoped_ptr indicates that Create() passes
ownership to the caller. However, there seems to be quite some precedence
throughout Chromium for returning a raw pointer, and the name Create() does
imply something is being allocated, so I don't feel very strongly about it.

Done.

https://codereview.chromium.org/580093003/diff/1/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/580093003/diff/1/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:430:
ExtensionWelcomeNotification::RegisterProfilePrefs(registry);
On 2014/09/18 20:13:06, robliao wrote:
> And a quick question here. What happens if there is a
> MessageCenterNotificationManager Registration but no
> ExtensionWelcomeNotification registration?

Hm, this would occur for an Android build which enables both notifications and
extensions. I've updated the #ifdef to check for !ANDROID as well.

What would happen -- I'm not sure, I'm not familiar enough with the preference
system to comment on that. Quickly checking with bauerb@ revealed that we'd hit
a DCHECK.

[Bernhard Bauer, 2014-09-22 08:34:31]

lgtm

[dewittj, 2014-09-24 18:32:02]

lgtm

[Peter Beverloo, 2014-09-24 18:53:38]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-09-24 18:53:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/580093003/40001

[commit-bot: I haz the power, 2014-09-24 19:56:35]

Committed patchset #3 (id:40001) as 54f296f005fcd62a584a00f6a818bea2688a1699

[commit-bot: I haz the power, 2014-09-24 19:57:12]

Patchset 3 (id:??) landed as
https://crrev.com/7015281c5501b8dffe74ed2b825a7a3c4edef34a
Cr-Commit-Position: refs/heads/master@{#296494}