Add LoadComponent API in ImageLoader dbus adapter.

Add LoadComponent API in ImageLoader dbus adapter.

BUG=698767
TEST=On chrome os, RegisterComponent and then LoadComponent works seamlessly to download and mount escpr image.

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

[xiaochu, 2017-03-06 19:29:47]

xiaochu@chromium.org changed reviewers:
+ adlr@chromium.org, kerrnel@chromium.org, sorin@chromium.org,
waffles@chromium.org

[Sorin Jianu, 2017-03-06 19:33:17]

lgtm

Thank you!

https://codereview.chromium.org/2731253003/diff/20001/chromeos/dbus/image_loa...
File chromeos/dbus/image_loader_client.cc (right):

https://codereview.chromium.org/2731253003/diff/20001/chromeos/dbus/image_loa...
chromeos/dbus/image_loader_client.cc:83: std::string result = "";
doesn't need the "" initialization. The default ctor does that.

[xiaochu, 2017-03-06 20:25:48]

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

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

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

[xiaochu, 2017-03-06 20:27:54]

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

[xiaochu, 2017-03-06 20:28:24]

Thanks!

https://codereview.chromium.org/2731253003/diff/20001/chromeos/dbus/image_loa...
File chromeos/dbus/image_loader_client.cc (right):

https://codereview.chromium.org/2731253003/diff/20001/chromeos/dbus/image_loa...
chromeos/dbus/image_loader_client.cc:83: std::string result = "";
On 2017/03/06 19:33:17, Sorin Jianu wrote:
> doesn't need the "" initialization. The default ctor does that.

Done.

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

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

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

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiaochu, 2017-03-06 21:32:07]

I am trying to upload this modification in
third_party/cros_system_api/dbus/service_constants.h as well. but it shows
submit errors when 'git cl upload':

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  third_party/cros_system_api/dbus/service_constants.h
      Illegal include: "apmanager/dbus-constants.h"
    Because of no rule applying. \
  third_party/cros_system_api/dbus/service_constants.h
      Illegal include: "authpolicy/dbus-constants.h"

For third_party code, is there any special rules when we change them? Thanks.

[xiaochu, 2017-03-06 21:54:24]

Fix is here:
https://codereview.chromium.org/2731253003

[xiaochu, 2017-03-06 21:55:00]

Sorry, correct link:
https://chromium-review.googlesource.com/#/c/450502/

[waffles, 2017-03-06 23:25:22]

lgtm but please wait for Greg.

[xiaochu, 2017-03-07 00:34:40]

Description was changed from

==========
Add LoadComponent API in ImageLoader dbus adapter.

BUG=698767
TEST=NONE
==========

to

==========
Add LoadComponent API in ImageLoader dbus adapter.

BUG=698767
TEST=On chrome os, RegisterComponent and then LoadComponent works seamlessly to
download and mount escpr image.
==========

[xiaochu, 2017-03-07 15:16:56]

On 2017/03/06 23:25:22, waffles wrote:
> lgtm but please wait for Greg.

hi Greg, can you please take a look?

[Greg K, 2017-03-07 18:11:27]

On 2017/03/07 15:16:56, xiaochu wrote:
> On 2017/03/06 23:25:22, waffles wrote:
> > lgtm but please wait for Greg.
> 
> hi Greg, can you please take a look?

The CL looks fine, but are prepared to start using the LoadComponent
functionality in the near future?

[xiaochu, 2017-03-07 18:19:32]

On 2017/03/07 18:11:27, Greg K wrote:
> On 2017/03/07 15:16:56, xiaochu wrote:
> > On 2017/03/06 23:25:22, waffles wrote:
> > > lgtm but please wait for Greg.
> > 
> > hi Greg, can you please take a look?
> 
> The CL looks fine, but are prepared to start using the LoadComponent
> functionality in the near future?

Thanks! Assuming we use LoadComponent for CrOSComponent's OnCustomInstall, the
side effect would be every time a component is updated a new image is mounted
(but not used unless you specifically told the users). After a reboot,
everything mounted are gone.
So the only downside is you might have a mount point for each update happens
since last reboot.

Then my next step is to make the new mounted image useful.

[xiaochu, 2017-03-07 18:20:48]

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

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

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

[Greg K, 2017-03-07 18:23:41]

On 2017/03/07 18:19:32, xiaochu wrote:
> On 2017/03/07 18:11:27, Greg K wrote:
> > On 2017/03/07 15:16:56, xiaochu wrote:
> > > On 2017/03/06 23:25:22, waffles wrote:
> > > > lgtm but please wait for Greg.
> > > 
> > > hi Greg, can you please take a look?
> > 
> > The CL looks fine, but are prepared to start using the LoadComponent
> > functionality in the near future?
> 
> Thanks! Assuming we use LoadComponent for CrOSComponent's OnCustomInstall, the
> side effect would be every time a component is updated a new image is mounted
> (but not used unless you specifically told the users). After a reboot,
> everything mounted are gone.
> So the only downside is you might have a mount point for each update happens
> since last reboot.
> 
> Then my next step is to make the new mounted image useful.

LGTM

[commit-bot: I haz the power, 2017-03-07 18:41:14]

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

[commit-bot: I haz the power, 2017-03-07 18:41:16]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[xiaochu, 2017-03-08 01:25:55]

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

[commit-bot: I haz the power, 2017-03-08 01:27:27]

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

[commit-bot: I haz the power, 2017-03-08 02:36:36]

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

[commit-bot: I haz the power, 2017-03-08 02:36:37]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-03-08 02:47:57]

The CQ bit was checked by xiaochu@chromium.org

[xiaochu, 2017-03-08 02:47:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org,
waffles@chromium.org, kerrnel@chromium.org
Link to the patchset: https://codereview.chromium.org/2731253003/#ps100001
(title: "revert last patch")

[commit-bot: I haz the power, 2017-03-08 02:48:38]

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

[commit-bot: I haz the power, 2017-03-08 02:56:26]

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

[commit-bot: I haz the power, 2017-03-08 02:56:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xiaochu, 2017-03-08 03:00:25]

The CQ bit was checked by xiaochu@chromium.org

[commit-bot: I haz the power, 2017-03-08 03:00:54]

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

[xiaochu, 2017-03-08 03:01:47]

The CQ bit was unchecked by xiaochu@chromium.org

[xiaochu, 2017-03-08 03:05:12]

On 2017/03/08 03:00:54, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Sorry I uploaded a incorrect file to this issue and a revert patch set needs a
new lgtm...

[xiaochu, 2017-03-08 03:29:30]

lgtm

[xiaochu, 2017-03-08 03:29:45]

The CQ bit was checked by xiaochu@chromium.org

[commit-bot: I haz the power, 2017-03-08 03:30:11]

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

[xiaochu, 2017-03-08 03:30:51]

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

[xiaochu, 2017-03-08 03:31:05]

lgtm

[adlr1, 2017-03-08 03:33:47]

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

[adlr1, 2017-03-08 03:33:47]

lgtm

[commit-bot: I haz the power, 2017-03-08 03:38:00]

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

[commit-bot: I haz the power, 2017-03-08 03:38:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xiaochu, 2017-03-08 03:48:22]

xiaochu@chromium.org changed reviewers:
+ hashimoto@chromium.org

[xiaochu, 2017-03-08 17:03:20]

xiaochu@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2017-03-08 17:14:18]

https://codereview.chromium.org/2731253003/diff/100001/chromeos/dbus/image_lo...
File chromeos/dbus/image_loader_client.h (right):

https://codereview.chromium.org/2731253003/diff/100001/chromeos/dbus/image_lo...
chromeos/dbus/image_loader_client.h:32: const StringDBusMethodCallback&
callback) = 0;
This (and all public non-inherited methods) should have a documentation comment.

[xiaochu, 2017-03-08 17:21:57]

thanks!

https://codereview.chromium.org/2731253003/diff/100001/chromeos/dbus/image_lo...
File chromeos/dbus/image_loader_client.h (right):

https://codereview.chromium.org/2731253003/diff/100001/chromeos/dbus/image_lo...
chromeos/dbus/image_loader_client.h:32: const StringDBusMethodCallback&
callback) = 0;
On 2017/03/08 17:14:18, stevenjb wrote:
> This (and all public non-inherited methods) should have a documentation
comment.

Done.

[stevenjb, 2017-03-08 17:50:01]

lgtm

[xiaochu, 2017-03-08 17:50:45]

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

[commit-bot: I haz the power, 2017-03-08 17:51:04]

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

[commit-bot: I haz the power, 2017-03-08 18:29:59]

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

[commit-bot: I haz the power, 2017-03-08 18:30:00]

Dry run: This issue passed the CQ dry run.

[xiaochu, 2017-03-08 18:31:04]

The CQ bit was checked by xiaochu@chromium.org

[xiaochu, 2017-03-08 18:31:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, sorin@chromium.org, kerrnel@chromium.org, adlr@google.com
Link to the patchset: https://codereview.chromium.org/2731253003/#ps140001
(title: "Add LoadComponent API in ImageLoader dbus adapter.")

[commit-bot: I haz the power, 2017-03-08 18:31:24]

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

[commit-bot: I haz the power, 2017-03-08 18:36:45]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488997864930990,
"parent_rev": "525950cc7033074e1a1d3e44e7de0a966345460c", "commit_rev":
"c0d16f0bde63a69dc117bb2b6860b3dd57f3b25d"}

[commit-bot: I haz the power, 2017-03-08 18:37:28]

Description was changed from

==========
Add LoadComponent API in ImageLoader dbus adapter.

BUG=698767
TEST=On chrome os, RegisterComponent and then LoadComponent works seamlessly to
download and mount escpr image.
==========

to

==========
Add LoadComponent API in ImageLoader dbus adapter.

BUG=698767
TEST=On chrome os, RegisterComponent and then LoadComponent works seamlessly to
download and mount escpr image.

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

[commit-bot: I haz the power, 2017-03-08 18:37:29]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/c0d16f0bde63a69dc117bb2b6860...