Move the notification part out of ConsumerEnrollmentHandler so that it can be reused for unenrollme…

Move the notification part out of ConsumerEnrollmentHandler so that it can be reused for unenrollment as well.

ConsumerManagementNotification is always created for owner profiles even when
there is no pending notification to show, because enrollment or unenrollment
may happen after the owner signs in.

BUG=chromium:353050
TEST=unit_tests

Committed: https://crrev.com/fcbf1a950801608326e710c50f4427b9da5fac9c
Cr-Commit-Position: refs/heads/master@{#304998}

[davidyu, 2014-11-15 06:16:45]

davidyu@chromium.org changed reviewers:
+ bartfab@chromium.org, mnissler@chromium.org

[Mattias Nissler (ping if slow), 2014-11-18 10:02:27]

LGTM, happy to make another sanity pass in case you do change the naming.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.cc
(right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.cc:60: if
(service->GetStatus() != ConsumerManagementService::STATUS_ENROLLING)
nit: could merge this conditional with the one in line 57.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_notification.cc (right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification.cc:9: #include
"base/logging.h"
needed?

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_notification.h (right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification.h:20: class
ConsumerManagementNotification
Naming is a bit unfortunate here. This class' life time doesn't actually
correspond with the life time of the actual notification. How about renaming
this to ConsumerManagementNotificationDispatcher to make it clear that this
creates notifications, and discourage thinking about this as the notification?

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc
(right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc:16:
#include "testing/gmock/include/gmock/gmock.h"
not used

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc:18: 
nit: remove extra newline

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc:20:
using testing::NiceMock;
not used

[davidyu, 2014-11-19 02:54:21]

I renamed the class to ConsumerManagementNotifier, which is shorter.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.cc
(right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.cc:60: if
(service->GetStatus() != ConsumerManagementService::STATUS_ENROLLING)
On 2014/11/18 10:02:26, Mattias Nissler wrote:
> nit: could merge this conditional with the one in line 57.

Done.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_notification.cc (right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification.cc:9: #include
"base/logging.h"
On 2014/11/18 10:02:26, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_notification.h (right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification.h:20: class
ConsumerManagementNotification
On 2014/11/18 10:02:27, Mattias Nissler wrote:
> Naming is a bit unfortunate here. This class' life time doesn't actually
> correspond with the life time of the actual notification. How about renaming
> this to ConsumerManagementNotificationDispatcher to make it clear that this
> creates notifications, and discourage thinking about this as the notification?

Done.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc
(right):

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc:16:
#include "testing/gmock/include/gmock/gmock.h"
On 2014/11/18 10:02:27, Mattias Nissler wrote:
> not used

Done.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc:18: 
On 2014/11/18 10:02:27, Mattias Nissler wrote:
> nit: remove extra newline

Done.

https://codereview.chromium.org/733613005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_notification_unittest.cc:20:
using testing::NiceMock;
On 2014/11/18 10:02:27, Mattias Nissler wrote:
> not used

Done.

[davidyu, 2014-11-19 02:56:13]

davidyu@chromium.org changed reviewers:
+ skuhne@chromium.org

[davidyu, 2014-11-19 02:56:14]

Hi Stefan, can you help reviewing chrome_browser_main_extra_parts_profiles.cc?
Thanks!

[Mattias Nissler (ping if slow), 2014-11-19 08:05:10]

LGTM

[Mr4D (OOO till 08-26), 2014-11-19 16:33:23]

chrome_browser_main_extra_parts_profiles lgtm

[Even though I have to say that the function
EnsureBrowserContextKeyedServiceFactoriesBuilt() in general should probably be
re-thought]

[davidyu, 2014-11-20 01:56:14]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-11-20 01:56:38]

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

[commit-bot: I haz the power, 2014-11-20 04:28:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 04:28:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidyu, 2014-11-20 05:07:07]

On 2014/11/20 04:28:35, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Hi Mattias,

PTAL of consumer_management_notifier_factory.cc and
consumer_enrollment_handler_factory.cc. I added a check for the existence of
UserManager before calling IsOwnerProfile(). An alternative is to put this check
in IsOwnerProfile(), but I am afraid that it may affect too many tests.

[davidyu, 2014-11-20 05:08:53]

On 2014/11/20 05:07:07, davidyu wrote:
> On 2014/11/20 04:28:35, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Hi Mattias,
> 
> PTAL of consumer_management_notifier_factory.cc and
> consumer_enrollment_handler_factory.cc. I added a check for the existence of
> UserManager before calling IsOwnerProfile(). An alternative is to put this
check
> in IsOwnerProfile(), but I am afraid that it may affect too many tests.

FYI, IsOwnerProfile() crashes when it calls UserManager::Get(), which has a
check statement to ensure that UserManager is already inialized, which is not
the case for some tests that create profile but don't have a user manager.

[Mattias Nissler (ping if slow), 2014-11-20 08:29:37]

Still LGTM, although it worries me that our testing code is still so b0rked.

[davidyu, 2014-11-20 08:56:26]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-11-20 08:56:39]

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

[commit-bot: I haz the power, 2014-11-20 10:20:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-20 10:21:12]

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

[bartfab (slow), 2014-11-27 14:09:10]

Belated LGTM with a few nits.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_enrollment_handler.cc (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_enrollment_handler.cc:11: #include
"base/time/time.h"
Nit: No longer used.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.h
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.h:21: class
ConsumerManagementService;
Nit: No longer used.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier.cc (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:40:
DesktopNotificationDelegate(const std::string& id,
Nit: #include <string>

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:44: virtual
std::string id() const override;
Nit: No |virtual| with |override|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:45: virtual void
ButtonClick(int button_index) override;
Nit: No |virtual| with |override|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:48: virtual
~DesktopNotificationDelegate();
Nit: Use |override| instead of |virtual|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:50: std::string
id_;
Nit: const.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:51: base::Closure
button_click_callback_;
Nit: const.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:96:
ConsumerManagementService::EnrollmentStage stage =
Nit: const.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:108:
base::string16 title;
Nit: #include "base/strings/string16.h"

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:138:
message_center::NOTIFICATION_TYPE_SIMPLE,
Nit: #include "ui/message_center/notification_types.h"

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier.h (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.h:27: virtual
~ConsumerManagementNotifier();
Nit: Use |override| instead of |virtual|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.h:29: virtual void
Shutdown() override;
Nit: No |virtual| with |override|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.h:32: virtual void
OnConsumerManagementStatusChanged() override;
Nit: No |virtual| with |override|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier_factory.h
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_factory.h:32:
virtual ~ConsumerManagementNotifierFactory();
Nit: Use |override| instead of |virtual|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_factory.h:37:
virtual bool ServiceIsCreatedWithBrowserContext() const override;
Nit: No |virtual| with |override|.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/consumer_management_notifier_factory_unittest.cc
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_factory_unittest.cc:12:
#include "chrome/browser/chromeos/policy/consumer_management_notifier.h"
Nit: Where is this used?

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier_unittest.cc
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_unittest.cc:32:
make_scoped_ptr(fake_service_));
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_unittest.cc:35:
virtual void SetUp() override {
Nit: No |virtual| with |override|.

[davidyu, 2014-11-28 02:45:26]

Fixed in https://codereview.chromium.org/760373002/

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_enrollment_handler.cc (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_enrollment_handler.cc:11: #include
"base/time/time.h"
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No longer used.

This is used by the parameter of OnGetTokenSuccess().

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.h
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_enrollment_handler_factory.h:21: class
ConsumerManagementService;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier.cc (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:40:
DesktopNotificationDelegate(const std::string& id,
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:44: virtual
std::string id() const override;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No |virtual| with |override|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:45: virtual void
ButtonClick(int button_index) override;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No |virtual| with |override|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:48: virtual
~DesktopNotificationDelegate();
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: Use |override| instead of |virtual|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:50: std::string
id_;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:51: base::Closure
button_click_callback_;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:96:
ConsumerManagementService::EnrollmentStage stage =
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:108:
base::string16 title;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: #include "base/strings/string16.h"

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.cc:138:
message_center::NOTIFICATION_TYPE_SIMPLE,
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: #include "ui/message_center/notification_types.h"

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier.h (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.h:27: virtual
~ConsumerManagementNotifier();
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: Use |override| instead of |virtual|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.h:29: virtual void
Shutdown() override;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No |virtual| with |override|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier.h:32: virtual void
OnConsumerManagementStatusChanged() override;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No |virtual| with |override|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier_factory.h
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_factory.h:32:
virtual ~ConsumerManagementNotifierFactory();
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: Use |override| instead of |virtual|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_factory.h:37:
virtual bool ServiceIsCreatedWithBrowserContext() const override;
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No |virtual| with |override|.

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/consumer_management_notifier_factory_unittest.cc
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_factory_unittest.cc:12:
#include "chrome/browser/chromeos/policy/consumer_management_notifier.h"
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: Where is this used?

It was used in a piece of testing code that didn't get into this CL at the end.
Removed.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_notifier_unittest.cc
(right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_unittest.cc:32:
make_scoped_ptr(fake_service_));
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_notifier_unittest.cc:35:
virtual void SetUp() override {
On 2014/11/27 14:09:09, bartfab wrote:
> Nit: No |virtual| with |override|.

Done.

[bartfab (slow), 2014-11-28 14:52:14]

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_enrollment_handler.cc (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_enrollment_handler.cc:11: #include
"base/time/time.h"
On 2014/11/28 02:45:25, davidyu wrote:
> On 2014/11/27 14:09:09, bartfab wrote:
> > Nit: No longer used.
> 
> This is used by the parameter of OnGetTokenSuccess().

You never dereference that type though. But sure, if the compiler complains
without the include, so be it.

[davidyu, 2014-11-30 04:16:04]

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_enrollment_handler.cc (right):

https://codereview.chromium.org/733613005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_enrollment_handler.cc:11: #include
"base/time/time.h"
On 2014/11/28 14:52:14, bartfab wrote:
> On 2014/11/28 02:45:25, davidyu wrote:
> > On 2014/11/27 14:09:09, bartfab wrote:
> > > Nit: No longer used.
> > 
> > This is used by the parameter of OnGetTokenSuccess().
> 
> You never dereference that type though. But sure, if the compiler complains
> without the include, so be it.

Ok. I'll fix it in another CL.