Do nothing on OnSessionStopped if ARC is being restarted.

chrome/browser/chromeos/arc/arc_session_manager.h

 // Copyright 2016 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_ARC_ARC_SESSION_MANAGER_H_
 #define CHROME_BROWSER_CHROMEOS_ARC_ARC_SESSION_MANAGER_H_
 
 #include <memory>
 #include <ostream>
 #include <string>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
 #include "chrome/browser/chromeos/policy/android_management_client.h"
-#include "components/arc/arc_session_observer.h"
+#include "components/arc/arc_session_runner.h"
+#include "components/arc/arc_stop_reason.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/sync_preferences/pref_service_syncable_observer.h"
 
 class ArcAppLauncher;
 class Profile;
 
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
 
 namespace arc {
 
 class ArcAndroidManagementChecker;
 class ArcAuthInfoFetcher;
 class ArcAuthContext;
-class ArcSessionRunner;
 class ArcTermsOfServiceNegotiator;
 enum class ProvisioningResult : int;
 
 // This class proxies the request from the client to fetch an auth code from
 // LSO. It lives on the UI thread.
-class ArcSessionManager : public ArcSessionObserver,
+class ArcSessionManager : public ArcSessionRunner::Observer,
                           public ArcSupportHost::Observer,
                           public sync_preferences::PrefServiceSyncableObserver {
  public:
   // Represents each State of ARC session.
   // NOT_INITIALIZED: represents the state that the Profile is not yet ready
   //   so that this service is not yet initialized, or Chrome is being shut
   //   down so that this is destroyed.
   // STOPPED: ARC session is not running, or being terminated.
   // SHOWING_TERMS_OF_SERVICE: "Terms Of Service" page is shown on ARC support
   //   Chrome app.
@@ skipping 27 matching lines @@
   // relationship with |enable_requested_|.
   enum class State {
     NOT_INITIALIZED,
     STOPPED,
     SHOWING_TERMS_OF_SERVICE,
     CHECKING_ANDROID_MANAGEMENT,
     REMOVING_DATA_DIR,
     ACTIVE,
   };
 
+  // Observer for those services outside of ARC which want to know ARC events.
   class Observer {
    public:
-    virtual ~Observer() = default;
-
     // Called to notify that whether Google Play Store is enabled or not, which
     // is represented by "arc.enabled" preference, is updated.
     virtual void OnArcPlayStoreEnabledChanged(bool enabled) {}
 
     // Called to notify that ARC has been initialized successfully.
     virtual void OnArcInitialStart() {}
 
+    // Called when ARC session is stopped, and is not being restarted
+    // automatically.
+    virtual void OnArcSessionStopped(ArcStopReason stop_reason) {}
+
     // Called to notify that Android data has been removed. Used in
     // browser_tests
     virtual void OnArcDataRemoved() {}
+
+   protected:
+    // Do not directly create/destroy the instance via the interface.
+    Observer() = default;
+    ~Observer() = default;

[Yusuke Sato, 2017/03/01 15:51:01]

qq: Thanks. Read the discussion. Is there any practical performance advantage
for us (ARC code) for not having virtual here? As mentioned in the discussion,
there seems to be one downside for omitting virtual:
https://bugs.llvm.org/show_bug.cgi?id=28460

The compiler won't catch the

 class D1 : public Observer {};
 class D2 : public D1 {};
 ...
 std::unique_ptr<D1> d2;

case, correct? As our observer code here is not performance sensitive, can't we
continue to use the original, easier to review way? Am I missing something?

[hidehiko, 2017/03/01 17:28:02]

As for your example, no, it won't. However, it is because D1 (not Observer) does
not have virtual dtor, although it is designed to be inherited.
I.e.,

class D1 {};
class D2 : public D1 {};
std::unique_ptr<D1> d2;

has same problem. In that sense, theoretically, IMHO it *can* be considered
Observer's dtor is unrelated.
# I hope compiler supports both cases.

Practically, an author may *misbelieve* Observer has virtual dtor, without any
checking.
If we'd like to help to avoid such misbelief, adding virtual to Observer's dtor
would be an approach, because on current Chrome build, D1's dtor is required to
be marked as "override".

On the other hand, putting dtor into "protected", in order to prohibit delete
via Observer, may have value, too, IMHO. It is because Observer's delete is not
designed to be polimorphically deleted.

So, options:
1) public virtual dtor.
2) protected non-virtual dtor.
3) protected virtual dtor.

Following the discussion I linked, I chose 2) in the PS, but 3) may be better to
satisfy both our thoughts?

Note: in Chromium, there seems no standard way, at the moment.
i.e., all of them (and public non-virual dtor :-/) are used (but now I found 2)
is relatively rare use case).

Note2: I do not think its performance critical, and not much worried about
performance here.

WDYT?

[Yusuke Sato, 2017/03/01 17:40:24]

Sorry if my comment above was not clear enough, but I'm 100% okay with using
protected:. I just wanted to make it virtual too to make it a bit less
error-prone. So I'd be very happy with 3).

[hidehiko, 2017/03/01 17:49:41]

Sure, done.
Note that, removed ctor, too, considering;
1) protected dtor is enough to prohibit instantiating Observer directly.
2) For consistency with other Chromium code.

[Luis Héctor Chávez, 2017/03/01 18:26:52]

I'll add this for posterity in the ARC++ style guide.

[hidehiko, 2017/03/01 18:43:02]

SG. Thanks!

   };
 
   explicit ArcSessionManager(
       std::unique_ptr<ArcSessionRunner> arc_session_runner);
   ~ArcSessionManager() override;
 
   static ArcSessionManager* Get();
 
   // Exposed here for unit_tests validation.
   static bool IsOobeOptInActive();
@@ skipping 26 matching lines @@
   // In addition, this triggers to show ArcAuthNotification, if necessary.
   // Note that this must be called after SetProfile().
   // TODO(hidehiko): Extract preference related code into a class to split the
   // dependencty.
   void StartPreferenceHandler();
 
   // Adds or removes observers.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  // Adds or removes ArcSessionObservers.
-  // TODO(hidehiko): The observer should be migrated into
-  // ArcSessionManager::Observer.
-  void AddSessionObserver(ArcSessionObserver* observer);
-  void RemoveSessionObserver(ArcSessionObserver* observer);
-
   // Returns true if ARC instance is running/stopped, respectively.
   // See ArcSessionRunner::IsRunning()/IsStopped() for details.
   bool IsSessionRunning() const;
   bool IsSessionStopped() const;
 
   // Called from ARC support platform app when user cancels signing.
   void CancelAuthCode();
 
   // Requests to enable ARC session. This starts ARC instance, or maybe starts
   // Terms Of Service negotiation if they haven't been accepted yet.
@@ skipping 90 matching lines @@
   // Called when the Android management check is done in opt-in flow or
   // re-auth flow.
   void OnAndroidManagementChecked(
       policy::AndroidManagementClient::Result result);
 
   // Called when the background Android management check is done. It is
   // triggered when the second or later ARC boot timing.
   void OnBackgroundAndroidManagementChecked(
       policy::AndroidManagementClient::Result result);
 
-  // ArcSessionObserver:
-  void OnSessionReady() override;
-  void OnSessionStopped(StopReason reason) override;
+  // ArcSessionRunner::Observer:
+  void OnSessionStopped(ArcStopReason reason, bool restarting) override;
 
   std::unique_ptr<ArcSessionRunner> arc_session_runner_;
 
   // Unowned pointer. Keeps current profile.
   Profile* profile_ = nullptr;
 
   // Registrar used to monitor ARC enabled state.
   PrefChangeRegistrar pref_change_registrar_;
 
   // Whether ArcSessionManager is requested to enable (starting to run ARC
   // instance) or not.
   bool enable_requested_ = false;
 
   // Internal state machine. See also State enum class.
   State state_ = State::NOT_INITIALIZED;
   base::ObserverList<Observer> observer_list_;
-  base::ObserverList<ArcSessionObserver> arc_session_observer_list_;
   std::unique_ptr<ArcAppLauncher> playstore_launcher_;
   bool reenable_arc_ = false;
   bool provisioning_reported_ = false;
   base::OneShotTimer arc_sign_in_timer_;
 
   std::unique_ptr<ArcSupportHost> support_host_;
 
   std::unique_ptr<ArcTermsOfServiceNegotiator> terms_of_service_negotiator_;
 
   std::unique_ptr<ArcAuthContext> context_;
@@ skipping 11 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(ArcSessionManager);
 };
 
 // Outputs the stringified |state| to |os|. This is only for logging purposes.
 std::ostream& operator<<(std::ostream& os,
                          const ArcSessionManager::State& state);
 
 }  // namespace arc
 
 #endif  // CHROME_BROWSER_CHROMEOS_ARC_ARC_SESSION_MANAGER_H_