arc-bridge: Add the ARC Bridge Service

arc-bridge: Add the ARC Bridge Service

This adds the ARC bridge functionality and starts an ARC instance when
the user logs in.

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

Committed: https://crrev.com/c951edfb84c3439da64cc2d97d6e7b495323d349
Cr-Commit-Position: refs/heads/master@{#359982}

[Luis Héctor Chávez, 2015-10-23 02:37:06]

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

[Luis Héctor Chávez, 2015-10-23 02:37:07]

PTAL

This depends on:

https://codereview.chromium.org/1412863004/
https://codereview.chromium.org/1424503002/

Also let me know if this is the best place to put this.

[satorux1, 2015-10-23 06:10:58]

satorux@chromium.org changed reviewers:
+ oshima@chromium.org

[satorux1, 2015-10-23 06:10:59]

took a quick look. +oshima for the startup change

https://codereview.chromium.org/1412863004/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:607: #endif
I'm not sure if this is the right place. Let's ask oshima@ who's an expert in
Chrome's startup code. :)

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:15: const char kArcBridgeSocketPath[]
= "/home/chronos/ArcBridge/bridge.sock";
should this be shared between here and the chromeos side? Then you can put the
constant in third_party/cros_system_api.

That's a bit tedious so maybe you can just add a TODO here.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:38:
chromeos::DBusThreadManager::Get()->GetArcInstanceClient()->StartInstance(
Is the D-Bus client code checked-in? I'm reviewing from a laptop without source
code, so I cannot check.

Anyway, I was curious what instance was started here.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:40:
base::Bind(&ArcBridgeService::OnInstanceStarted, base::Unretained(this)));
Is it safe to base::Unretained() here? Please document. 

In general, base::Unretained() shouldn't be used, because it's tricky to
guarantee that |this| is alive when the callback is run.

However, this seems safe because this is a keyed service instance.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:46: channel_->Close();
channel_ is defined in the parent class right? Maybe give it a more descriptive
name like ipc_channel_?

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:51:
base::Bind(&ArcBridgeService::OnInstanceStopped, base::Unretained(this)));
ditto. add a comment about base::Unretained()?

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:18: class ArcBridgeService : public
KeyedService, public BridgeHostEndpoint {
class comment is missing. please describe what this class does and what is for,
etc.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:47: // Add or remove observers.
nit: Adds or removes

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:57: bool IsRunning() const { return
running_; }
function comment is missing.

// Returns true if the service is running

would be a typical function comment, but would be nice to explain what it means
to be "running".

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:76: bool running_;
enabled_, connected_, running_ sound similar to me. Could you add comments and
explain the differences?

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service_unittest.cc:11: #include
"testing/gmock/include/gmock/gmock.h"
gmock should generally be avoided in Chrome. Let's not use it.

https://codereview.chromium.org/1412863004/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/chromeos.gyp#newco...
chromeos/chromeos.gyp:19: 'arc/libarcbridge/arc_message_generator.cc',
the last two lines should belong to the other  patch?

[oshima, 2015-10-23 20:29:45]

is there a design doc I can look at?

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/DEPS
File chromeos/arc/bridge/DEPS (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/DEPS#ne...
chromeos/arc/bridge/DEPS:3: "+chrome/browser/profiles",
chromeos/ should not depend on chrome nor content

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:8: #include
"chrome/browser/profiles/profile.h"
ditto

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:14: class Profile;
chromeos/ should not depend on Profile

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service_factory.h (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service_factory.h:20: class
ArcBridgeServiceFactory : public BrowserContextKeyedServiceFactory {
Does this have to be BrowserContext? can't you just use
KeyedService/SupportsUserData?

[satorux1, 2015-10-26 05:24:58]

satorux@chromium.org changed reviewers:
+ nya@chromium.org

[satorux1, 2015-10-26 05:24:58]

Adding nya@ based on the ground I mentioned at
https://codereview.chromium.org/1424503002/#msg8

[Shuhei Takahashi, 2015-10-26 11:05:32]

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

[Shuhei Takahashi, 2015-10-26 11:05:33]

+hidehiko

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

After discussing with oshima@, moved the implementation to components/arc where
it makes more sense.

https://codereview.chromium.org/1412863004/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:607: #endif
On 2015/10/23 06:10:58, satorux1 wrote:
> I'm not sure if this is the right place. Let's ask oshima@ who's an expert in
> Chrome's startup code. :)

Ack. He expressed that this is an okay place.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/DEPS
File chromeos/arc/bridge/DEPS (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/DEPS#ne...
chromeos/arc/bridge/DEPS:3: "+chrome/browser/profiles",
On 2015/10/23 20:29:45, oshima wrote:
> chromeos/ should not depend on chrome nor content

Moved to components/arc/, so this is not needed anymore.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:8: #include
"chrome/browser/profiles/profile.h"
On 2015/10/23 20:29:45, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:15: const char kArcBridgeSocketPath[]
= "/home/chronos/ArcBridge/bridge.sock";
On 2015/10/23 06:10:58, satorux1 wrote:
> should this be shared between here and the chromeos side? Then you can put the
> constant in third_party/cros_system_api.
> 
> That's a bit tedious so maybe you can just add a TODO here.

There will be no sharing of this constant. It will be passed as an explicit
parameter to StartInstance.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:38:
chromeos::DBusThreadManager::Get()->GetArcInstanceClient()->StartInstance(
On 2015/10/23 06:10:58, satorux1 wrote:
> Is the D-Bus client code checked-in? I'm reviewing from a laptop without
source
> code, so I cannot check.
> 
> Anyway, I was curious what instance was started here.

Not yet, but here's its review: https://codereview.chromium.org/1421023002/

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:40:
base::Bind(&ArcBridgeService::OnInstanceStarted, base::Unretained(this)));
On 2015/10/23 06:10:58, satorux1 wrote:
> Is it safe to base::Unretained() here? Please document. 
> 
> In general, base::Unretained() shouldn't be used, because it's tricky to
> guarantee that |this| is alive when the callback is run.
> 
> However, this seems safe because this is a keyed service instance.

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:46: channel_->Close();
On 2015/10/23 06:10:58, satorux1 wrote:
> channel_ is defined in the parent class right? Maybe give it a more
descriptive
> name like ipc_channel_?

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.cc:51:
base::Bind(&ArcBridgeService::OnInstanceStopped, base::Unretained(this)));
On 2015/10/23 06:10:58, satorux1 wrote:
> ditto. add a comment about base::Unretained()?

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:14: class Profile;
On 2015/10/23 20:29:45, oshima wrote:
> chromeos/ should not depend on Profile

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:18: class ArcBridgeService : public
KeyedService, public BridgeHostEndpoint {
On 2015/10/23 06:10:58, satorux1 wrote:
> class comment is missing. please describe what this class does and what is
for,
> etc.

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:47: // Add or remove observers.
On 2015/10/23 06:10:58, satorux1 wrote:
> nit: Adds or removes

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:57: bool IsRunning() const { return
running_; }
On 2015/10/23 06:10:58, satorux1 wrote:
> function comment is missing.
> 
> // Returns true if the service is running
> 
> would be a typical function comment, but would be nice to explain what it
means
> to be "running".

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service.h:76: bool running_;
On 2015/10/23 06:10:58, satorux1 wrote:
> enabled_, connected_, running_ sound similar to me. Could you add comments and
> explain the differences?

Changed to an enum.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service_factory.h (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service_factory.h:20: class
ArcBridgeServiceFactory : public BrowserContextKeyedServiceFactory {
On 2015/10/23 20:29:45, oshima wrote:
> Does this have to be BrowserContext? can't you just use
> KeyedService/SupportsUserData?

Changed the ArcBridgeService into a KeyedService. Kept this as BrowserContext
since it was moved to components/arc/ and other classes there use this.

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
File chromeos/arc/bridge/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/arc/bridge/arc_bri...
chromeos/arc/bridge/arc_bridge_service_unittest.cc:11: #include
"testing/gmock/include/gmock/gmock.h"
On 2015/10/23 06:10:58, satorux1 wrote:
> gmock should generally be avoided in Chrome. Let's not use it.

Done.

https://codereview.chromium.org/1412863004/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/1412863004/diff/1/chromeos/chromeos.gyp#newco...
chromeos/chromeos.gyp:19: 'arc/libarcbridge/arc_message_generator.cc',
On 2015/10/23 06:10:58, satorux1 wrote:
> the last two lines should belong to the other  patch?

Probably. I'll move it.

[satorux1, 2015-10-28 05:45:23]

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:23: const char kArcBridgeSocketPath[] =
"/home/chronos/ArcBridge/bridge.sock";
Please use const FilePath::CharType and FILE_PATH_LITERAL to follow Chrome's
common practice

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:25: bool EnsureParentDirectoryPresent(const
std::string& socket_path) {
how about taking base::FilePath? 

The caller should pass base::FilePath(kArcBridgeSocketPath)

also function comment is missing. please check other places too

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:30: bool SocketPermissions(const
std::string& socket_path) {
Maybe SetSocketPermissions? std::string -> base::FilePath.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:72: base::Unretained(this),
kArcBridgeSocketPath));
How about using WeakPtrFactory and remove use of Unretained() from this class?
As mentioned elsewhere, Unretained() should generally be avoided, and Chrome's
common practice is to use WeakPtr for the 2nd callback for PostTaskAndRely,
like:

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: base::ScopedFD fd) {
on which thread should this be called? Add a DCHECK ?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:62: ArcBridgeService(
Explicit is missing?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:80: // Add or remove observers.
nit: Adds or removes

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:90: // Request for registering an input
device on the ARC instance.
nit: Requests

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:107: bool Connect(const IPC::ChannelHandle&
handle, IPC::Channel::Mode mode);
If this function is named like ConnectForTesting(), the presubmit check will
warn if the function is called outside of tests.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:109: // Finish connecting after setting
socket permissions.
nit: Finishes

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:117: // Change the current state and notify
all observers.
nit: Changes

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:61: ArcBridgeTest() {}
Initialize ready_ here with ready_(false)? unitialized members should generally
be avoided.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:94: service_.reset();
do you need to reset fake_sender_ and service_ here? these are scoped_ptr, so
it's probably not necessary

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_pre...
File components/arc/arc_pref_names.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_pre...
components/arc/arc_pref_names.cc:10: // Whether using ARC is enabled.
maybe drop 'using'?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_pre...
components/arc/arc_pref_names.cc:12: "arc.enabled";
make it one line?

[Luis Héctor Chávez, 2015-10-28 17:19:15]

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:23: const char kArcBridgeSocketPath[] =
"/home/chronos/ArcBridge/bridge.sock";
On 2015/10/28 05:45:22, satorux1 wrote:
> Please use const FilePath::CharType and FILE_PATH_LITERAL to follow Chrome's
> common practice

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:25: bool EnsureParentDirectoryPresent(const
std::string& socket_path) {
On 2015/10/28 05:45:22, satorux1 wrote:
> how about taking base::FilePath? 
> 
> The caller should pass base::FilePath(kArcBridgeSocketPath)
> 
> also function comment is missing. please check other places too

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:30: bool SocketPermissions(const
std::string& socket_path) {
On 2015/10/28 05:45:22, satorux1 wrote:
> Maybe SetSocketPermissions? std::string -> base::FilePath.

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:72: base::Unretained(this),
kArcBridgeSocketPath));
On 2015/10/28 05:45:22, satorux1 wrote:
> How about using WeakPtrFactory and remove use of Unretained() from this class?
> As mentioned elsewhere, Unretained() should generally be avoided, and Chrome's
> common practice is to use WeakPtr for the 2nd callback for PostTaskAndRely,
> like:
> 

All other KeyedServices I saw used Unretained(), but done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: base::ScopedFD fd) {
On 2015/10/28 05:45:22, satorux1 wrote:
> on which thread should this be called? Add a DCHECK ?

It doesn't matter, IPC::ChannelProxy has a thread-safe Send.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:62: ArcBridgeService(
On 2015/10/28 05:45:23, satorux1 wrote:
> Explicit is missing?

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:80: // Add or remove observers.
On 2015/10/28 05:45:23, satorux1 wrote:
> nit: Adds or removes

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:90: // Request for registering an input
device on the ARC instance.
On 2015/10/28 05:45:23, satorux1 wrote:
> nit: Requests

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:107: bool Connect(const IPC::ChannelHandle&
handle, IPC::Channel::Mode mode);
On 2015/10/28 05:45:23, satorux1 wrote:
> If this function is named like ConnectForTesting(), the presubmit check will
> warn if the function is called outside of tests.

The function is used outside of tests in arc_bridge_service.cc, so I can't do
that. Instead I changed the comment.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:109: // Finish connecting after setting
socket permissions.
On 2015/10/28 05:45:23, satorux1 wrote:
> nit: Finishes

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:117: // Change the current state and notify
all observers.
On 2015/10/28 05:45:23, satorux1 wrote:
> nit: Changes

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:61: ArcBridgeTest() {}
On 2015/10/28 05:45:23, satorux1 wrote:
> Initialize ready_ here with ready_(false)? unitialized members should
generally
> be avoided.

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:94: service_.reset();
On 2015/10/28 05:45:23, satorux1 wrote:
> do you need to reset fake_sender_ and service_ here? these are scoped_ptr, so
> it's probably not necessary

It's not necessary, but I'd like to stop the threads in the TearDown() if
possible.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_pre...
File components/arc/arc_pref_names.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_pre...
components/arc/arc_pref_names.cc:10: // Whether using ARC is enabled.
On 2015/10/28 05:45:23, satorux1 wrote:
> maybe drop 'using'?

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_pre...
components/arc/arc_pref_names.cc:12: "arc.enabled";
On 2015/10/28 05:45:23, satorux1 wrote:
> make it one line?

Hmm I thought I had seen some other pref_names.cc files with very short lines
split this way but I can't find the examples I looked at. Done.

[hidehiko, 2015-10-28 17:48:45]

Drive by.

I know it'd be very rare to hit race problem, but I still believe it's better to
be addressed.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:25: bool EnsureParentDirectoryPresent(const
std::string& socket_path) {
On 2015/10/28 05:45:22, satorux1 wrote:
> how about taking base::FilePath? 
> 
> The caller should pass base::FilePath(kArcBridgeSocketPath)
> 
> also function comment is missing. please check other places too

Or, in this case, we can inline the code in the caller?
E.g.) at L67:

// Before SocketConnect(), ensure its parent directory exists.
base::PostTaskAndReplyWithResult(
  file_task_runner_.get(),
  FROM_HERE,
  base::Bind(&base::CreateDirectory,
             base::FilePath(kArcBridgeSocketPath).DirName()),
  base::Bind(&ArcBridgeService::SocketConnect, ...));

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:30: bool SocketPermissions(const
std::string& socket_path) {
Or, does base::SetPosixFilePermissions work?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:65: if (state_ !=
ArcBridgeService::STOPPED)
Is this expected case?
If yes, it's good to comment when this happens, IMO.
If not, CHECK family (or at least LOG) would be your friend.

Also, as this does not update the state_, IPC channel could be created twice or
more,
if this is called multiple-times quickly.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:67: base::PostTaskAndReplyWithResult(
Just a suggestion.
1) It seems like directory creation is a part of SocketConnect, rather than a
part of HandleStartup()?
2) How about making a naming rule for async functions?

E.g.;
Here:
SocketConnect(base::FilePath(kArcBridgeSocketPath));

:

void SocketConnect(const base::FilePath& socket_path) {
  base::PostTaskAndReplyWithResult(
     :
     base::Bind(base::CreateDirectory, socket_path.Dir());
     base::Bind(SocketConnectAfterEnsureParentDirectory, ..., socket_path));
}


void SocketConnectAfterEnsureParentDirectory(const base::FilePath& socket_path,
                                             bool
is_ensure_parent_directory_succeeded) {
   :
   if (!ConnectIPCChannel(...)) {
     :
   }

   base::PostTaskAndReplyWithResult(
     :
     base::Bind(SetSocketPermission, ...),
     base::Bind(SocketConnectAfterSetSocketPermission, ...));
}

  :

And so on...
Some more notes: it'd be better to use |socket_path| or kArcBridgeSocketPath
consistently.
Current code seems to use in a mixed way.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:75: void ArcBridgeService::Shutdown() {
Looks to have a race issue?

If this is called while HandleStartup() but not completed,
then this instance would get into a something weird state.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:79: state_ != ArcBridgeService::READY)
Maybe logging?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: base::ScopedFD fd) {
On 2015/10/28 05:45:22, satorux1 wrote:
> on which thread should this be called? Add a DCHECK ?

FYI: I believe this runs on the browser thread. Assuming that, ScopedFD would be
somehow danger, because it would try to close fd in dtor on the thread
(BrowserThread), where the FD operation is not allowed.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:28: public IPC::Listener {
nit: can be fit in a line?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:30: enum State {
As this is statemachine, it'd be good to note the state transition briefly.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:106: // Internal connection method. Exposed
for testing.
For testing, I'd recommend you to pass that socket path or Handle to this
service via ctor and to test via public methods, rather than accessing to the
private methods. It should be more reliable tests.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:118: void NotifyStateChanged(State state);
nit: How about ChangeState(), if this actually changes its status.
NotifyStateChanged sounds like just sending a notification *without* changing
its status?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_factory.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_factory.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: s/2012/2015/ :-).

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:85:
EXPECT_TRUE(service_->Connect(handle, IPC::Channel::MODE_SERVER));
It'd be good to note that, for testing, we do not register the service to the
DBus.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:105: EXPECT_FALSE(IsReady());
nit: maybe ASSERT_FALSE, as it does not make sense to continue the testing if
this condition is not satisfied?

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:107:
EXPECT_TRUE(fake_sender_->Send(new ArcInstanceHostMsg_InstanceReady()));
ditto.

[Luis Héctor Chávez, 2015-10-28 21:06:32]

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:25: bool EnsureParentDirectoryPresent(const
std::string& socket_path) {
On 2015/10/28 17:48:45, hidehiko wrote:
> On 2015/10/28 05:45:22, satorux1 wrote:
> > how about taking base::FilePath? 
> > 
> > The caller should pass base::FilePath(kArcBridgeSocketPath)
> > 
> > also function comment is missing. please check other places too
> 
> Or, in this case, we can inline the code in the caller?
> E.g.) at L67:
> 
> // Before SocketConnect(), ensure its parent directory exists.
> base::PostTaskAndReplyWithResult(
>   file_task_runner_.get(),
>   FROM_HERE,
>   base::Bind(&base::CreateDirectory,
>              base::FilePath(kArcBridgeSocketPath).DirName()),
>   base::Bind(&ArcBridgeService::SocketConnect, ...));

Yep, that's cleaner. Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:30: bool SocketPermissions(const
std::string& socket_path) {
On 2015/10/28 17:48:45, hidehiko wrote:
> Or, does base::SetPosixFilePermissions work?

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:65: if (state_ !=
ArcBridgeService::STOPPED)
On 2015/10/28 17:48:45, hidehiko wrote:
> Is this expected case?
> If yes, it's good to comment when this happens, IMO.
> If not, CHECK family (or at least LOG) would be your friend.
> 
> Also, as this does not update the state_, IPC channel could be created twice
or
> more,
> if this is called multiple-times quickly.

Done

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:67: base::PostTaskAndReplyWithResult(
On 2015/10/28 17:48:45, hidehiko wrote:
> Just a suggestion.
> 1) It seems like directory creation is a part of SocketConnect, rather than a
> part of HandleStartup()?
> 2) How about making a naming rule for async functions?
> 
> E.g.;
> Here:
> SocketConnect(base::FilePath(kArcBridgeSocketPath));
> 
> :
> 
> void SocketConnect(const base::FilePath& socket_path) {
>   base::PostTaskAndReplyWithResult(
>      :
>      base::Bind(base::CreateDirectory, socket_path.Dir());
>      base::Bind(SocketConnectAfterEnsureParentDirectory, ..., socket_path));
> }
> 
> 
> void SocketConnectAfterEnsureParentDirectory(const base::FilePath&
socket_path,
>                                              bool
> is_ensure_parent_directory_succeeded) {
>    :
>    if (!ConnectIPCChannel(...)) {
>      :
>    }
> 
>    base::PostTaskAndReplyWithResult(
>      :
>      base::Bind(SetSocketPermission, ...),
>      base::Bind(SocketConnectAfterSetSocketPermission, ...));
> }
> 
>   :
> 
> And so on...
> Some more notes: it'd be better to use |socket_path| or kArcBridgeSocketPath
> consistently.
> Current code seems to use in a mixed way.

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:75: void ArcBridgeService::Shutdown() {
On 2015/10/28 17:48:45, hidehiko wrote:
> Looks to have a race issue?
> 
> If this is called while HandleStartup() but not completed,
> then this instance would get into a something weird state.

All state transitions are now guarded.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:79: state_ != ArcBridgeService::READY)
On 2015/10/28 17:48:45, hidehiko wrote:
> Maybe logging?

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: base::ScopedFD fd) {
On 2015/10/28 17:48:45, hidehiko wrote:
> On 2015/10/28 05:45:22, satorux1 wrote:
> > on which thread should this be called? Add a DCHECK ?
> 
> FYI: I believe this runs on the browser thread. Assuming that, ScopedFD would
be
> somehow danger, because it would try to close fd in dtor on the thread
> (BrowserThread), where the FD operation is not allowed.

This can run on any thread (you're the second to mention this, so it deserves a
code comment), so the caller must ensure it can safely pass the ScopedFD in the
first place. Also, since we are using Pass(), it will not even try to close it.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:28: public IPC::Listener {
On 2015/10/28 17:48:45, hidehiko wrote:
> nit: can be fit in a line?

It can, but having it separate seems to be the predominant convention.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_factory.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_factory.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2015/10/28 17:48:45, hidehiko wrote:
> nit: s/2012/2015/ :-).

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:85:
EXPECT_TRUE(service_->Connect(handle, IPC::Channel::MODE_SERVER));
On 2015/10/28 17:48:45, hidehiko wrote:
> It'd be good to note that, for testing, we do not register the service to the
> DBus.

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:105: EXPECT_FALSE(IsReady());
On 2015/10/28 17:48:45, hidehiko wrote:
> nit: maybe ASSERT_FALSE, as it does not make sense to continue the testing if
> this condition is not satisfied?

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:107:
EXPECT_TRUE(fake_sender_->Send(new ArcInstanceHostMsg_InstanceReady()));
On 2015/10/28 17:48:45, hidehiko wrote:
> ditto.

Done.

[Shuhei Takahashi, 2015-10-28 21:13:57]

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:35: // TODO(lhchavez): Tighten the security
around the socket by tying it to the
Could you file a bug so we don't forget this?

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:49: available_(false),
Hmm,I don't see any code setting available_ to true.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:90: ipc_channel_->Close();
Could you set ipc_channel_ to null?

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:101: observer_list_.AddObserver(observer);
ObserverList is not thread-safe. Could you insert a thread assertion here, or if
the list can be accessed from multiple threads, please use
ObserverListThreadSafe.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: Shutdown();
Shutdown() will fail when state_ is either STOPPED or ERROR.

Could you clarify when SetEnabled() is called? I'm not sure the lifetime of
ArcBridgeService. Does it outlive multiple sessions and SetEnabled() called
multiple times? Or is SetEnabled() called at most once when the session started?

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:122: return ipc_channel_->Send(new
ArcInstanceMsg_RegisterInputDevice(
Could you check state_? In some cases ipc_channel_ can be null.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:163: // Use base::Unretained since this is
a KeyedService.
Unretained seems not used here?

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:179:
NotifyStateChanged(ArcBridgeService::CONNECTED);
It is confusing that state is changed only when IPC::ChannelProxy::Create has
succeeded. Please move this line inside SocketConnect().

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:188: void
ArcBridgeService::NotifyStateChanged(State state) {
This function does set the current state, not only notifying observers. Could
you name this SetState() or something?

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:198: return;
In this code path, state_ is not reset to STOPPED.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:218: void
ArcBridgeService::OnInstanceStarted(
Could you swap the position of OnInstanceStarted() and OnInstanceReady()? The
definition order looks strange.

[Luis Héctor Chávez, 2015-10-28 23:12:20]

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:35: // TODO(lhchavez): Tighten the security
around the socket by tying it to the
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Could you file a bug so we don't forget this?

It's part of the same bug.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:49: available_(false),
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Hmm,I don't see any code setting available_ to true.

It's not there yet, but I want to have it available now.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:90: ipc_channel_->Close();
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Could you set ipc_channel_ to null?

Done.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:101: observer_list_.AddObserver(observer);
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> ObserverList is not thread-safe. Could you insert a thread assertion here, or
if
> the list can be accessed from multiple threads, please use
> ObserverListThreadSafe.

Done.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: Shutdown();
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Shutdown() will fail when state_ is either STOPPED or ERROR.
> 
> Could you clarify when SetEnabled() is called? I'm not sure the lifetime of
> ArcBridgeService. Does it outlive multiple sessions and SetEnabled() called
> multiple times? Or is SetEnabled() called at most once when the session
started?

Good catch. Done.

The lifetime of ArcBridgeService is tied to the session. SetEnabled can be
called multiple times since the preference can be changed.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:122: return ipc_channel_->Send(new
ArcInstanceMsg_RegisterInputDevice(
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Could you check state_? In some cases ipc_channel_ can be null.

Done.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:163: // Use base::Unretained since this is
a KeyedService.
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Unretained seems not used here?

Missed that one. Done.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:179:
NotifyStateChanged(ArcBridgeService::CONNECTED);
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> It is confusing that state is changed only when IPC::ChannelProxy::Create has
> succeeded. Please move this line inside SocketConnect().

Connect is also used from tests, so it needs to stay here.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:188: void
ArcBridgeService::NotifyStateChanged(State state) {
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> This function does set the current state, not only notifying observers. Could
> you name this SetState() or something?

Done.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:198: return;
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> In this code path, state_ is not reset to STOPPED.

It still needs to wait for the DBus message to return. Otherwise a different
message to start can be delivered while this is in-flight and get in a weird
state. This code was also changed in the next revision.

https://codereview.chromium.org/1412863004/diff/120001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:218: void
ArcBridgeService::OnInstanceStarted(
On 2015/10/28 21:13:57, Shuhei Takahashi wrote:
> Could you swap the position of OnInstanceStarted() and OnInstanceReady()? The
> definition order looks strange.

I don't want to reorder any further until it's ready to commit to avoid
polluting the diffs any more.

[Shuhei Takahashi, 2015-10-29 00:13:08]

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:72: ipc_channel_->Close();
ipc_channel_ can be null when state_ == CONNECTING.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:99: Shutdown();
I'm puzzled by asymmetry here that SetEnabled(false) calls Shutdown() but
SetEnabled(true) does not call HandleStartup().

Do you expect HandleStartup() is soon called by the same caller as
SetEnabled(true)? Then I think you can stop exposing HandleStartup() and
Shutdown(), and just provide SetEnabled() only.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:109: if (state_ != ArcBridgeService::READY)
{
If this function is called from arbitrary threads, I think we need to protect
state_ and ipc_channel_ with a lock so ipc_channel_ is not nullified after we
check state_.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:35: // When Shutdown() is called, if it is
not already stopped or in the ERROR
ERROR state has been removed.

[satorux1, 2015-10-29 05:23:19]

I'll defer to nya@ and hidehiko@ :)

[hidehiko, 2015-10-29 16:29:47]

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:106: // Internal connection method. Exposed
for testing.
On 2015/10/28 17:48:45, hidehiko wrote:
> For testing, I'd recommend you to pass that socket path or Handle to this
> service via ctor and to test via public methods, rather than accessing to the
> private methods. It should be more reliable tests.

ping?

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:24:
FILE_PATH_LITERAL("/home/chronos/ArcBridge/bridge.sock");
nit: 4 indent.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:32: : ipc_thread_("ARC bridge listener"),
nit: 4 indent.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I...

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:74: base::PostTaskAndReplyWithResult(
In general, shutdown step is the reverse order of the set-up steps.
In this case, setting up is 1) creating a socket and then 2) register the DBus,
so IMHO, it's better to 1) unregister DBus first then, 2) deleting a socket.
WDYT?

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:188: // Testing code can have the state in
STOPPED since it does not go through
How about DCHECK(state_ == CONNECTING);?
For testing, could you check my comment in the previous reply?

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:200: SetState(ArcBridgeService::CONNECTED);
Let's move this to SocketConnectAfterEnsureParentDirectory.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:122: const base::FilePath& socket_path, bool
directory_present);
nit: 4 indent.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:128: void
FinishConnectAfterSetSocketPermissions(const base::FilePath& socket_path,
Maybe: SocketConnectAfterSetSocketPermissions, as this is still a part of
SocketConnect.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:139: void
FinishShutdownAfterSocketDeleted(bool socket_deleted);
ditto: ShutdownAfterSocketDeleted.

[Luis Héctor Chávez, 2015-10-29 23:43:53]

There might be some changes in the D-Bus method names.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:30: enum State {
On 2015/10/28 17:48:45, hidehiko wrote:
> As this is statemachine, it'd be good to note the state transition briefly.

Done.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:106: // Internal connection method. Exposed
for testing.
On 2015/10/29 16:29:46, hidehiko wrote:
> On 2015/10/28 17:48:45, hidehiko wrote:
> > For testing, I'd recommend you to pass that socket path or Handle to this
> > service via ctor and to test via public methods, rather than accessing to
the
> > private methods. It should be more reliable tests.
> 
> ping?

Sorry I missed this. We can't do that since this is supposed to be built through
a factory. There's also the fact that if we do create an alternate constructor,
we would need to add additional logic to this class to figure out if it should
try to do the D-Bus connection, and that doesn't sound great. An alternative
discussed offline was to extract all the connection logic from this class, but
that seems like too much amount of work at this point in time.

I don't plan to modify this particular code in the future, but if I end up doing
that, I'll revisit this.

https://codereview.chromium.org/1412863004/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:118: void NotifyStateChanged(State state);
On 2015/10/28 17:48:45, hidehiko wrote:
> nit: How about ChangeState(), if this actually changes its status.
> NotifyStateChanged sounds like just sending a notification *without* changing
> its status?

Missed this as well. Was changed to SetState.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:24:
FILE_PATH_LITERAL("/home/chronos/ArcBridge/bridge.sock");
On 2015/10/29 16:29:47, hidehiko wrote:
> nit: 4 indent.

Done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:32: : ipc_thread_("ARC bridge listener"),
On 2015/10/29 16:29:47, hidehiko wrote:
> nit: 4 indent.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I...

Done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:72: ipc_channel_->Close();
On 2015/10/29 00:13:08, Shuhei Takahashi wrote:
> ipc_channel_ can be null when state_ == CONNECTING.

Done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:74: base::PostTaskAndReplyWithResult(
On 2015/10/29 16:29:47, hidehiko wrote:
> In general, shutdown step is the reverse order of the set-up steps.
> In this case, setting up is 1) creating a socket and then 2) register the
DBus,
> so IMHO, it's better to 1) unregister DBus first then, 2) deleting a socket.
> WDYT?

SGTM, done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:99: Shutdown();
On 2015/10/29 00:13:08, Shuhei Takahashi wrote:
> I'm puzzled by asymmetry here that SetEnabled(false) calls Shutdown() but
> SetEnabled(true) does not call HandleStartup().
> 
> Do you expect HandleStartup() is soon called by the same caller as
> SetEnabled(true)? Then I think you can stop exposing HandleStartup() and
> Shutdown(), and just provide SetEnabled() only.

HandleStartup() is called upon profile startup, whereas SetEnabled() can be
called later and with different values. Both functions are needed.

Eventually I will also start the connection when SetEnabled(true) is called
after HandleStartup() but I'm waiting to have more functionality to do so, like
the policy decision, but that will come later.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:109: if (state_ != ArcBridgeService::READY)
{
On 2015/10/29 00:13:08, Shuhei Takahashi wrote:
> If this function is called from arbitrary threads, I think we need to protect
> state_ and ipc_channel_ with a lock so ipc_channel_ is not nullified after we
> check state_.

Ugh, yes. I was trying to avoid locks, but it's not going to be possible :/ To
avoid having to also protect |state_| (which should be protected by the
thread-checking code already), removing the READY requirement and relying only
on the channel being non-null.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:188: // Testing code can have the state in
STOPPED since it does not go through
On 2015/10/29 16:29:47, hidehiko wrote:
> How about DCHECK(state_ == CONNECTING);?
> For testing, could you check my comment in the previous reply?

Keeping the current code as discussed offline.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:200: SetState(ArcBridgeService::CONNECTED);
On 2015/10/29 16:29:47, hidehiko wrote:
> Let's move this to SocketConnectAfterEnsureParentDirectory.

Keeping the current code as discussed offline.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:35: // When Shutdown() is called, if it is
not already stopped or in the ERROR
On 2015/10/29 00:13:08, Shuhei Takahashi wrote:
> ERROR state has been removed.

Done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:122: const base::FilePath& socket_path, bool
directory_present);
On 2015/10/29 16:29:47, hidehiko wrote:
> nit: 4 indent.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...

Done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:128: void
FinishConnectAfterSetSocketPermissions(const base::FilePath& socket_path,
On 2015/10/29 16:29:47, hidehiko wrote:
> Maybe: SocketConnectAfterSetSocketPermissions, as this is still a part of
> SocketConnect.

Done.

https://codereview.chromium.org/1412863004/diff/160001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:139: void
FinishShutdownAfterSocketDeleted(bool socket_deleted);
On 2015/10/29 16:29:47, hidehiko wrote:
> ditto: ShutdownAfterSocketDeleted.

Done.

[Shuhei Takahashi, 2015-11-02 17:58:03]

LGTM with several nitpicks. Thanks for working on this!

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:31: const
scoped_refptr<base::SequencedTaskRunner>& file_task_runner)
nit: excess indent (4 spaces here)

In general I recommend using auto indentation.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:87: weak_factory_.GetWeakPtr()));
nit: wrong indent

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: // ipc_channel_->Send() is
thread-safe, so there is no thread check.
This comment is obsolete.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:227: if (state_ == state)
I think we should rather ensure this condition:

DCHECK(state != state_);

Otherwise something should have went wrong (e.g. a callback called twice).

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:267: CHECK(state_ ==
ArcBridgeService::STOPPING);
Maybe DCHECK? I remember Chrome should avoid CHECK().

[oshima, 2015-11-02 19:13:49]

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:68: LOG(ERROR) << "Shutdown() called when
ARC is not running";
This file is using too many LOG(ERROR). Consider using

1) DCHECK, or CHECK if you want to catch an issue that can be caused by coding
error.
2) VLOG and enable using vmodule if it's not severer and won't affect the
functionality
3) Keep LOG(ERROR) to catch unexpected condition on the device that leads to bad
state.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:43: // When Shutdown() is called, the state
changes depending on the state it was at:
80 chars. You may want to add presubmit check for git cl formta. (searchfor
CheckPatchFormattted)

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service_factory.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_factory.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nuke (c)

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service_factory.h (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_factory.h:38: bool
ServiceIsCreatedWithBrowserContext() const override;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:37: return true;
return ipc_channel_;

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:71: bool IsReady() { return
ready_; }
bool ready() const {

or

rename variable to is_ready_, and then

bool is_ready() const { return is_ready_; }

[Luis Héctor Chávez, 2015-11-04 17:49:19]

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:31: const
scoped_refptr<base::SequencedTaskRunner>& file_task_runner)
On 2015/11/02 17:58:02, Shuhei Takahashi wrote:
> nit: excess indent (4 spaces here)
> 
> In general I recommend using auto indentation.

I messed up with my Vim setup recently and lost the auto-indent settings :(
Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:68: LOG(ERROR) << "Shutdown() called when
ARC is not running";
On 2015/11/02 19:13:49, oshima wrote:
> This file is using too many LOG(ERROR). Consider using
> 
> 1) DCHECK, or CHECK if you want to catch an issue that can be caused by coding
> error.
> 2) VLOG and enable using vmodule if it's not severer and won't affect the
> functionality
> 3) Keep LOG(ERROR) to catch unexpected condition on the device that leads to
bad
> state.
> 

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:87: weak_factory_.GetWeakPtr()));
On 2015/11/02 17:58:02, Shuhei Takahashi wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:115: // ipc_channel_->Send() is
thread-safe, so there is no thread check.
On 2015/11/02 17:58:02, Shuhei Takahashi wrote:
> This comment is obsolete.

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:227: if (state_ == state)
On 2015/11/02 17:58:02, Shuhei Takahashi wrote:
> I think we should rather ensure this condition:
> 
> DCHECK(state != state_);
> 
> Otherwise something should have went wrong (e.g. a callback called twice).

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:267: CHECK(state_ ==
ArcBridgeService::STOPPING);
On 2015/11/02 17:58:02, Shuhei Takahashi wrote:
> Maybe DCHECK? I remember Chrome should avoid CHECK().

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:43: // When Shutdown() is called, the state
changes depending on the state it was at:
On 2015/11/02 19:13:49, oshima wrote:
> 80 chars. You may want to add presubmit check for git cl formta. (searchfor
> CheckPatchFormattted)

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service_factory.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_factory.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/11/02 19:13:49, oshima wrote:
> nuke (c)
> 

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service_factory.h (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_factory.h:38: bool
ServiceIsCreatedWithBrowserContext() const override;
On 2015/11/02 19:13:49, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:37: return true;
On 2015/11/02 19:13:49, oshima wrote:
> return ipc_channel_;

Done.

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:71: bool IsReady() { return
ready_; }
On 2015/11/02 19:13:49, oshima wrote:
> bool ready() const {
> 
> or
> 
> rename variable to is_ready_, and then
> 
> bool is_ready() const { return is_ready_; }

Done.

[oshima, 2015-11-04 18:14:08]

lgtm

[Luis Héctor Chávez, 2015-11-04 19:27:11]

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

[Luis Héctor Chávez, 2015-11-04 19:27:11]

lhchavez@chromium.org changed required reviewers:
+ jochen@chromium.org

[Luis Héctor Chávez, 2015-11-04 19:27:12]

+jochen@ as OWNER of src/components

[jochen (gone - plz use gerrit), 2015-11-05 01:31:40]

can you please file a chrome tracking bug for this?

Can you also point me at the "Intent to implement" email?

[hidehiko, 2015-11-05 18:08:50]

Sorry for my delayed reply.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:32: : ipc_thread_("ARC bridge listener"),
nit: 4 space indent, followed by 6 space indent.
https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:118: if (!ipc_channel_) {
IIUC, state_ should be checked here? (If it is not ready, the message should not
be sent, right?)
Otherwise, even during the shutdown or if DBus is not established, the message
can be sent.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:38: //  
SocketConnectAfterSetsocketPermissions() ->
nit: SocketConnecetAfterSet*S*ocketPermissions.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:164: scoped_ptr<IPC::ChannelProxy>
ipc_channel_;
Design question.
ChannelProxy looks designed to be used from only one (in most cases Browser)
thread. If you're trying to send messages from various threads, probably using
IPC::Channel directly and communicating between threads manually would be an
option?

Or, alternatively, you may be able to PostMessage() to the BrowserThread once,
and SendMessage() on it, if the cost of the PostMessage is acceptable.
(IIUC, posting message between threads is a common way in Chrome).

WDYT?

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:23: : ipc_thread_("IPCSenderFake")
{
ditto (indent).

[Ben Goodger (Google), 2015-11-05 20:38:21]

FYI,

I had a discussion with Luis about this. He's going to make this not depend on
content and instead:

- instantiate this object in ChromeBrowserMain, where it will also be owned
(lifetime is bound to the Chrome browser process)
- implement user preference observation in Chrome, and simply provide an API on
the bridge to update enabled/disabled state
- provide a setter for the file thread task runner, called by ChromeBrowserMain

With these three changes the content dependency is no longer required.

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

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

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

+dcheng@ for arc::ArcBridgeService::RegisterInputDevice (IPC message
implementation)

[Ben Goodger (Google), 2015-11-05 21:53:15]

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

[Ben Goodger (Google), 2015-11-05 21:53:16]

meta lgtm to land this component now the content dependency issue is resolved,
once you address the rest of your team members' comments.

thanks!

[dcheng, 2015-11-06 01:35:28]

At a high-level:

1) Is it a requirement that adding/removing observers and RegisterInputDevice
must be callable on any thread? It makes things harder to reason about: for
example, after acquiring the lock, you still need to remember to check that the
ipc channel still exists. Much of Chrome doesn't allow multi-threaded stuff like
this for this reason.

2) I guess the IPC endpoint isn't even implemented in Chrome. Is this something
we've done before? How do you plan on keeping the two endpoints in sync so they
agree on message definitions, etc?

3) Has this gone through some sort of security review?

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:106: base::AutoLock
scoped_lock(ipc_channel_lock_);
Consider using clang-format to fix formatting issues like this.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:194: ipc_channel_ =
IPC::ChannelProxy::Create(handle, mode, this,
I don't think this can actually return null.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:46: enum State {
Consider using an enum class.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:97: State GetState() const { return state_;
}
Nit: google style is to name this state(), since it's a simple inlined accessor.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:103: bool IsEnabled() const { return
enabled_; }
Ditto: should just be:
bool enabled() const { return enabled_; }

[lhc(google), 2015-11-06 04:14:15]

lhchavez@google.com changed reviewers:
+ lhchavez@google.com

[lhc(google), 2015-11-06 04:14:17]

> 1) Is it a requirement that adding/removing observers and RegisterInputDevice
> must be callable on any thread? It makes things harder to reason about: for
> example, after acquiring the lock, you still need to remember to check that
the
> ipc channel still exists. Much of Chrome doesn't allow multi-threaded stuff
like
> this for this reason.

I thought it would be good for ease of use, but given that it is too much of a
hassle, I dropped it.

> 2) I guess the IPC endpoint isn't even implemented in Chrome. Is this
something
> we've done before? How do you plan on keeping the two endpoints in sync so
they
> agree on message definitions, etc?

That's correct, the other endpoint will be ARC. Right now the priority is to get
the rest of the team unblocked so it will be copied manually, but once
everything is stable, I plan to have an external repository with the IPC
definitions so that both Chrome and ARC can consume them. 

> 3) Has this gone through some sort of security review?

Only the design was looked at and no major concerns were raised, but a formal
security review has not happened.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:32: : ipc_thread_("ARC bridge listener"),
On 2015/11/05 18:08:50, hidehiko wrote:
> nit: 4 space indent, followed by 6 space indent.
>
https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists

Done.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:118: if (!ipc_channel_) {
On 2015/11/05 18:08:50, hidehiko wrote:
> IIUC, state_ should be checked here? (If it is not ready, the message should
not
> be sent, right?)
> Otherwise, even during the shutdown or if DBus is not established, the message
> can be sent.

Done, although no matter what we do there will always be some amount of inherent
raciness unless we make everything synchronized, which is not desirable. If the
message is sent and a Shutdown request is immediately issued, the message might
still be in flight.  Previously if DBus was not yet established, the message
would be queued up.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:38: //  
SocketConnectAfterSetsocketPermissions() ->
On 2015/11/05 18:08:50, hidehiko wrote:
> nit: SocketConnecetAfterSet*S*ocketPermissions.

Done.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:164: scoped_ptr<IPC::ChannelProxy>
ipc_channel_;
On 2015/11/05 18:08:50, hidehiko wrote:
> Design question.
> ChannelProxy looks designed to be used from only one (in most cases Browser)
> thread. If you're trying to send messages from various threads, probably using
> IPC::Channel directly and communicating between threads manually would be an
> option?
> 
> Or, alternatively, you may be able to PostMessage() to the BrowserThread once,
> and SendMessage() on it, if the cost of the PostMessage is acceptable.
> (IIUC, posting message between threads is a common way in Chrome).
> 
> WDYT?

Based on the number of comments about it, making this single-threaded, so
keeping the ChannelProxy.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:23: : ipc_thread_("IPCSenderFake")
{
On 2015/11/05 18:08:50, hidehiko wrote:
> ditto (indent).

Done.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:106: base::AutoLock
scoped_lock(ipc_channel_lock_);
On 2015/11/06 01:35:28, dcheng wrote:
> Consider using clang-format to fix formatting issues like this.

Done.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:194: ipc_channel_ =
IPC::ChannelProxy::Create(handle, mode, this,
On 2015/11/06 01:35:28, dcheng wrote:
> I don't think this can actually return null.

Done.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:46: enum State {
On 2015/11/06 01:35:28, dcheng wrote:
> Consider using an enum class.

Done.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:97: State GetState() const { return state_;
}
On 2015/11/06 01:35:28, dcheng wrote:
> Nit: google style is to name this state(), since it's a simple inlined
accessor.

Done.

https://codereview.chromium.org/1412863004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:103: bool IsEnabled() const { return
enabled_; }
On 2015/11/06 01:35:28, dcheng wrote:
> Ditto: should just be:
> bool enabled() const { return enabled_; }

Done.

[jochen (gone - plz use gerrit), 2015-11-06 06:02:17]

i would prefer if this code was landed together with something that uses it,
it's otherwise difficult to judge whether the API does what it's supposed to do.

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi
File components/arc.gypi (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi#ne...
components/arc.gypi:8: # GN version: //components/arc
the gn version is missing

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi#ne...
components/arc.gypi:15: '../base/base.gyp:base',
you also depend on ipc and chromeos

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi#ne...
components/arc.gypi:18: 'arc/arc_bridge_service.cc',
please include headers

https://codereview.chromium.org/1412863004/diff/240001/components/arc/OWNERS
File components/arc/OWNERS (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/OWNERS#...
components/arc/OWNERS:2: lhchavez@chromium.org
only chromium committers should be listed in owners files

https://codereview.chromium.org/1412863004/diff/240001/components/arc/OWNERS#...
components/arc/OWNERS:2: lhchavez@chromium.org
only chromium committers should be listed in owners files

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:39: ipc_thread_.StartWithOptions(
why do you need a dedicated thread?

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:39: ipc_thread_.StartWithOptions(
why do you need a dedicated thread?

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:71: virtual void OnStateChanged(State state)
{}
why aren't those pure virtual?

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:115: friend class arc::ArcBridgeTest;
please use the FRIEND_TEST_ALL_PREFIXES macro

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:115: friend class arc::ArcBridgeTest;
please use the FRIEND_TEST_ALL_PREFIXES macro

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:112: TEST_F(ArcBridgeTest, Basic)
{
can you add more tests please?

[Luis Héctor Chávez, 2015-11-09 16:41:04]

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/180001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:31: const
scoped_refptr<base::SequencedTaskRunner>& file_task_runner)
On 2015/11/04 17:49:18, Luis Héctor Chávez wrote:
> On 2015/11/02 17:58:02, Shuhei Takahashi wrote:
> > nit: excess indent (4 spaces here)
> > 
> > In general I recommend using auto indentation.
> 
> I messed up with my Vim setup recently and lost the auto-indent settings :(
> Done.

Turns out this was the correct indentation :/

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:32: : ipc_thread_("ARC bridge listener"),
On 2015/11/05 18:08:50, hidehiko wrote:
> nit: 4 space indent, followed by 6 space indent.
>
https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists

Hmm I had it this way before. Done.

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:38: //  
SocketConnectAfterSetsocketPermissions() ->
On 2015/11/05 18:08:50, hidehiko wrote:
> nit: SocketConnecetAfterSet*S*ocketPermissions.

Done.

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi
File components/arc.gypi (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi#ne...
components/arc.gypi:8: # GN version: //components/arc
On 2015/11/06 06:02:16, jochen wrote:
> the gn version is missing

Done.

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi#ne...
components/arc.gypi:15: '../base/base.gyp:base',
On 2015/11/06 06:02:16, jochen wrote:
> you also depend on ipc and chromeos

Done.

https://codereview.chromium.org/1412863004/diff/240001/components/arc.gypi#ne...
components/arc.gypi:18: 'arc/arc_bridge_service.cc',
On 2015/11/06 06:02:16, jochen wrote:
> please include headers

Done.

https://codereview.chromium.org/1412863004/diff/240001/components/arc/OWNERS
File components/arc/OWNERS (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/OWNERS#...
components/arc/OWNERS:2: lhchavez@chromium.org
On 2015/11/06 06:02:16, jochen wrote:
> only chromium committers should be listed in owners files

Done.

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:39: ipc_thread_.StartWithOptions(
On 2015/11/06 06:02:17, jochen (slow - traveling) wrote:
> why do you need a dedicated thread?

Changed it to ask for the task runner in the constructor instead.

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:71: virtual void OnStateChanged(State state)
{}
On 2015/11/06 06:02:17, jochen wrote:
> why aren't those pure virtual?

I saw examples of both pure virtual and this style. This is more convenient, but
I can change it if the convention is to only provide pure virtual.

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:115: friend class arc::ArcBridgeTest;
On 2015/11/06 06:02:17, jochen wrote:
> please use the FRIEND_TEST_ALL_PREFIXES macro

Done.

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/240001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:112: TEST_F(ArcBridgeTest, Basic)
{
On 2015/11/06 06:02:17, jochen wrote:
> can you add more tests please?

Done.

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

dcheng, any chance we can land this soon? This is becoming a bottleneck for us
:(

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

The consumer of this can be found in https://codereview.chromium.org/1416313004/

[jochen (gone - plz use gerrit), 2015-11-10 19:58:25]

I'd prefer if we could merge those two CLs (this plus the one that actually uses
the service).

Also, there's still a crbug.com/ entry missing.

[jochen (gone - plz use gerrit), 2015-11-10 19:59:07]

https://codereview.chromium.org/1412863004/diff/380001/chromeos/arc/bridge/co...
File chromeos/arc/bridge/common/arc_host_messages.h (right):

https://codereview.chromium.org/1412863004/diff/380001/chromeos/arc/bridge/co...
chromeos/arc/bridge/common/arc_host_messages.h:13: #define IPC_MESSAGE_EXPORT
CHROMEOS_EXPORT
why is this needed? I don't see this anywhere else

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

Description was changed from

==========
arc-bridge: Add the ARC Bridge Service

This adds the ARC bridge functionality and starts an ARC instance when
the user logs in.

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

to

==========
arc-bridge: Add the ARC Bridge Service

This adds the ARC bridge functionality and starts an ARC instance when
the user logs in.

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

[Luis Héctor Chávez, 2015-11-10 20:56:45]

crbug entry was added. I split the code to make reviewing faster since this is
an already large review.

https://codereview.chromium.org/1412863004/diff/380001/chromeos/arc/bridge/co...
File chromeos/arc/bridge/common/arc_host_messages.h (right):

https://codereview.chromium.org/1412863004/diff/380001/chromeos/arc/bridge/co...
chromeos/arc/bridge/common/arc_host_messages.h:13: #define IPC_MESSAGE_EXPORT
CHROMEOS_EXPORT
On 2015/11/10 19:59:07, jochen (slow - traveling) wrote:
> why is this needed? I don't see this anywhere else

This is needed because this piece of code gets built in chromeos/ into
libchromeos.a and it is not exported by default.

[jochen (gone - plz use gerrit), 2015-11-10 21:51:46]

hum, exporting the IPC macros strikes me as odd.

What's the reason you make this a component? Maybe the IPC messages you need
should be part of the component.

Can you please merge the CL that uses this service in this CL? I find it hard to
judge the API without also looking at its usage.

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

On 2015/11/10 21:51:46, jochen (slow - traveling) wrote:
> hum, exporting the IPC macros strikes me as odd.
> 
> What's the reason you make this a component? Maybe the IPC messages you need
> should be part of the component.

We need to build it outside of Chrome as well (in ARC). Having it separate makes
it easier for us to keep it in sync.

> 
> Can you please merge the CL that uses this service in this CL? I find it hard
to
> judge the API without also looking at its usage.

Will do.

[jochen (gone - plz use gerrit), 2015-11-11 22:10:05]

i don't how you can compile the component separately if it depends on ipc
messages from chromeos

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

On 2015/11/11 22:10:05, jochen (slow - traveling) wrote:
> i don't how you can compile the component separately if it depends on ipc
> messages from chromeos

We have a copy of chrome ipc in ARC. We need to manually synchronize the header
files between repositories.

[jochen (gone - plz use gerrit), 2015-11-11 22:16:10]

On 2015/11/11 at 22:12:08, lhchavez wrote:
> On 2015/11/11 22:10:05, jochen (slow - traveling) wrote:
> > i don't how you can compile the component separately if it depends on ipc
> > messages from chromeos
> 
> We have a copy of chrome ipc in ARC. We need to manually synchronize the
header files between repositories.

can we make the arc-bridge component use its own IPC macros anyways? it sounds
like you could sync them manually as well

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

On 2015/11/11 22:16:10, jochen (slow - traveling) wrote:
> On 2015/11/11 at 22:12:08, lhchavez wrote:
> > On 2015/11/11 22:10:05, jochen (slow - traveling) wrote:
> > > i don't how you can compile the component separately if it depends on ipc
> > > messages from chromeos
> > 
> > We have a copy of chrome ipc in ARC. We need to manually synchronize the
> header files between repositories.
> 
> can we make the arc-bridge component use its own IPC macros anyways? it sounds
> like you could sync them manually as well

I can, but I was really trying to avoid it in the hopes that once this lands and
I have some spare cycles we can migrate it to a separate repository for that and
make updating things easier with much less churn.

[jochen (gone - plz use gerrit), 2015-11-11 22:33:39]

On 2015/11/11 at 22:30:27, lhchavez wrote:
> On 2015/11/11 22:16:10, jochen (slow - traveling) wrote:
> > On 2015/11/11 at 22:12:08, lhchavez wrote:
> > > On 2015/11/11 22:10:05, jochen (slow - traveling) wrote:
> > > > i don't how you can compile the component separately if it depends on
ipc
> > > > messages from chromeos
> > > 
> > > We have a copy of chrome ipc in ARC. We need to manually synchronize the
> > header files between repositories.
> > 
> > can we make the arc-bridge component use its own IPC macros anyways? it
sounds
> > like you could sync them manually as well
> 
> I can, but I was really trying to avoid it in the hopes that once this lands
and I have some spare cycles we can migrate it to a separate repository for that
and make updating things easier with much less churn.

I think it's preferable to land a solution we can live with in the future in
case those spare cycles never show up.

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

On 2015/11/11 22:33:39, jochen (slow - traveling) wrote:
> On 2015/11/11 at 22:30:27, lhchavez wrote:
> > On 2015/11/11 22:16:10, jochen (slow - traveling) wrote:
> > > On 2015/11/11 at 22:12:08, lhchavez wrote:
> > > > On 2015/11/11 22:10:05, jochen (slow - traveling) wrote:
> > > > > i don't how you can compile the component separately if it depends on
> ipc
> > > > > messages from chromeos
> > > > 
> > > > We have a copy of chrome ipc in ARC. We need to manually synchronize the
> > > header files between repositories.
> > > 
> > > can we make the arc-bridge component use its own IPC macros anyways? it
> sounds
> > > like you could sync them manually as well
> > 
> > I can, but I was really trying to avoid it in the hopes that once this lands
> and I have some spare cycles we can migrate it to a separate repository for
that
> and make updating things easier with much less churn.
> 
> I think it's preferable to land a solution we can live with in the future in
> case those spare cycles never show up.

Should have been clearer, I was planning to do that immediately after landing
this.

I can move this here but there is one caveat: Since we want to build it from
outside of Chrome as well, there are two #include paths that are relative
instead of following the convention of using the full path (see
https://codereview.chromium.org/1424503002/patch/160001/170007 ). I was trying
to isolate that to chromeos/arc, but if it is acceptable to have that here, I'll
move it.

[jochen (gone - plz use gerrit), 2015-11-11 22:52:13]

ok, I'll defer to dcheng on the IPC header stuff then.

Waiting for the usage of this service to be merged into this CL, and the
referenced bug to be upgraded to a launch bug (you could then also reference it
from the other pending CLs..)

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

On 2015/11/11 22:52:13, jochen (slow - traveling) wrote:
> ok, I'll defer to dcheng on the IPC header stuff then.
> 
> Waiting for the usage of this service to be merged into this CL, and the
> referenced bug to be upgraded to a launch bug (you could then also reference
it
> from the other pending CLs..)

Done both.

[Luis Héctor Chávez, 2015-11-11 23:34:49]

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

[Luis Héctor Chávez, 2015-11-11 23:34:50]

+msw, sky since https://codereview.chromium.org/1416313004/ was merged into this
change.

[sky, 2015-11-11 23:39:53]

On 2015/11/11 23:34:50, Luis Héctor Chávez wrote:
> +msw, sky since https://codereview.chromium.org/1416313004/ was merged into
this
> change.

The owners you already have likely cover every file in this patch. Do you really
need Mike and myself to review too?

[oshima, 2015-11-12 02:17:49]

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:28: //   SetEnabled(true) + HandleStartup()
-> SocketConnect() ->
What's the reason for having both SetEnabled and HandleStartup? Looks like you
can just use one to start/shutdown the service.

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

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:28: //   SetEnabled(true) + HandleStartup()
-> SocketConnect() ->
On 2015/11/12 02:17:49, oshima wrote:
> What's the reason for having both SetEnabled and HandleStartup? Looks like you
> can just use one to start/shutdown the service.

Support for future changes that khmel@ is working on that will use the
ArcEnabled preference instead of/in addition to the switch.

[Luis Héctor Chávez, 2015-11-12 17:42:28]

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

[Luis Héctor Chávez, 2015-11-12 17:42:29]

+mpearson for tools/metrics/histograms/histograms.xml

[Luis Héctor Chávez, 2015-11-12 17:46:43]

On 2015/11/11 23:39:53, sky wrote:
> On 2015/11/11 23:34:50, Luis Héctor Chávez wrote:
> > +msw, sky since https://codereview.chromium.org/1416313004/ was merged into
> this
> > change.
> 
> The owners you already have likely cover every file in this patch. Do you
really
> need Mike and myself to review too?

No, just FYI.

[Mark P, 2015-11-12 17:49:22]

histograms.xml lgtm

[oshima, 2015-11-12 19:01:38]

On 2015/11/12 17:04:12, Luis Héctor Chávez wrote:
>
https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
> File components/arc/arc_bridge_service.h (right):
> 
>
https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
> components/arc/arc_bridge_service.h:28: //   SetEnabled(true) +
HandleStartup()
> -> SocketConnect() ->
> On 2015/11/12 02:17:49, oshima wrote:
> > What's the reason for having both SetEnabled and HandleStartup? Looks like
you
> > can just use one to start/shutdown the service.
> 
> Support for future changes that khmel@ is working on that will use the
> ArcEnabled preference instead of/in addition to the switch.

You can just start/stop upon preference change right? I also recommend to add it
when you really need it, not when you think you will need it in the future.

[Luis Héctor Chávez, 2015-11-12 19:20:45]

On 2015/11/12 19:01:38, oshima wrote:
> On 2015/11/12 17:04:12, Luis Héctor Chávez wrote:
> >
>
https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
> > File components/arc/arc_bridge_service.h (right):
> > 
> >
>
https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
> > components/arc/arc_bridge_service.h:28: //   SetEnabled(true) +
> HandleStartup()
> > -> SocketConnect() ->
> > On 2015/11/12 02:17:49, oshima wrote:
> > > What's the reason for having both SetEnabled and HandleStartup? Looks like
> you
> > > can just use one to start/shutdown the service.
> > 
> > Support for future changes that khmel@ is working on that will use the
> > ArcEnabled preference instead of/in addition to the switch.
> 
> You can just start/stop upon preference change right? I also recommend to add
it
> when you really need it, not when you think you will need it in the future.

Fair enough, removed.

[oshima, 2015-11-12 19:57:11]

thanks, lgtm

[jochen (gone - plz use gerrit), 2015-11-12 23:40:59]

https://codereview.chromium.org/1412863004/diff/420001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412863004/diff/420001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15316: +        <message
name="IDS_FLAGS_ENABLE_ARC_NAME" desc="Enable the use of the ARC instance.">
the desc needs to be descriptive enough so the translators understand what the
string actually means.

https://codereview.chromium.org/1412863004/diff/420001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1412863004/diff/420001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:708: // Stop the
currently running ARC instance, if any.
that comment doesn't add value

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:124: friend class arc::ArcBridgeTest;
should not be required anymore

https://codereview.chromium.org/1412863004/diff/500001/components/arc/DEPS
File components/arc/DEPS (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/DEPS#ne...
components/arc/DEPS:2: "+chromeos",
maybe restrict this to chromeos/dbus and chromeos/arc ?

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:50: g_arc_bridge_service = NULL;
nullptr

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:17: class ArcBridgeTest;
should not be required

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:81: // Gets the global instance of the ARC
Bridge Service.
A better comment would point out that you can only invoke this on the thread
that this class was created on

in general, please add comments to all public methods about the thread they may
be invoked on

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:164: // Task runner on which file operations
are performed.
i'd encourage you to remove comments that just restate what the code does.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:22: IPCSenderFake(
explicit

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:125: };
disallow copy/assign

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:144: };
disallow copy/assign

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_pre...
File components/arc/arc_prefs.cc (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_pre...
components/arc/arc_prefs.cc:25: (prefs &&
prefs->GetBoolean(arc::prefs::kArcEnabled));
when is prefs null?

[Luis Héctor Chávez, 2015-11-13 00:29:29]

https://codereview.chromium.org/1412863004/diff/420001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1412863004/diff/420001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15316: +        <message
name="IDS_FLAGS_ENABLE_ARC_NAME" desc="Enable the use of the ARC instance.">
On 2015/11/12 23:40:58, jochen (slow - traveling) wrote:
> the desc needs to be descriptive enough so the translators understand what the
> string actually means.

Done.

https://codereview.chromium.org/1412863004/diff/420001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1412863004/diff/420001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:708: // Stop the
currently running ARC instance, if any.
On 2015/11/12 23:40:58, jochen (slow - traveling) wrote:
> that comment doesn't add value

Done.

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/420001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:124: friend class arc::ArcBridgeTest;
On 2015/11/12 23:40:58, jochen (slow - traveling) wrote:
> should not be required anymore

This is still required to avoid a bunch of compilation issues (removed the arc::
namespace, though).

https://codereview.chromium.org/1412863004/diff/500001/components/arc/DEPS
File components/arc/DEPS (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/DEPS#ne...
components/arc/DEPS:2: "+chromeos",
On 2015/11/12 23:40:58, jochen (slow - traveling) wrote:
> maybe restrict this to chromeos/dbus and chromeos/arc ?

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:50: g_arc_bridge_service = NULL;
On 2015/11/12 23:40:58, jochen (slow - traveling) wrote:
> nullptr

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:17: class ArcBridgeTest;
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> should not be required

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:81: // Gets the global instance of the ARC
Bridge Service.
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> A better comment would point out that you can only invoke this on the thread
> that this class was created on
> 
> in general, please add comments to all public methods about the thread they
may
> be invoked on

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:164: // Task runner on which file operations
are performed.
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> i'd encourage you to remove comments that just restate what the code does.

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
File components/arc/arc_bridge_service_unittest.cc (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:22: IPCSenderFake(
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> explicit

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:125: };
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_bri...
components/arc/arc_bridge_service_unittest.cc:144: };
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_pre...
File components/arc/arc_prefs.cc (right):

https://codereview.chromium.org/1412863004/diff/500001/components/arc/arc_pre...
components/arc/arc_prefs.cc:25: (prefs &&
prefs->GetBoolean(arc::prefs::kArcEnabled));
On 2015/11/12 23:40:59, jochen (slow - traveling) wrote:
> when is prefs null?

Not anymore. Removed check.

[jochen (gone - plz use gerrit), 2015-11-13 00:35:03]

lgtm from my side, but please wait for dcheng@ to sign off on this.

The security review of a new ipc feature is important and will take some time.
If required, I encourage you to setup a vc to walk through this.

[dcheng, 2015-11-13 01:51:10]

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:29: ArcBridgeService* g_arc_bridge_service
= NULL;
nullptr

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:49: Shutdown();
In theory, shouldn't this always be called by
ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun? It seems like it would
be better to DCHECK that it has, in fact, been shutdown.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:103: ipc_channel_->Close();
Nit: Here and in OnInstanceStopped(), just call .reset() (the destructor calls
Close(): we should probably explicitly specify this as part of the interface)

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:177: // the user the instance will run as.
Please file a bug to track this.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:234: DCHECK(state_ != state);
DCHECK_NE

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:278: DCHECK(state_ == State::STOPPING);
DCHECK_EQ(State::STOPPING, state_)

[lhc(google), 2015-11-13 02:11:20]

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:29: ArcBridgeService* g_arc_bridge_service
= NULL;
On 2015/11/13 01:51:10, dcheng wrote:
> nullptr

Done.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:49: Shutdown();
On 2015/11/13 01:51:10, dcheng wrote:
> In theory, shouldn't this always be called by
> ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun? It seems like it would
> be better to DCHECK that it has, in fact, been shutdown.

Done.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:103: ipc_channel_->Close();
On 2015/11/13 01:51:10, dcheng wrote:
> Nit: Here and in OnInstanceStopped(), just call .reset() (the destructor calls
> Close(): we should probably explicitly specify this as part of the interface)

Done.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:177: // the user the instance will run as.
On 2015/11/13 01:51:10, dcheng wrote:
> Please file a bug to track this.

Done.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:234: DCHECK(state_ != state);
On 2015/11/13 01:51:10, dcheng wrote:
> DCHECK_NE

DCHECK_NE on enum classes didn't seem to be supported (at least I saw several
comments in other places that confirmed this).

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:278: DCHECK(state_ == State::STOPPING);
On 2015/11/13 01:51:10, dcheng wrote:
> DCHECK_EQ(State::STOPPING, state_)

Same as above. Added a comment about it.

[jochen (gone - plz use gerrit), 2015-11-13 02:17:51]

Zel tells me there should be no way for users to enable this code. If that's the
case, we shouldn't include the about:flags entry entry

Can you please clarify what the intention here is?

[lhc(google), 2015-11-13 02:20:35]

On 2015/11/13 02:17:51, jochen (slow - traveling) wrote:
> Zel tells me there should be no way for users to enable this code. If that's
the
> case, we shouldn't include the about:flags entry entry
> 
> Can you please clarify what the intention here is?

The about:flags entry was added at sky's request. Users cannot enable the code
unless session_manager says it is available, and that currently cannot be
user-enabled.

[sky, 2015-11-13 15:41:16]

The earlier patches I reviewed were looking at preferences to
determine if enabled. That didn't make sense to me, so I suggested
about:flags. Should it be a command line flag?

  -Scott

On Thu, Nov 12, 2015 at 6:20 PM,  <lhchavez@google.com> wrote:
> On 2015/11/13 02:17:51, jochen (slow - traveling) wrote:
>>
>> Zel tells me there should be no way for users to enable this code. If
>> that's
>
> the
>>
>> case, we shouldn't include the about:flags entry entry
>
>
>> Can you please clarify what the intention here is?
>
>
> The about:flags entry was added at sky's request. Users cannot enable the
> code
> unless session_manager says it is available, and that currently cannot be
> user-enabled.
>
> https://codereview.chromium.org/1412863004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Luis Héctor Chávez, 2015-11-13 16:53:23]

On 2015/11/13 15:41:16, sky wrote:
> The earlier patches I reviewed were looking at preferences to
> determine if enabled. That didn't make sense to me, so I suggested
> about:flags. Should it be a command line flag?

After a discussion with Zel, I'll change it to a compile-time option.

> 
>   -Scott
> 
> On Thu, Nov 12, 2015 at 6:20 PM,  <mailto:lhchavez@google.com> wrote:
> > On 2015/11/13 02:17:51, jochen (slow - traveling) wrote:
> >>
> >> Zel tells me there should be no way for users to enable this code. If
> >> that's
> >
> > the
> >>
> >> case, we shouldn't include the about:flags entry entry
> >
> >
> >> Can you please clarify what the intention here is?
> >
> >
> > The about:flags entry was added at sky's request. Users cannot enable the
> > code
> > unless session_manager says it is available, and that currently cannot be
> > user-enabled.
> >
> > https://codereview.chromium.org/1412863004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Luis Héctor Chávez, 2015-11-13 18:31:03]

This is now using a GYP_DEFINE feature instead of a switch.

[dcheng, 2015-11-13 23:14:21]

Do you expect a lot more code to be checked in under //chromeos/arc? From a
Chrome perspective, it's weird for IPC messages to be defined in one component
and only used in another.

I know it's shared outside Chrome, but it seems like having
//components/arc/common and just sharing that out wouldn't be a huge burden
either.

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:234: DCHECK(state_ != state);
On 2015/11/13 at 02:11:20, lhc(google) wrote:
> On 2015/11/13 01:51:10, dcheng wrote:
> > DCHECK_NE
> 
> DCHECK_NE on enum classes didn't seem to be supported (at least I saw several
comments in other places that confirmed this).

Doh. I filed a bug to fix this (https://crbug.com/555314)

[Luis Héctor Chávez, 2015-11-13 23:16:32]

On 2015/11/13 23:14:21, dcheng wrote:
> Do you expect a lot more code to be checked in under //chromeos/arc? From a
> Chrome perspective, it's weird for IPC messages to be defined in one component
> and only used in another.

It would be limited to the message definitions.

> 
> I know it's shared outside Chrome, but it seems like having
> //components/arc/common and just sharing that out wouldn't be a huge burden
> either.

If people are okay with the relative include path in that one file, I can move
it (I just don't want to move the file just to have somebody else block the
commit on that detail).

> 
>
https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
> File components/arc/arc_bridge_service.cc (right):
> 
>
https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
> components/arc/arc_bridge_service.cc:234: DCHECK(state_ != state);
> On 2015/11/13 at 02:11:20, lhc(google) wrote:
> > On 2015/11/13 01:51:10, dcheng wrote:
> > > DCHECK_NE
> > 
> > DCHECK_NE on enum classes didn't seem to be supported (at least I saw
several
> comments in other places that confirmed this).
> 
> Doh. I filed a bug to fix this (https://crbug.com/555314)

[dcheng, 2015-11-13 23:24:11]

On 2015/11/13 at 23:16:32, lhchavez wrote:
> On 2015/11/13 23:14:21, dcheng wrote:
> > Do you expect a lot more code to be checked in under //chromeos/arc? From a
> > Chrome perspective, it's weird for IPC messages to be defined in one
component
> > and only used in another.
> 
> It would be limited to the message definitions.
> 
> > 
> > I know it's shared outside Chrome, but it seems like having
> > //components/arc/common and just sharing that out wouldn't be a huge burden
> > either.
> 
> If people are okay with the relative include path in that one file, I can move
it (I just don't want to move the file just to have somebody else block the
commit on that detail).

From what I can tell, jochen@ and I both feel it would feel less out of place in
//components/arc (and it probably means you don't need the IPC_EXPORT magic
anymore).

> 
> > 
> >
https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
> > File components/arc/arc_bridge_service.cc (right):
> > 
> >
https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
> > components/arc/arc_bridge_service.cc:234: DCHECK(state_ != state);
> > On 2015/11/13 at 02:11:20, lhc(google) wrote:
> > > On 2015/11/13 01:51:10, dcheng wrote:
> > > > DCHECK_NE
> > > 
> > > DCHECK_NE on enum classes didn't seem to be supported (at least I saw
several
> > comments in other places that confirmed this).
> > 
> > Doh. I filed a bug to fix this (https://crbug.com/555314)

[Luis Héctor Chávez, 2015-11-13 23:36:37]

On 2015/11/13 23:24:11, dcheng wrote:
> On 2015/11/13 at 23:16:32, lhchavez wrote:
> > On 2015/11/13 23:14:21, dcheng wrote:
> > > Do you expect a lot more code to be checked in under //chromeos/arc? From
a
> > > Chrome perspective, it's weird for IPC messages to be defined in one
> component
> > > and only used in another.
> > 
> > It would be limited to the message definitions.
> > 
> > > 
> > > I know it's shared outside Chrome, but it seems like having
> > > //components/arc/common and just sharing that out wouldn't be a huge
burden
> > > either.
> > 
> > If people are okay with the relative include path in that one file, I can
move
> it (I just don't want to move the file just to have somebody else block the
> commit on that detail).
> 
> From what I can tell, jochen@ and I both feel it would feel less out of place
in
> //components/arc (and it probably means you don't need the IPC_EXPORT magic
> anymore).
> 
> > 
> > > 
> > >
>
https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
> > > File components/arc/arc_bridge_service.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1412863004/diff/520001/components/arc/arc_bri...
> > > components/arc/arc_bridge_service.cc:234: DCHECK(state_ != state);
> > > On 2015/11/13 at 02:11:20, lhc(google) wrote:
> > > > On 2015/11/13 01:51:10, dcheng wrote:
> > > > > DCHECK_NE
> > > > 
> > > > DCHECK_NE on enum classes didn't seem to be supported (at least I saw
> several
> > > comments in other places that confirmed this).
> > > 
> > > Doh. I filed a bug to fix this (https://crbug.com/555314)

Done. PTAL.

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

ping? is there anything else we can do to check this in?

[dcheng, 2015-11-16 22:35:58]

Sorry, my apologizes. I've been trying to figure out a last-minute M47 fix.

lgtm with a nit

https://codereview.chromium.org/1412863004/diff/620001/components/arc.gypi
File components/arc.gypi (right):

https://codereview.chromium.org/1412863004/diff/620001/components/arc.gypi#ne...
components/arc.gypi:22: 'arc/common/arc_message_generator.cc',
I think the moved IPC headers should also be in this list.

[lhc(google), 2015-11-17 02:32:05]

Thanks, everyone!

https://codereview.chromium.org/1412863004/diff/620001/components/arc.gypi
File components/arc.gypi (right):

https://codereview.chromium.org/1412863004/diff/620001/components/arc.gypi#ne...
components/arc.gypi:22: 'arc/common/arc_message_generator.cc',
On 2015/11/16 22:35:58, dcheng wrote:
> I think the moved IPC headers should also be in this list.

Done.

[lhc(google), 2015-11-17 02:32:20]

The CQ bit was checked by lhchavez@google.com

[lhc(google), 2015-11-17 02:32:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nya@chromium.org,
ben@chromium.org, mpearson@chromium.org, oshima@chromium.org,
jochen@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1412863004/#ps660001
(title: "Rebased to ToT")

[commit-bot: I haz the power, 2015-11-17 02:34:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412863004/660001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412863004/660001

[commit-bot: I haz the power, 2015-11-17 02:41:07]

Committed patchset #35 (id:660001)

[commit-bot: I haz the power, 2015-11-17 02:41:58]

Patchset 35 (id:??) landed as
https://crrev.com/c951edfb84c3439da64cc2d97d6e7b495323d349
Cr-Commit-Position: refs/heads/master@{#359982}