chrome/browser/chromeos/arc/arc_session_manager.h - Issue 2720303002: Do nothing on OnSessionStopped if ARC is being restarted.

Unified Diff: chrome/browser/chromeos/arc/arc_session_manager.h

Issue 2720303002: Do nothing on OnSessionStopped if ARC is being restarted. (Closed)

Patch Set: Get rid of ArcSessionObserver Created 3 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Index: chrome/browser/chromeos/arc/arc_session_manager.h

diff --git a/chrome/browser/chromeos/arc/arc_session_manager.h b/chrome/browser/chromeos/arc/arc_session_manager.h

index fba0655f2cd7932501d2c4b18af762c6ef266a8b..0407b7f9fc35b2e35f7b7d50615a245ab505c318 100644

--- a/chrome/browser/chromeos/arc/arc_session_manager.h

+++ b/chrome/browser/chromeos/arc/arc_session_manager.h

@@ -15,7 +15,8 @@

#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"

@@ -31,13 +32,12 @@ 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:

@@ -85,10 +85,9 @@ class ArcSessionManager : public ArcSessionObserver,

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) {}

@@ -96,9 +95,18 @@ class ArcSessionManager : public ArcSessionObserver,

// 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 prac

hidehiko 2017/03/01 17:28:02 As for your example, no, it won't. However, it is

Yusuke Sato 2017/03/01 17:40:24 Sorry if my comment above was not clear enough, bu

On 2017/03/01 17:28:02, hidehiko wrote: > On 2017/03/01 15:51:01, Yusuke Sato wrote: > > 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? > > 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?

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, consider

On 2017/03/01 17:40:24, Yusuke Sato wrote: > On 2017/03/01 17:28:02, hidehiko wrote: > > On 2017/03/01 15:51:01, Yusuke Sato wrote: > > > 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? > > > > 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? > > 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).

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 gui

On 2017/03/01 17:49:41, hidehiko wrote: > On 2017/03/01 17:40:24, Yusuke Sato wrote: > > On 2017/03/01 17:28:02, hidehiko wrote: > > > On 2017/03/01 15:51:01, Yusuke Sato wrote: > > > > 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? > > > > > > 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? > > > > 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). > > 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.

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

hidehiko 2017/03/01 18:43:02 SG. Thanks!

On 2017/03/01 18:26:52, Luis Héctor Chávez wrote: > On 2017/03/01 17:49:41, hidehiko wrote: > > On 2017/03/01 17:40:24, Yusuke Sato wrote: > > > On 2017/03/01 17:28:02, hidehiko wrote: > > > > On 2017/03/01 15:51:01, Yusuke Sato wrote: > > > > > 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? > > > > > > > > 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? > > > > > > 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). > > > > 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. > > I'll add this for posterity in the ARC++ style guide.

SG. Thanks!

};

explicit ArcSessionManager(

@@ -145,12 +153,6 @@ class ArcSessionManager : public ArcSessionObserver,

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;

@@ -261,9 +263,8 @@ class ArcSessionManager : public ArcSessionObserver,

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_;

@@ -280,7 +281,6 @@ class ArcSessionManager : public ArcSessionObserver,

// 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;

« no previous file with comments | « no previous file | chrome/browser/chromeos/arc/arc_session_manager.cc » ('j') | components/arc/arc_session.h » ('J')