Do nothing on OnSessionStopped if ARC is being restarted.

Do nothing on OnSessionStopped if ARC is being restarted.

ArcSessionManager::OnSessionStopped() implements operations
running on ARC session is stopped.
However, ArcSessionRunner supports automatic restarting,
and the function does not expect such cases, so that state
machine gets into a wrong state.

Along with the behavior change, unifies the observers
for ArcSessionManager.

BUG=657687
BUG=636218
BUG=633258
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2720303002
Cr-Commit-Position: refs/heads/master@{#454007}
Committed: https://chromium.googlesource.com/chromium/src/+/ae82e3ad7862628f263303d750ee7e1ac5775219

[hidehiko, 2017-02-28 16:23:41]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 16:24:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hidehiko, 2017-02-28 16:33:47]

hidehiko@chromium.org changed reviewers:
+ lhchavez@chromium.org

[hidehiko, 2017-02-28 16:33:48]

PTAL.

Note: this is also a part of fixing state machine in ArcSessionManager.

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:154: if
(arc_sign_in_timer_.IsRunning())
Note: this case is the biggest change in this CL.
If ARC is somehow crashed and then being restarted in this case, restarting will
get priority with this change.
Even in the crash loop case, as a last resort, timeout should be fired.

Another approach would be, if the timer is running,
stop the ArcSessionRunner, even in restarting case.

Luis, WDYT?

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:157: if
(profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
Note: in case of restarting, we do not want to enter data removal state.

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:173:
observer.OnArcSessionStopped(reason);
Note: The only use case of this is ArcBootNotification, which sees LOW_DISK
error only.
On LOW_DISK error, restarting should always false (because it is a part of
container boot sequence), so this should be practically no change.

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:605: observer.OnSessionStopped(reason, false /*
restarting */);
Note: Alternative approach would be splitting ArcSession's Observer and
ArcSessionRunner's Observer.

At the moment, I think it is still ok to share. WDYT?

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
File components/arc/arc_session_runner.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
components/arc/arc_session_runner.cc:195: restarting = true;  // Overwrite the
passing argument to true.
Note: another approach will be; not calling OnSessionStopped on restarting.
However, at the moment it makes testing difficult, so I kept it.

[commit-bot: I haz the power, 2017-02-28 17:03:01]

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

[commit-bot: I haz the power, 2017-02-28 17:03:01]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2017-02-28 21:07:16]

yusukes@chromium.org changed reviewers:
+ yusukes@chromium.org

[Yusuke Sato, 2017-02-28 21:07:17]

drive-by

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (left):

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:136: // Adds or removes
ArcSessionObservers.
nice.

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:88: class Observer {
nit: Probably it's a good idea to have a short comment for the class now. Who
should use the observer and when?

As you know, similar events are relayed from ArcSession to ArcSessionRunner to
ArcSessionManager to its observers (with slightly different semantics, and now
with slightly different observer interfaces too), which is kind of confusing
without comments.

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:92: virtual ~Observer() =
default;
nit: (while you're at it) isn't it better to make this pure virtual to prevent
Observer from being instantiated?

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:605: observer.OnSessionStopped(reason, false /*
restarting */);
On 2017/02/28 16:33:48, hidehiko wrote:
> Note: Alternative approach would be splitting ArcSession's Observer and
> ArcSessionRunner's Observer.
> 
> At the moment, I think it is still ok to share. WDYT?

I'd actually prefer to split the interface.

* ASM now completely switched to its own observer interface. There seems to be
no reason for ASR to do the same.
* Removing the second parameter makes it clearer that AS never restarts the
instance by itself.
* If you split the observer interface, each interface can have better document
(each interface can mention more things its observers can assume, for example.)

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
File components/arc/arc_session_observer.h (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
components/arc/arc_session_observer.h:32: virtual ~ArcSessionObserver() =
default;
nit: same

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
File components/arc/arc_session_runner.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
components/arc/arc_session_runner.cc:165: // Because ArcSession won't restart
automatically by itself, |restarting|
Then you can remove L165-167 :)

[Yusuke Sato, 2017-02-28 23:13:54]

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:605: observer.OnSessionStopped(reason, false /*
restarting */);
On 2017/02/28 21:07:17, Yusuke Sato wrote:
> On 2017/02/28 16:33:48, hidehiko wrote:
> > Note: Alternative approach would be splitting ArcSession's Observer and
> > ArcSessionRunner's Observer.
> > 
> > At the moment, I think it is still ok to share. WDYT?
> 
> I'd actually prefer to split the interface.
> 
> * ASM now completely switched to its own observer interface. There seems to be
> no reason for ASR to do the same.

Sorry there was a typo. I mean, I'd like AS and ASR to use different observer
interfaces.

> * Removing the second parameter makes it clearer that AS never restarts the
> instance by itself.
> * If you split the observer interface, each interface can have better document
> (each interface can mention more things its observers can assume, for
example.)
> 
>

[hidehiko, 2017-03-01 06:29:35]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 06:29:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 07:22:45]

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

[commit-bot: I haz the power, 2017-03-01 07:22:46]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-03-01 08:55:17]

Patchset #3 (id:40001) has been deleted

[hidehiko, 2017-03-01 09:09:57]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[hidehiko, 2017-03-01 09:10:05]

Thank you for review. PTAL.

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (left):

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:136: // Adds or removes
ArcSessionObservers.
On 2017/02/28 21:07:16, Yusuke Sato wrote:
> nice.

:-)

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:88: class Observer {
On 2017/02/28 21:07:17, Yusuke Sato wrote:
> nit: Probably it's a good idea to have a short comment for the class now. Who
> should use the observer and when?
> 
> As you know, similar events are relayed from ArcSession to ArcSessionRunner to
> ArcSessionManager to its observers (with slightly different semantics, and now
> with slightly different observer interfaces too), which is kind of confusing
> without comments.
> 

Done.

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:92: virtual ~Observer() =
default;
On 2017/02/28 21:07:16, Yusuke Sato wrote:
> nit: (while you're at it) isn't it better to make this pure virtual to prevent
> Observer from being instantiated?

For that purpose, protected ctor looks more straightforward.
Also, I moved dtor to protected (with removing virtual) because we do not expect
the class is removed via Observer interface.
cf) http://www.gotw.ca/publications/mill18.htm

WDYT?

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session.cc
File components/arc/arc_session.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session....
components/arc/arc_session.cc:605: observer.OnSessionStopped(reason, false /*
restarting */);
On 2017/02/28 23:13:53, Yusuke Sato wrote:
> On 2017/02/28 21:07:17, Yusuke Sato wrote:
> > On 2017/02/28 16:33:48, hidehiko wrote:
> > > Note: Alternative approach would be splitting ArcSession's Observer and
> > > ArcSessionRunner's Observer.
> > > 
> > > At the moment, I think it is still ok to share. WDYT?
> > 
> > I'd actually prefer to split the interface.
> > 
> > * ASM now completely switched to its own observer interface. There seems to
be
> > no reason for ASR to do the same.
> 
> Sorry there was a typo. I mean, I'd like AS and ASR to use different observer
> interfaces.
> 
> > * Removing the second parameter makes it clearer that AS never restarts the
> > instance by itself.
> > * If you split the observer interface, each interface can have better
document
> > (each interface can mention more things its observers can assume, for
> example.)
> > 
> > 
> 

So, done.
Split the observer into two: ArcSession::Observer and
ArcSessionRunner::Observer.
To share the StopReason easily, I introduced a new file arc_stop_reason.h.

Note that, if we want to split the dependency between headers, we can define
those observer interfaces in standalone files, e.g. arc_session_observer.h, and
arc_session_runner_observer.h etc. Though, at the moment, it looks not needed,
IMHO, so I defined them as nested class.

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
File components/arc/arc_session_observer.h (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
components/arc/arc_session_observer.h:32: virtual ~ArcSessionObserver() =
default;
On 2017/02/28 21:07:17, Yusuke Sato wrote:
> nit: same

Done.

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
File components/arc/arc_session_runner.cc (right):

https://codereview.chromium.org/2720303002/diff/1/components/arc/arc_session_...
components/arc/arc_session_runner.cc:165: // Because ArcSession won't restart
automatically by itself, |restarting|
On 2017/02/28 21:07:17, Yusuke Sato wrote:
> Then you can remove L165-167 :)

Done.

[commit-bot: I haz the power, 2017-03-01 09:10:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 09:58:11]

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

[commit-bot: I haz the power, 2017-03-01 09:58:11]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-03-01 11:19:03]

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:92: virtual ~Observer() =
default;
On 2017/03/01 09:10:04, hidehiko wrote:
> On 2017/02/28 21:07:16, Yusuke Sato wrote:
> > nit: (while you're at it) isn't it better to make this pure virtual to
prevent
> > Observer from being instantiated?
> 
> For that purpose, protected ctor looks more straightforward.
> Also, I moved dtor to protected (with removing virtual) because we do not
expect
> the class is removed via Observer interface.
> cf) http://www.gotw.ca/publications/mill18.htm
> 
> WDYT?

FYI: Found a recent discussion.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5GIek4TYiAw

[Yusuke Sato, 2017-03-01 15:51:02]

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:109: ~Observer() = default;
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?

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
File components/arc/arc_session.h (left):

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
components/arc/arc_session.h:20: class ArcSessionObserver;
'git grep ArcSessionObserver' please, just to make sure no obsolete comments are
left.

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
File components/arc/arc_session.h (right):

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
components/arc/arc_session.h:30: class Observer {
nit: comment on who should use it?

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
File components/arc/arc_session_runner.h (right):

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
components/arc/arc_session_runner.h:23: class Observer {
same?

[hidehiko, 2017-03-01 17:28:03]

Thank you for review. PTAL.

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:109: ~Observer() = default;
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?

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
File components/arc/arc_session.h (left):

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
components/arc/arc_session.h:20: class ArcSessionObserver;
On 2017/03/01 15:51:01, Yusuke Sato wrote:
> 'git grep ArcSessionObserver' please, just to make sure no obsolete comments
are
> left.
> 

Yes, I had done, and no ArcSessionObserver should be remained.

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
File components/arc/arc_session.h (right):

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
components/arc/arc_session.h:30: class Observer {
On 2017/03/01 15:51:01, Yusuke Sato wrote:
> nit: comment on who should use it?

I added a brief comment, thought I wonder if it is what you want...
Any advice is welcome.

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
File components/arc/arc_session_runner.h (right):

https://codereview.chromium.org/2720303002/diff/60001/components/arc/arc_sess...
components/arc/arc_session_runner.h:23: class Observer {
On 2017/03/01 15:51:01, Yusuke Sato wrote:
> same?

Done.

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

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:109: ~Observer() = default;
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]

PTAL.

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:109: ~Observer() = default;
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.

[Yusuke Sato, 2017-03-01 17:55:44]

thanks, lgtm

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

sorry for the delay D: couple of nits, but otherwise lgtm.

Thanks yusukes@ for picking this up!

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:109: ~Observer() = default;
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.

https://codereview.chromium.org/2720303002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_session_manager.h:96: virtual void
OnArcInitialStart() {}
nit: maybe OnArcSessionInitialStart()? for consistency with below.

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
File components/arc/arc_session_runner.cc (right):

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
components/arc/arc_session_runner.cc:172: bool restarting = false;
maybe make this

const bool restarting = (state == State::RUNNING || ...);
if (restarting) {
  ...
}

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
File components/arc/arc_session_runner_unittest.cc (right):

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:25: class DummyObserver : public
ArcSessionRunner::Observer {
nit: maybe s/DummyObserver/DoNothingObserver/. (for consistency with
https://cs.chromium.org/chromium/src/base/bind_helpers.h?q=DoNothing+package:...
)

[hidehiko, 2017-03-01 18:43:03]

Thank you for review. Submitting.

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.h:109: ~Observer() = default;
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!

https://codereview.chromium.org/2720303002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2720303002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_session_manager.h:96: virtual void
OnArcInitialStart() {}
On 2017/03/01 18:26:52, Luis Héctor Chávez wrote:
> nit: maybe OnArcSessionInitialStart()? for consistency with below.

Let me keep this as is in this CL to focus on session stopped handling.
Will work on it in another CL.

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
File components/arc/arc_session_runner.cc (right):

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
components/arc/arc_session_runner.cc:172: bool restarting = false;
On 2017/03/01 18:26:53, Luis Héctor Chávez wrote:
> maybe make this
> 
> const bool restarting = (state == State::RUNNING || ...);
> if (restarting) {
>   ...
> }

Done.

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
File components/arc/arc_session_runner_unittest.cc (right):

https://codereview.chromium.org/2720303002/diff/100001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:25: class DummyObserver : public
ArcSessionRunner::Observer {
On 2017/03/01 18:26:53, Luis Héctor Chávez wrote:
> nit: maybe s/DummyObserver/DoNothingObserver/. (for consistency with
>
https://cs.chromium.org/chromium/src/base/bind_helpers.h?q=DoNothing+package:...
> )

Hmm, I imagined there is some common name pattern in Chromium for this case, but
I couldn't find.
No DoNothingObserver, no DummyObserver, no EmptyObserver, no StubObserver...
Something else...?

Anyway, I do not have any strong preference here now, so renamed.

[hidehiko, 2017-03-01 18:43:25]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-03-01 18:43:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2720303002/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-03-01 18:43:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 20:05:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488393805478450,
"parent_rev": "b2d4d4ba555e4658e5b2ea2ba4cbd9b95ac84442", "commit_rev":
"ae82e3ad7862628f263303d750ee7e1ac5775219"}

[commit-bot: I haz the power, 2017-03-01 20:06:20]

Description was changed from

==========
Do nothing on OnSessionStopped if ARC is being restarted.

ArcSessionManager::OnSessionStopped() implements operations
running on ARC session is stopped.
However, ArcSessionRunner supports automatic restarting,
and the function does not expect such cases, so that state
machine gets into a wrong state.

Along with the behavior change, unifies the observers
for ArcSessionManager.

BUG=657687
BUG=636218
BUG=633258
TEST=Ran bots.
==========

to

==========
Do nothing on OnSessionStopped if ARC is being restarted.

ArcSessionManager::OnSessionStopped() implements operations
running on ARC session is stopped.
However, ArcSessionRunner supports automatic restarting,
and the function does not expect such cases, so that state
machine gets into a wrong state.

Along with the behavior change, unifies the observers
for ArcSessionManager.

BUG=657687
BUG=636218
BUG=633258
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2720303002
Cr-Commit-Position: refs/heads/master@{#454007}
Committed:
https://chromium.googlesource.com/chromium/src/+/ae82e3ad7862628f263303d750ee...
==========

[commit-bot: I haz the power, 2017-03-01 20:06:21]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ae82e3ad7862628f263303d750ee...