Move free disk space check to session_manager.

Move free disk space check to session_manager.

Now session_manager returns an error if the free disk
space is low. Thus, this removes the current disk space
checking in arc_bridge_bootstrap, instead, looking at
the error code on StartArcInstance failure.

BUG=628124, 633258
, b/30498714
TEST=Ran on test device. Ran trybots.

Committed: https://crrev.com/14f70fe81402b55617e7f92839368b14c42eaf1f
Cr-Commit-Position: refs/heads/master@{#423873}

[hidehiko, 2016-10-06 07:24:53]

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

[commit-bot: I haz the power, 2016-10-06 07:25:04]

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

[hidehiko, 2016-10-06 07:27:54]

hidehiko@chromium.org changed reviewers:
+ derat@chromium.org, hashimoto@chromium.org, nya@chromium.org

[hidehiko, 2016-10-06 07:27:55]

Thank you for review in advance.

nya@: all code, incl. ArcBridgeBootstrap.
hashimoto@: chromeos/dbus
derat@: chrome/browser/chromeos/settings

FYI: lhchavez@.

[hidehiko, 2016-10-06 07:28:56]

Note: this CL needs https://chromium-review.googlesource.com/#/c/393628/ to be
landed in advance.
But it is already LGTM'ed and in CQ, so let me send this CL to review now.

[commit-bot: I haz the power, 2016-10-06 08:10:44]

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

[commit-bot: I haz the power, 2016-10-06 08:10:44]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-10-06 08:12:20]

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:738: response ? response->GetErrorName()
: base::EmptyString());
Please be consistent by sticking to one of "" or base::EmptyString.

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.h:236: // manager can not be reached). On
Failure, the error is passed to the second
Please make it clear that what you get is an error name which looks like
"org.freedesktop.DBus.Error.ServiceUnknown".

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.h:240: typedef base::Callback<void(bool,
const std::string&)>
Like GetArcStartTimeCallback, please give names to the parameters.

[Shuhei Takahashi, 2016-10-06 08:21:03]

https://codereview.chromium.org/2397863003/diff/1/components/arc/arc_bridge_b...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/2397863003/diff/1/components/arc/arc_bridge_b...
components/arc/arc_bridge_bootstrap.cc:118: // CHECKING_DISK_SPACE
Please update these comments to remove CHECKING_DISK_SPACE.

https://codereview.chromium.org/2397863003/diff/1/components/arc/arc_bridge_b...
components/arc/arc_bridge_bootstrap.cc:282: VLOG(2) << "Disk space check is
done. Creating socket...";
Could you remove this line?

[hidehiko, 2016-10-06 08:32:58]

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

[hidehiko, 2016-10-06 08:33:21]

Thank you for quick review. PTAL.

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.cc:738: response ? response->GetErrorName()
: base::EmptyString());
On 2016/10/06 08:12:20, hashimoto wrote:
> Please be consistent by sticking to one of "" or base::EmptyString.

Done.

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.h:236: // manager can not be reached). On
Failure, the error is passed to the second
On 2016/10/06 08:12:20, hashimoto wrote:
> Please make it clear that what you get is an error name which looks like
> "org.freedesktop.DBus.Error.ServiceUnknown".

Done.

https://codereview.chromium.org/2397863003/diff/1/chromeos/dbus/session_manag...
chromeos/dbus/session_manager_client.h:240: typedef base::Callback<void(bool,
const std::string&)>
On 2016/10/06 08:12:20, hashimoto wrote:
> Like GetArcStartTimeCallback, please give names to the parameters.

Done.

https://codereview.chromium.org/2397863003/diff/1/components/arc/arc_bridge_b...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/2397863003/diff/1/components/arc/arc_bridge_b...
components/arc/arc_bridge_bootstrap.cc:118: // CHECKING_DISK_SPACE
On 2016/10/06 08:21:03, Shuhei Takahashi wrote:
> Please update these comments to remove CHECKING_DISK_SPACE.

Good catch. Done.

https://codereview.chromium.org/2397863003/diff/1/components/arc/arc_bridge_b...
components/arc/arc_bridge_bootstrap.cc:282: VLOG(2) << "Disk space check is
done. Creating socket...";
On 2016/10/06 08:21:03, Shuhei Takahashi wrote:
> Could you remove this line?

Done.

[commit-bot: I haz the power, 2016-10-06 08:33:23]

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

[hashimoto, 2016-10-06 08:36:59]

chromeos/dbus lgtm

https://codereview.chromium.org/2397863003/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2397863003/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:241: typedef base::Callback<void(bool
is_succeeded, const std::string& error)>
nit: "bool success" would be more consistent with existing code.

[hidehiko, 2016-10-06 08:40:22]

https://codereview.chromium.org/2397863003/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2397863003/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:241: typedef base::Callback<void(bool
is_succeeded, const std::string& error)>
On 2016/10/06 08:36:59, hashimoto wrote:
> nit: "bool success" would be more consistent with existing code.

Ah, right. Done.

[Shuhei Takahashi, 2016-10-06 08:42:54]

lgtm

Thanks!

[Daniel Erat, 2016-10-06 14:53:54]

+stevenjb, who is out (i.e. don't wait for his review) but may have opinions
when he's back :-)

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:236: // manager can not be reached). On
Failure, the error is passed to the second
nit: s/Failure/failure/

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:238: // reponse is not received at all.
The error looks like, e.g.,
nit: s/reponse/response/

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:241: typedef base::Callback<void(bool
success, const std::string& error)>
is there any reason you can't perform this TODO in this change?

https://codereview.chromium.org/2397863003/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/2397863003/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_bootstrap.cc:49: // TODO(hidehiko): Share the constant
between Chrome and ChromeOS.
i agree that this constant should be moved to system_api so it can be shared,
but i don't think we want these strings to leak out of SessionManagerClient;
they feel like an implementation detail of the d-bus interface.

could you instead move this constant to session_manager_client.cc and introduce
a new enum in SessionManagerClient? e.g.

  enum class StartArcInstanceResult {
    SUCCESS,  
    LOW_FREE_DISK_SPACE,
    OTHER_ERROR,
  };

[hidehiko, 2016-10-07 05:24:33]

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

[commit-bot: I haz the power, 2016-10-07 05:24:49]

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

[hidehiko, 2016-10-07 05:25:07]

The CQ bit was unchecked by hidehiko@chromium.org

[hidehiko, 2016-10-07 05:28:26]

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

[hidehiko, 2016-10-07 05:28:38]

Thank you for review. PTAL.

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:236: // manager can not be reached). On
Failure, the error is passed to the second
On 2016/10/06 14:53:54, Daniel Erat wrote:
> nit: s/Failure/failure/

Acknowledged.

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:238: // reponse is not received at all.
The error looks like, e.g.,
On 2016/10/06 14:53:53, Daniel Erat wrote:
> nit: s/reponse/response/

Acknowledged.

https://codereview.chromium.org/2397863003/diff/40001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:241: typedef base::Callback<void(bool
success, const std::string& error)>
On 2016/10/06 14:53:53, Daniel Erat wrote:
> is there any reason you can't perform this TODO in this change?

Other "Callback"s defined above use typedef for now, so I used typedef for
consistency in this CL.
Let me send another CL to replace all typedefs to usings to keep the CL's focus.

https://codereview.chromium.org/2397863003/diff/40001/components/arc/arc_brid...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/2397863003/diff/40001/components/arc/arc_brid...
components/arc/arc_bridge_bootstrap.cc:49: // TODO(hidehiko): Share the constant
between Chrome and ChromeOS.
On 2016/10/06 14:53:54, Daniel Erat wrote:
> i agree that this constant should be moved to system_api so it can be shared,
> but i don't think we want these strings to leak out of SessionManagerClient;
> they feel like an implementation detail of the d-bus interface.
> 
> could you instead move this constant to session_manager_client.cc and
introduce
> a new enum in SessionManagerClient? e.g.
> 
>   enum class StartArcInstanceResult {
>     SUCCESS,  
>     LOW_FREE_DISK_SPACE,
>     OTHER_ERROR,
>   };

Done.

[commit-bot: I haz the power, 2016-10-07 05:28:44]

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

[commit-bot: I haz the power, 2016-10-07 06:06:05]

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

[commit-bot: I haz the power, 2016-10-07 06:06:06]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-10-07 14:26:10]

lgtm

thanks!

[hidehiko, 2016-10-07 16:08:31]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-10-07 16:08:32]

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

[commit-bot: I haz the power, 2016-10-07 16:08:55]

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

[commit-bot: I haz the power, 2016-10-07 16:14:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-07 16:15:40]

Description was changed from

==========
Move free disk space check to session_manager.

Now session_manager returns an error if the free disk
space is low. Thus, this removes the current disk space
checking in arc_bridge_bootstrap, instead, looking at
the error code on StartArcInstance failure.

BUG=628124, 633258, b/30498714
TEST=Ran on test device. Ran trybots.
==========

to

==========
Move free disk space check to session_manager.

Now session_manager returns an error if the free disk
space is low. Thus, this removes the current disk space
checking in arc_bridge_bootstrap, instead, looking at
the error code on StartArcInstance failure.

BUG=628124, 633258, b/30498714
TEST=Ran on test device. Ran trybots.

Committed: https://crrev.com/14f70fe81402b55617e7f92839368b14c42eaf1f
Cr-Commit-Position: refs/heads/master@{#423873}
==========

[commit-bot: I haz the power, 2016-10-07 16:15:42]

Patchset 4 (id:??) landed as
https://crrev.com/14f70fe81402b55617e7f92839368b14c42eaf1f
Cr-Commit-Position: refs/heads/master@{#423873}