arc-bridge: Start ArcBridgeService on session start

arc-bridge: Start ArcBridgeService on session start

This change starts the ArcBridgeService when the session starts.

BUG=b:24339743
TEST=ARC instance running on login

[Luis Héctor Chávez, 2015-11-10 00:35:47]

lhchavez@chromium.org changed reviewers:
+ msw@chromium.org, oshima@chromium.org, sky@chromium.org

[Luis Héctor Chávez, 2015-11-10 00:35:48]

PTAL

This depends on https://codereview.chromium.org/1412863004/, will run `git cl
try` once that one gets checked in.

msw:
  chrome/browser/ui/DEPS
  chrome/browser/ui/startup/startup_browser_creator_impl.cc
oshima:
  chrome/browser/chromeos/chrome_browser_main_chromeos.cc
  chrome/browser/chromeos/chrome_browser_main_chromeos.h
sky:
  chrome/browser/DEPS
  chrome/browser/chromeos/chrome_browser_main_chromeos.cc
  chrome/browser/chromeos/chrome_browser_main_chromeos.h
  chrome/browser/prefs/browser_prefs.cc
  chrome/browser/ui/DEPS
  chrome/browser/ui/startup/startup_browser_creator_impl.cc

(Let me know if there's a better reviewer available for those files)

[sky, 2015-11-10 18:47:11]

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:603:
arc::ArcBridgeService::Get()->SetEnabled(
Why are you doing this here rather than during browser process startup?

[sky, 2015-11-10 18:47:15]

On 2015/11/10 00:35:48, Luis Héctor Chávez wrote:
> PTAL
> 
> This depends on https://codereview.chromium.org/1412863004/, will run `git cl
> try` once that one gets checked in.
> 
> msw:
>   chrome/browser/ui/DEPS
>   chrome/browser/ui/startup/startup_browser_creator_impl.cc
> oshima:
>   chrome/browser/chromeos/chrome_browser_main_chromeos.cc
>   chrome/browser/chromeos/chrome_browser_main_chromeos.h
> sky:
>   chrome/browser/DEPS
>   chrome/browser/chromeos/chrome_browser_main_chromeos.cc
>   chrome/browser/chromeos/chrome_browser_main_chromeos.h

You already have Oshima listed as a reviewer for the chromeos files, so I'm not
going to look at them.

>   chrome/browser/prefs/browser_prefs.cc
>   chrome/browser/ui/DEPS
>   chrome/browser/ui/startup/startup_browser_creator_impl.cc
> 
> (Let me know if there's a better reviewer available for those files)

[Luis Héctor Chávez, 2015-11-10 21:03:12]

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:603:
arc::ArcBridgeService::Get()->SetEnabled(
On 2015/11/10 18:47:11, sky wrote:
> Why are you doing this here rather than during browser process startup?

Are profile preferences available there? I had trouble finding a good spot where
this is available and happens after cryptohome had been mounted. I'm happy to
move this to a better place.

[sky, 2015-11-10 21:39:16]

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:603:
arc::ArcBridgeService::Get()->SetEnabled(
On 2015/11/10 21:03:12, Luis Héctor Chávez wrote:
> On 2015/11/10 18:47:11, sky wrote:
> > Why are you doing this here rather than during browser process startup?
> 
> Are profile preferences available there? I had trouble finding a good spot
where
> this is available and happens after cryptohome had been mounted. I'm happy to
> move this to a better place.

You may be right. But that begs another question. Shouldn't enabling arc be via
a switch rather than a pref?

[Luis Héctor Chávez, 2015-11-10 21:59:48]

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:603:
arc::ArcBridgeService::Get()->SetEnabled(
On 2015/11/10 21:39:16, sky wrote:
> On 2015/11/10 21:03:12, Luis Héctor Chávez wrote:
> > On 2015/11/10 18:47:11, sky wrote:
> > > Why are you doing this here rather than during browser process startup?
> > 
> > Are profile preferences available there? I had trouble finding a good spot
> where
> > this is available and happens after cryptohome had been mounted. I'm happy
to
> > move this to a better place.
> 
> You may be right. But that begs another question. Shouldn't enabling arc be
via
> a switch rather than a pref?

This feature will be opt-in, so we need a pref to start it.

There is also a flag in session_manager that we check to see if it is
available/supported by Chrome OS. To start, it needs to be both available and
enabled.

[sky, 2015-11-10 22:02:10]

On Tue, Nov 10, 2015 at 1:59 PM,  <lhchavez@chromium.org> wrote:
>
>
https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
>
>
https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:603:
> arc::ArcBridgeService::Get()->SetEnabled(
> On 2015/11/10 21:39:16, sky wrote:
>>
>> On 2015/11/10 21:03:12, Luis Héctor Chávez wrote:
>> > On 2015/11/10 18:47:11, sky wrote:
>> > > Why are you doing this here rather than during browser process
>
> startup?
>>
>> >
>> > Are profile preferences available there? I had trouble finding a
>
> good spot
>>
>> where
>> > this is available and happens after cryptohome had been mounted. I'm
>
> happy to
>>
>> > move this to a better place.
>
>
>> You may be right. But that begs another question. Shouldn't enabling
>
> arc be via
>>
>> a switch rather than a pref?
>
>
> This feature will be opt-in, so we need a pref to start it.

Why aren't you using about:flags for that?

  -Scott

>
> There is also a flag in session_manager that we check to see if it is
> available/supported by Chrome OS. To start, it needs to be both
> available and enabled.
>
> https://codereview.chromium.org/1416313004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-11-10 22:09:03]

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:605:
arc::ArcBridgeService::Get()->HandleStartup();
how about doing this in ShellObserver::OnLoginStateChanged? You can add observer
in chrome_shell_delegate_chromeos.cc.

[Luis Héctor Chávez, 2015-11-11 17:59:35]

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:605:
arc::ArcBridgeService::Get()->HandleStartup();
On 2015/11/10 22:09:03, oshima wrote:
> how about doing this in ShellObserver::OnLoginStateChanged? You can add
observer
> in chrome_shell_delegate_chromeos.cc.

Done.

[oshima, 2015-11-11 18:15:11]

https://codereview.chromium.org/1416313004/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:602:
arc::ArcBridgeService::Get()->SetEnabled(
can't you do this in observer?

[Luis Héctor Chávez, 2015-11-11 21:11:51]

jochen is requesting me to merge this review into
https://codereview.chromium.org/1412863004/ , so I might need to add you to the
other one.

https://codereview.chromium.org/1416313004/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1416313004/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:602:
arc::ArcBridgeService::Get()->SetEnabled(
On 2015/11/11 18:15:10, oshima wrote:
> can't you do this in observer?

Done.

[oshima, 2015-11-11 22:07:38]

cros part looks mostly good

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
move the class definition in .cc file.

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:333:
arc::ArcBridgeService::Get()->HandleStartup();
This is a future suggestion, as this requires changes in another CL. Do you
really need separate SetEnabled and HnadleStartup? Can you just call
HandleStartup (Or just StartUp) only when the feature is enabled?

[Luis Héctor Chávez, 2015-11-11 22:11:12]

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
On 2015/11/11 22:07:38, oshima wrote:
> move the class definition in .cc file.

I tried to do that, but if I put it in chrome_shell_delegate.cc, I cannot access
it in chrome_shell_delegate_chromeos.cc. If I put it in the latter, scoped_ptr
complains that the type is not complete when building
chromeos_shell_delegate.cc. I can put it in a separate .h file, if that is
desirable.

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:333:
arc::ArcBridgeService::Get()->HandleStartup();
On 2015/11/11 22:07:38, oshima wrote:
> This is a future suggestion, as this requires changes in another CL. Do you
> really need separate SetEnabled and HnadleStartup? Can you just call
> HandleStartup (Or just StartUp) only when the feature is enabled?

SetEnabled might change during runtime, so HandleStartup must be called
regardless.

[oshima, 2015-11-11 22:55:30]

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
On 2015/11/11 22:11:12, Luis Héctor Chávez wrote:
> On 2015/11/11 22:07:38, oshima wrote:
> > move the class definition in .cc file.
> 
> I tried to do that, but if I put it in chrome_shell_delegate.cc, I cannot
access
> it in chrome_shell_delegate_chromeos.cc. If I put it in the latter, scoped_ptr
> complains that the type is not complete when building
> chromeos_shell_delegate.cc. I can put it in a separate .h file, if that is
> desirable.

you can do this

chrome_shell_delegate.h:

class ChromeShellDelegate : .. {
  class ArcSessionObserver;
  scoped_ptr<ArcSessionObserver> arc_session_observer_;
}

then
chrome_shell_dlegate_chromeos.cc

class ChromeShellDelegate::ArcSessionObserver :  {
}

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:333:
arc::ArcBridgeService::Get()->HandleStartup();
On 2015/11/11 22:11:12, Luis Héctor Chávez wrote:
> On 2015/11/11 22:07:38, oshima wrote:
> > This is a future suggestion, as this requires changes in another CL. Do you
> > really need separate SetEnabled and HnadleStartup? Can you just call
> > HandleStartup (Or just StartUp) only when the feature is enabled?
> 
> SetEnabled might change during runtime, so HandleStartup must be called
> regardless.

It's not clear to me why you need both (SetEnable and HandleStartup/Shutdown),
but we can discuss it in the other CL.

[Luis Héctor Chávez, 2015-11-11 23:41:07]

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
On 2015/11/11 22:55:30, oshima wrote:
> On 2015/11/11 22:11:12, Luis Héctor Chávez wrote:
> > On 2015/11/11 22:07:38, oshima wrote:
> > > move the class definition in .cc file.
> > 
> > I tried to do that, but if I put it in chrome_shell_delegate.cc, I cannot
> access
> > it in chrome_shell_delegate_chromeos.cc. If I put it in the latter,
scoped_ptr
> > complains that the type is not complete when building
> > chromeos_shell_delegate.cc. I can put it in a separate .h file, if that is
> > desirable.
> 
> you can do this
> 
> chrome_shell_delegate.h:
> 
> class ChromeShellDelegate : .. {
>   class ArcSessionObserver;
>   scoped_ptr<ArcSessionObserver> arc_session_observer_;
> }
> 
> then
> chrome_shell_dlegate_chromeos.cc
> 
> class ChromeShellDelegate::ArcSessionObserver :  {
> }
> 
> 

That's what I tried. Compilation fails in chrome_shell_delegate.cc when trying
to create the shared_ptr due to base/memory/scoped_ptr.h:127 since the type is
not complete (in that file).

[oshima, 2015-11-12 19:09:51]

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
On 2015/11/11 23:41:07, Luis Héctor Chávez wrote:
> On 2015/11/11 22:55:30, oshima wrote:
> > On 2015/11/11 22:11:12, Luis Héctor Chávez wrote:
> > > On 2015/11/11 22:07:38, oshima wrote:
> > > > move the class definition in .cc file.
> > > 
> > > I tried to do that, but if I put it in chrome_shell_delegate.cc, I cannot
> > access
> > > it in chrome_shell_delegate_chromeos.cc. If I put it in the latter,
> scoped_ptr
> > > complains that the type is not complete when building
> > > chromeos_shell_delegate.cc. I can put it in a separate .h file, if that is
> > > desirable.
> > 
> > you can do this
> > 
> > chrome_shell_delegate.h:
> > 
> > class ChromeShellDelegate : .. {
> >   class ArcSessionObserver;
> >   scoped_ptr<ArcSessionObserver> arc_session_observer_;
> > }
> > 
> > then
> > chrome_shell_dlegate_chromeos.cc
> > 
> > class ChromeShellDelegate::ArcSessionObserver :  {
> > }
> > 
> > 
> 
> That's what I tried. Compilation fails in chrome_shell_delegate.cc when trying
> to create the shared_ptr due to base/memory/scoped_ptr.h:127 since the type is
> not complete (in that file).

I see. Too bad given that this will soon be chromeos only. The reason I was
asking is that having private class definition in a header makes little sense.
(we don't use private static member function for the same reason)

We can move this when we consolidate files.

[oshima, 2015-11-12 19:10:30]

On 2015/11/12 19:09:51, oshima wrote:
>
https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
> File chrome/browser/ui/ash/chrome_shell_delegate.h (right):
> 
>
https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
> chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
> public ash::ShellObserver {
> On 2015/11/11 23:41:07, Luis Héctor Chávez wrote:
> > On 2015/11/11 22:55:30, oshima wrote:
> > > On 2015/11/11 22:11:12, Luis Héctor Chávez wrote:
> > > > On 2015/11/11 22:07:38, oshima wrote:
> > > > > move the class definition in .cc file.
> > > > 
> > > > I tried to do that, but if I put it in chrome_shell_delegate.cc, I
cannot
> > > access
> > > > it in chrome_shell_delegate_chromeos.cc. If I put it in the latter,
> > scoped_ptr
> > > > complains that the type is not complete when building
> > > > chromeos_shell_delegate.cc. I can put it in a separate .h file, if that
is
> > > > desirable.
> > > 
> > > you can do this
> > > 
> > > chrome_shell_delegate.h:
> > > 
> > > class ChromeShellDelegate : .. {
> > >   class ArcSessionObserver;
> > >   scoped_ptr<ArcSessionObserver> arc_session_observer_;
> > > }
> > > 
> > > then
> > > chrome_shell_dlegate_chromeos.cc
> > > 
> > > class ChromeShellDelegate::ArcSessionObserver :  {
> > > }
> > > 
> > > 
> > 
> > That's what I tried. Compilation fails in chrome_shell_delegate.cc when
trying
> > to create the shared_ptr due to base/memory/scoped_ptr.h:127 since the type
is
> > not complete (in that file).
> 
> I see. Too bad given that this will soon be chromeos only. The reason I was
> asking is that having private class definition in a header makes little sense.
> (we don't use private static member function for the same reason)
> 
> We can move this when we consolidate files.

lgtm cros bits.

[sky, 2015-11-12 22:06:53]

LGTM

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
Move this above other fields. That is, keep all fields grouped together and
don't mix other types in between them.

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:236:
arc_session_observer_.reset();
Please document why this needs to be shutdown here rather than the scoped_ptr.
It isn't obvious.

[Luis Héctor Chávez, 2015-11-12 22:21:38]

The changes will be reflected in https://codereview.chromium.org/1412863004/

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.h (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.h:112: class ArcSessionObserver :
public ash::ShellObserver {
On 2015/11/12 22:06:53, sky wrote:
> Move this above other fields. That is, keep all fields grouped together and
> don't mix other types in between them.

Done.

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1416313004/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:236:
arc_session_observer_.reset();
On 2015/11/12 22:06:53, sky wrote:
> Please document why this needs to be shutdown here rather than the scoped_ptr.
> It isn't obvious.

Done.