Remove ArcSessionManager::Observer::OnArcBridgeShutdown.

Remove ArcSessionManager::Observer::OnArcBridgeShutdown.

The method is;
- Used only in one browser test.
- It is not called on ArcBridgeShutdown, but on beginning of
  ARC session stopping.
So, its name is not what it is.
Because the browser test can be rewritten with another callback,
so this CL removes the method.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2692993007
Cr-Commit-Position: refs/heads/master@{#450856}
Committed: https://chromium.googlesource.com/chromium/src/+/8983a9ebba43f3935f7e1481022f4865e6c5c5bd

[hidehiko, 2017-02-15 13:51:49]

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

[commit-bot: I haz the power, 2017-02-15 13:52:02]

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-02-15 14:29:18]

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

[commit-bot: I haz the power, 2017-02-15 14:29:19]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-15 14:39:27]

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

[hidehiko, 2017-02-15 14:39:27]

PTAL.

[Luis Héctor Chávez, 2017-02-15 18:44:50]

lgtm

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

https://codereview.chromium.org/2692993007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:81:
base::AutoReset<base::RunLoop*> reset(&run_loop_, &run_loop);
nice, this looks _much_ cleaner :)

https://codereview.chromium.org/2692993007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:88: if
(run_loop_)
nit:

if (!enabled) {
  DCHECK(run_loop_);
  run_loop_->Quit();
}

[hidehiko, 2017-02-15 18:51:53]

Thank you for review. Quick question.

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

https://codereview.chromium.org/2692993007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:81:
base::AutoReset<base::RunLoop*> reset(&run_loop_, &run_loop);
On 2017/02/15 18:44:50, Luis Héctor Chávez wrote:
> nice, this looks _much_ cleaner :)

Thanks :-).

https://codereview.chromium.org/2692993007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:88: if
(run_loop_)
On 2017/02/15 18:44:50, Luis Héctor Chávez wrote:
> nit:
> 
> if (!enabled) {
>   DCHECK(run_loop_);
>   run_loop_->Quit();
> }

It changes the semantics (from the brief document of this class), I think.
Do you mean we should create ArcPlayStoreDisabledWaiter?

[Luis Héctor Chávez, 2017-02-15 19:09:44]

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

https://codereview.chromium.org/2692993007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:88: if
(run_loop_)
On 2017/02/15 18:51:53, hidehiko wrote:
> On 2017/02/15 18:44:50, Luis Héctor Chávez wrote:
> > nit:
> > 
> > if (!enabled) {
> >   DCHECK(run_loop_);
> >   run_loop_->Quit();
> > }
> 
> It changes the semantics (from the brief document of this class), I think.
> Do you mean we should create ArcPlayStoreDisabledWaiter?

So previously the semantics for this class was to wait until ARC session was
shut down. Now this listens for the play store state to change in any direction
(since you're not inspecting the value of |enabled|), which is not compatible
with what was there previously. At the very least this should limit to waiting
until the state goes from enabled to disabled. 

Naming the class ArcPlayStoreDisabledWaiter seems like a good direction, and
maybe also changing the class comment to "Waits for the "arc.enabled" preference
to change from enabled to disabled".

[hidehiko, 2017-02-15 19:52:01]

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

[hidehiko, 2017-02-15 19:52:43]

Thank you for review. PTAL.

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

https://codereview.chromium.org/2692993007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc:88: if
(run_loop_)
On 2017/02/15 19:09:44, Luis Héctor Chávez wrote:
> On 2017/02/15 18:51:53, hidehiko wrote:
> > On 2017/02/15 18:44:50, Luis Héctor Chávez wrote:
> > > nit:
> > > 
> > > if (!enabled) {
> > >   DCHECK(run_loop_);
> > >   run_loop_->Quit();
> > > }
> > 
> > It changes the semantics (from the brief document of this class), I think.
> > Do you mean we should create ArcPlayStoreDisabledWaiter?
> 
> So previously the semantics for this class was to wait until ARC session was
> shut down. Now this listens for the play store state to change in any
direction
> (since you're not inspecting the value of |enabled|), which is not compatible
> with what was there previously. At the very least this should limit to waiting
> until the state goes from enabled to disabled. 
> 
> Naming the class ArcPlayStoreDisabledWaiter seems like a good direction, and
> maybe also changing the class comment to "Waits for the "arc.enabled"
preference
> to change from enabled to disabled".

Ok, thank you for detailed explanation.
Done.

[commit-bot: I haz the power, 2017-02-15 19:53:11]

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

[Luis Héctor Chávez, 2017-02-15 20:34:00]

lgtm++!

[commit-bot: I haz the power, 2017-02-15 21:35:20]

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

[commit-bot: I haz the power, 2017-02-15 21:35:21]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-16 01:34:26]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2017-02-16 01:35:44]

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-02-16 03:23:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487208866490640,
"parent_rev": "d8b91eaadcd63c7ece9def5ed795b99256c8188a", "commit_rev":
"8983a9ebba43f3935f7e1481022f4865e6c5c5bd"}

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

Description was changed from

==========
Remove ArcSessionManager::Observer::OnArcBridgeShutdown.

The method is;
- Used only in one browser test.
- It is not called on ArcBridgeShutdown, but on beginning of
  ARC session stopping.
So, its name is not what it is.
Because the browser test can be rewritten with another callback,
so this CL removes the method.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========

to

==========
Remove ArcSessionManager::Observer::OnArcBridgeShutdown.

The method is;
- Used only in one browser test.
- It is not called on ArcBridgeShutdown, but on beginning of
  ARC session stopping.
So, its name is not what it is.
Because the browser test can be rewritten with another callback,
so this CL removes the method.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2692993007
Cr-Commit-Position: refs/heads/master@{#450856}
Committed:
https://chromium.googlesource.com/chromium/src/+/8983a9ebba43f3935f7e1481022f...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8983a9ebba43f3935f7e1481022f...