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

chrome/browser/chromeos/policy/consumer_management_service.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_CHROMEOS_POLICY_CONSUMER_MANAGEMENT_SERVICE_H_
 #define CHROME_BROWSER_CHROMEOS_POLICY_CONSUMER_MANAGEMENT_SERVICE_H_
 
 #include <string>
 
 #include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #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"
 #include "google_apis/gaia/oauth2_token_service.h"
 
 class PrefRegistrySimple;
 class Profile;
 class ProfileOAuth2TokenService;

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

Nit: No longer used.

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

Done.

 
 namespace chromeos {
 class CryptohomeClient;
 }
 
 namespace cryptohome {
 class BaseReply;
 }
 
 namespace policy {
@@ skipping 77 matching lines @@
       const base::Time& expiration_time) OVERRIDE;
   virtual void OnGetTokenFailure(
       const OAuth2TokenService::Request* request,
       const GoogleServiceAuthError& error) OVERRIDE;
 
   OAuth2TokenService::Request* GetTokenRequestForTesting() {
     return token_request_.get();
   }
 
  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,
       bool dbus_success,
       const cryptohome::BaseReply& reply);
 
   void OnSetBootAttributeDone(const SetOwnerCallback& callback,
                               chromeos::DBusMethodCallStatus call_status,
                               bool dbus_success,
                               const cryptohome::BaseReply& reply);
@@ skipping 18 matching lines @@
   void OnOwnerAccessTokenAvailable(const std::string& access_token);
 
   // Called upon the completion of the enrollment process.
   void OnEnrollmentCompleted(EnrollmentStatus status);
 
   // Ends the enrollment process and shows a desktop notification if the
   // current user is the owner.
   void EndEnrollment(ConsumerEnrollmentState state);
 
   // Shows a desktop notification and resets the enrollment state.
-  void ShowDesktopNotificationAndResetState(ConsumerEnrollmentState state);
+  void ShowDesktopNotificationAndResetState(ConsumerEnrollmentState state,
+                                            Profile* profile);
+
+  // Opens the settings page.
+  void OpenSettingsPage(Profile* profile) const;
+
+  // 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_;
 
-  std::string enrolling_account_id_;
-  ProfileOAuth2TokenService* enrolling_token_service_;
-
+  Profile* enrolling_profile_;
   scoped_ptr<OAuth2TokenService::Request> token_request_;
   content::NotificationRegistrar registrar_;
   base::WeakPtrFactory<ConsumerManagementService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ConsumerManagementService);
 };
 
 }  // namespace policy
 
 #endif  // CHROME_BROWSER_CHROMEOS_POLICY_CONSUMER_MANAGEMENT_SERVICE_H_