Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744453003:
Mount image upon component installed (Closed)
DescriptionMount image upon component registered
Previously, component is copied to /var/lib/imageloader/xxx folder upon
installation and next reboot mount the extracted image.
This patch mount the extracted image in place folder copy and no reboot
is required.
BUG=690521
TEST=test OnCustumInstall on a chromebook, escpr component is downloaded and mounted seamlessly.
Review-Url: https://codereview.chromium.org/2744453003
Cr-Commit-Position: refs/heads/master@{#456109}
Committed: https://chromium.googlesource.com/chromium/src/+/4e103bd54a3f7bb6bbca13928f804165a29400b6
Patch Set 1 #Patch Set 2 : incremental change #
Total comments: 6
Patch Set 3 : addressing comments from hashimoto #Messages
Total messages: 19 (10 generated)
Description was changed from ========== Mount image upon component registered Previously, component is copied to /var/lib/imageloader/xxx folder upon installation and next reboot mount the extracted image. This patch mount the extracted image in place folder copy and no reboot is required. BUG=690521 TEST=test OnCustumInstall on a chromebook, escpr component is downloaded and mounted seamlessly. ========== to ========== Mount image upon component registered Previously, component is copied to /var/lib/imageloader/xxx folder upon installation and next reboot mount the extracted image. This patch mount the extracted image in place folder copy and no reboot is required. BUG=690521 TEST=test OnCustumInstall on a chromebook, escpr component is downloaded and mounted seamlessly. ==========
On 2017/03/08 23:45:44, xiaochu wrote: > mailto:xiaochu@chromium.org changed reviewers: > + mailto:adlr@chromium.org, mailto:kerrnel@chromium.org, mailto:sorin@chromium.org, > mailto:stevenjb@chromium.org, mailto:waffles@chromium.org, mailto:xiaochu@chromium.org can you please take a look? thanks!
hashimoto@chromium.org changed reviewers: + hashimoto@chromium.org
https://codereview.chromium.org/2744453003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2744453003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/cros_component_installer.cc:22: void LogLoadResult(chromeos::DBusMethodCallStatus call_status, Does this code compile? Seems this callback lacks the bool argument. https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/dbus_meth... File chromeos/dbus/dbus_method_call_status.h (right): https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/dbus_meth... chromeos/dbus/dbus_method_call_status.h:39: BoolDBusMethodCallbackPm; Our style guide prohibits cryptic abbreviation like "Pm". https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/image_loa... File chromeos/dbus/image_loader_client.cc (right): https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/image_loa... chromeos/dbus/image_loader_client.cc:52: base::Bind(&ImageLoaderClientImpl::OnBoolPmMethod, callback, name)); You should just use OnBoolMethod here. If you want to use the value of |name| in the callback, you can just bind it in cros_component_installer.cc after reordering the callback arguments.
Thanks, hashimoto! All done. Thanks for pointing out the usage of 'Bind'. Now only one file requires review. https://codereview.chromium.org/2744453003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/cros_component_installer.cc (right): https://codereview.chromium.org/2744453003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/cros_component_installer.cc:22: void LogLoadResult(chromeos::DBusMethodCallStatus call_status, On 2017/03/10 04:27:22, hashimoto wrote: > Does this code compile? > Seems this callback lacks the bool argument. it is a StringDBusMethodCallback type. And a new imageloader API loadComponent handles it. https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/dbus_meth... File chromeos/dbus/dbus_method_call_status.h (right): https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/dbus_meth... chromeos/dbus/dbus_method_call_status.h:39: BoolDBusMethodCallbackPm; On 2017/03/10 04:27:22, hashimoto wrote: > Our style guide prohibits cryptic abbreviation like "Pm". > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules removed. https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/image_loa... File chromeos/dbus/image_loader_client.cc (right): https://codereview.chromium.org/2744453003/diff/20001/chromeos/dbus/image_loa... chromeos/dbus/image_loader_client.cc:52: base::Bind(&ImageLoaderClientImpl::OnBoolPmMethod, callback, name)); On 2017/03/10 04:27:22, hashimoto wrote: > You should just use OnBoolMethod here. > If you want to use the value of |name| in the callback, you can just bind it in > cros_component_installer.cc after reordering the callback arguments. Done.
lgtm
The CQ bit was checked by xiaochu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Thank you"!
On 2017/03/10 16:56:05, Sorin Jianu wrote: > lgtm > > Thank you"! LGTM - Greg
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thank you all!
The CQ bit was checked by xiaochu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489169327759660,
"parent_rev": "f06a8bf73cce8614d325903a7e96e363fa90ce48", "commit_rev":
"4e103bd54a3f7bb6bbca13928f804165a29400b6"}
Message was sent while issue was closed.
Description was changed from ========== Mount image upon component registered Previously, component is copied to /var/lib/imageloader/xxx folder upon installation and next reboot mount the extracted image. This patch mount the extracted image in place folder copy and no reboot is required. BUG=690521 TEST=test OnCustumInstall on a chromebook, escpr component is downloaded and mounted seamlessly. ========== to ========== Mount image upon component registered Previously, component is copied to /var/lib/imageloader/xxx folder upon installation and next reboot mount the extracted image. This patch mount the extracted image in place folder copy and no reboot is required. BUG=690521 TEST=test OnCustumInstall on a chromebook, escpr component is downloaded and mounted seamlessly. Review-Url: https://codereview.chromium.org/2744453003 Cr-Commit-Position: refs/heads/master@{#456109} Committed: https://chromium.googlesource.com/chromium/src/+/4e103bd54a3f7bb6bbca13928f80... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4e103bd54a3f7bb6bbca13928f80... |
