Show a desktop notification when the enrollment is completed or failed.

chrome/browser/chromeos/policy/consumer_management_service.h

Index: chrome/browser/chromeos/policy/consumer_management_service.h
diff --git a/chrome/browser/chromeos/policy/consumer_management_service.h b/chrome/browser/chromeos/policy/consumer_management_service.h
index d1cb6998398c4b90e3bdddd2abae2cc624c0bcc9..a647429c300e82360299b743189b41c2aaa8d905 100644
--- a/chrome/browser/chromeos/policy/consumer_management_service.h
+++ b/chrome/browser/chromeos/policy/consumer_management_service.h
@@ -12,6 +12,8 @@
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/strings/string16.h"
+#include "chrome/browser/notifications/notification_delegate.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -117,6 +119,32 @@ class ConsumerManagementService : public content::NotificationObserver,
   }
 
  private:
+  class DesktopNotificationDelegate : public NotificationDelegate {

[oshima, 2014/08/21 22:47:07]

Looks like you're not using this in the header. In that case, it's better to
move to anonymous namespace in .cc. You can inline virtual methods there (it's
prohibited in header but .cc is fine)

[bartfab (slow), 2014/08/21 23:35:45]

I have seen inlining of virtual methods done in .cc files (especially in tests,
where it is very common) but I cannot see any rule in the style guide that would
actually allow it. The style guide says *in bold* that you must never inline
virtual methods.

[oshima, 2014/08/22 00:44:05]

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

It's prohibited in header for technical reasons. In .cc, there are people who
don't
like and those who likt it. I'm one of those who think this is ok. There are
actually
a lot of examples in chromium. (I spent enough time on Java, which made me think
not inlining in .cc is a kind of dup)

That's being said, you're the owner of this dir, so your opinion matters than
mine.
That's why I lgtm'ed with my comment. I still think this should be in .cc file
though.

[davidyu, 2014/08/22 04:23:10]

Moved to .cc.

[bartfab (slow), 2014/08/25 13:07:30]

In addition to the Dos and Don'ts you linked to, there is the actual style guide
which explicitly forbids inlining of virtual methods, without making any
distinction between header and implementation files:

http://www.chromium.org/developers/coding-style#TOC-Inline-functions

[oshima, 2014/08/25 13:42:04]

That line describe the rule for accessor method.
The reason why this is not allowed in header is that compiler generates
code for each file that include header. Here are examples:

https://code.google.com/p/chromium/codesearch#search/&q=virtual.*OVERRIDE.*%7...

There is another exception in this rule. You can inline a virtual function in
empty body. This is often used in observer classes. Here are examples:

https://code.google.com/p/chromium/codesearch#search/&q=virtual.*OVERRIDE.*%7...

It's up to OWNERS whether or not to allow it, so you can decide.

+   public:
+    // |button_click_callback| is called when the button in the notification is
+    // clicked.
+    DesktopNotificationDelegate(
+        const std::string& id,
+        const base::Closure& button_click_callback);
+
+    virtual ~DesktopNotificationDelegate();
+
+    // NotificationDelegate:
+    virtual std::string id() const OVERRIDE;
+    virtual content::WebContents* GetWebContents() const OVERRIDE;
+    virtual void Display() OVERRIDE;
+    virtual void ButtonClick(int button_index) OVERRIDE;
+    virtual void Error() OVERRIDE;
+    virtual void Close(bool by_user) OVERRIDE;
+    virtual void Click() OVERRIDE;
+
+   private:
+    std::string id_;
+    base::Closure button_click_callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(DesktopNotificationDelegate);
+  };
+
   void OnGetBootAttributeDone(
       const GetOwnerCallback& callback,
       chromeos::DBusMethodCallStatus call_status,
@@ -155,13 +183,21 @@ class ConsumerManagementService : public content::NotificationObserver,
   void EndEnrollment(ConsumerEnrollmentState state);
 
   // Shows a desktop notification and resets the enrollment state.
-  void ShowDesktopNotificationAndResetState(ConsumerEnrollmentState state);
+  void ShowDesktopNotificationAndResetState(ConsumerEnrollmentState state,
+                                            Profile* profile);
 
-  chromeos::CryptohomeClient* client_;
+  // Opens the settings page.
+  void OpenSettingsPage(Profile* profile) const;
 
-  std::string enrolling_account_id_;
-  ProfileOAuth2TokenService* enrolling_token_service_;
+  // Opens the enrollment confirmation dialog in the settings page.
+  void TryEnrollmentAgain(Profile* profile) const;
+
+  // Returns the sign-in authenticated account ID of the profile.
+  const std::string& GetAccountIdFromProfile(Profile* profile) const;

[bartfab (slow), 2014/08/21 12:04:25]

Nit 1: "sign-in authenticated account ID" is really hard to parse. How about
something like: Returns the account ID signed in to |profile|.

Nit 2: This could be a helper method in an anonymous namespace.

[davidyu, 2014/08/22 04:23:10]

Done.

+
+  chromeos::CryptohomeClient* client_;
 
+  Profile* enrolling_profile_;
   scoped_ptr<OAuth2TokenService::Request> token_request_;
   content::NotificationRegistrar registrar_;
   base::WeakPtrFactory<ConsumerManagementService> weak_ptr_factory_;