Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693013006:
arc: Add cryptohome_id to EmitArcBooted. (Closed)
|
Created:
3 years, 10 months ago by xzhou Modified:
3 years, 10 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hashimoto+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, Jorge Lucangeli Obes Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Add cryptohome_id to EmitArcBooted.
When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When
ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed.
This CL notifies session_manager that ARC++ has started and passes
the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR.
BUG=b/35049414
Review-Url: https://codereview.chromium.org/2693013006
Cr-Commit-Position: refs/heads/master@{#451468}
Committed: https://chromium.googlesource.com/chromium/src/+/e41cdb5e0d670d9b30f4da1e734b01c0c91ad4af
Patch Set 1 #
Total comments: 34
Patch Set 2 : arc: Add cryptohome_id to EmitArcBooted. #
Total comments: 10
Patch Set 3 : Address Yusuke's comments. #
Total comments: 2
Patch Set 4 : Address Luis's comments. #Messages
Total messages: 59 (37 generated)
The CQ bit was checked by xzhou@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...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
yusukes@chromium.org changed reviewers: + yusukes@chromium.org
drive-by https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:31: explicit ArcBootPhaseMonitorBridge(ArcBridgeService* bridge_service, remove explicit https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... File chromeos/dbus/fake_session_manager_client.h (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... chromeos/dbus/fake_session_manager_client.h:1: remove
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:64: LOG_IF(ERROR, !success) << "Failed to emit arc booted signal."; Optional: how about VLOG on success? https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:44: Profile* profile; s/profile/profile_/ Could you comment its lifetime and ownership? Or, you can keep AccountId instead?
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
arrived late to the drive-by party. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_service_launcher.cc:149: arc_service_manager_->AddService(base::MakeUnique<ArcBootPhaseMonitorBridge>( nit: add "// List in lexicographical order" in L145, remove the blank line in L148 and sort. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:64: LOG_IF(ERROR, !success) << "Failed to emit arc booted signal."; This does not touch any members, so please remove it from the class and put in the anonymous namespace. That way you don't need the |weak_ptr_factory_|. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:44: Profile* profile; Profile* const profile_; (Chrome members have a trailing underscore, and having the 'const' there prevents you from accidentally forgetting to initialize it). https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:49: base::WeakPtrFactory<ArcBootPhaseMonitorBridge> weak_ptr_factory_; nit: remove https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... chromeos/dbus/fake_session_manager_client.cc:195: if (!callback.is_null()) { use guard clause pattern: if (callback.is_null()) return; // rest. then again, why would |callback| be null? Why not DCHECK(!callback.is_null()); ? https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/session_manag... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/session_manag... chromeos/dbus/session_manager_client.cc:975: if (!callback.is_null()) same here, why would |callback| be null?
Argh, missed a few IWYU things. Please add the "arc: " prefix to the subject + commit message. Also, s/ARC\+\+/ARC/ in your commit message for consistency with all the comments in the codebase. Re: compilation errors, you might need to roll DEPS to include the changes in session_manager's D-Bus interface before trying again. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:11: #include "base/memory/weak_ptr.h" nit: remove since it will be unused. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:13: #include "chrome/browser/profiles/profile.h" nit: move to the .cc file and fwd-declare it here. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:17: #include "components/signin/core/account_id/account_id.h" nit: move it to the .cc since it's not used here.
The CQ bit was checked by xzhou@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...
Description was changed from ========== Add cryptohome_id to EmitArcBooted. When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed. This CL notifies session_manager that ARC++ has started and passes the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR. BUG=b/35049414 ========== to ========== Add cryptohome_id to EmitArcBooted. When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed. This CL notifies session_manager that ARC++ has started and passes the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR. BUG=b/35049414 ==========
Description was changed from ========== Add cryptohome_id to EmitArcBooted. When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed. This CL notifies session_manager that ARC++ has started and passes the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR. BUG=b/35049414 ========== to ========== arc: Add cryptohome_id to EmitArcBooted. When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed. This CL notifies session_manager that ARC++ has started and passes the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR. BUG=b/35049414 ==========
On 2017/02/15 19:32:32, Luis Héctor Chávez wrote: > Argh, missed a few IWYU things. > > Please add the "arc: " prefix to the subject + commit message. Also, > s/ARC\+\+/ARC/ in your commit message for consistency with all the comments in > the codebase. > > Re: compilation errors, you might need to roll DEPS to include the changes in > session_manager's D-Bus interface before trying again. > > https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... > File > chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h > (right): > > https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:11: > #include "base/memory/weak_ptr.h" > nit: remove since it will be unused. > > https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:13: > #include "chrome/browser/profiles/profile.h" > nit: move to the .cc file and fwd-declare it here. > > https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:17: > #include "components/signin/core/account_id/account_id.h" > nit: move it to the .cc since it's not used here. Addressed in patch set 2. Thanks.
Addressed all comments on patch set 2. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_service_launcher.cc:149: arc_service_manager_->AddService(base::MakeUnique<ArcBootPhaseMonitorBridge>( On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > nit: add "// List in lexicographical order" in L145, remove the blank line in > L148 and sort. Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:64: LOG_IF(ERROR, !success) << "Failed to emit arc booted signal."; On 2017/02/15 18:48:09, hidehiko wrote: > Optional: how about VLOG on success? Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:64: LOG_IF(ERROR, !success) << "Failed to emit arc booted signal."; On 2017/02/15 18:48:09, hidehiko wrote: > Optional: how about VLOG on success? Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:64: LOG_IF(ERROR, !success) << "Failed to emit arc booted signal."; On 2017/02/15 18:48:09, hidehiko wrote: > Optional: how about VLOG on success? Moved to Line 18. Done https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:64: LOG_IF(ERROR, !success) << "Failed to emit arc booted signal."; On 2017/02/15 18:48:09, hidehiko wrote: > Optional: how about VLOG on success? Moved to Line 18. Done https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:11: #include "base/memory/weak_ptr.h" On 2017/02/15 19:32:32, Luis Héctor Chávez wrote: > nit: remove since it will be unused. Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:13: #include "chrome/browser/profiles/profile.h" On 2017/02/15 19:32:32, Luis Héctor Chávez wrote: > nit: move to the .cc file and fwd-declare it here. Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:13: #include "chrome/browser/profiles/profile.h" On 2017/02/15 19:32:32, Luis Héctor Chávez wrote: > nit: move to the .cc file and fwd-declare it here. I changed to save AccountID per hidehiko's suggestions. Should I forward declare as well? I see src/chromium/src/ui/arc/notification/arc_notification_manager.h:57 directly included account_id.h so I am not sure if I should also use forward declare here. Happy to change it. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:17: #include "components/signin/core/account_id/account_id.h" On 2017/02/15 19:32:32, Luis Héctor Chávez wrote: > nit: move it to the .cc since it's not used here. Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:31: explicit ArcBootPhaseMonitorBridge(ArcBridgeService* bridge_service, On 2017/02/15 18:31:37, Yusuke Sato wrote: > remove explicit Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:44: Profile* profile; On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > Profile* const profile_; > > (Chrome members have a trailing underscore, and having the 'const' there > prevents you from accidentally forgetting to initialize it). Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:44: Profile* profile; On 2017/02/15 18:48:09, hidehiko wrote: > s/profile/profile_/ > Could you comment its lifetime and ownership? > > Or, you can keep AccountId instead? Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:44: Profile* profile; On 2017/02/15 18:48:09, hidehiko wrote: > s/profile/profile_/ > Could you comment its lifetime and ownership? > > Or, you can keep AccountId instead? Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:44: Profile* profile; On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > Profile* const profile_; > > (Chrome members have a trailing underscore, and having the 'const' there > prevents you from accidentally forgetting to initialize it). Done. https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:49: base::WeakPtrFactory<ArcBootPhaseMonitorBridge> weak_ptr_factory_; On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > nit: remove Done. https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... chromeos/dbus/fake_session_manager_client.cc:195: if (!callback.is_null()) { On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > use guard clause pattern: > > if (callback.is_null()) > return; > // rest. > > then again, why would |callback| be null? Why not DCHECK(!callback.is_null()); ? I am not sure why we check it here. I see RemoveArcData checks it. Is that related the life cycle of ArcSessionManager? https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... File chromeos/dbus/fake_session_manager_client.h (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... chromeos/dbus/fake_session_manager_client.h:1: On 2017/02/15 18:31:37, Yusuke Sato wrote: > remove Done. https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/session_manag... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/session_manag... chromeos/dbus/session_manager_client.cc:975: if (!callback.is_null()) On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > same here, why would |callback| be null? Done.
https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h (right): https://codereview.chromium.org/2693013006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h:13: #include "chrome/browser/profiles/profile.h" On 2017/02/16 18:34:50, xzhou wrote: > On 2017/02/15 19:32:32, Luis Héctor Chávez wrote: > > nit: move to the .cc file and fwd-declare it here. > > I changed to save AccountID per hidehiko's suggestions. Should I forward declare > as well? I see > src/chromium/src/ui/arc/notification/arc_notification_manager.h:57 directly > included account_id.h so I am not sure if I should also use forward declare > here. Happy to change it. Your code seems fine as https://google.github.io/styleguide/cppguide.html#Forward_Declarations says "Try to avoid forward declarations of entities defined in another project." https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... chromeos/dbus/fake_session_manager_client.cc:195: if (!callback.is_null()) { On 2017/02/16 18:34:50, xzhou wrote: > On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > > use guard clause pattern: > > > > if (callback.is_null()) > > return; > > // rest. > > > > then again, why would |callback| be null? Why not DCHECK(!callback.is_null()); > ? > > I am not sure why we check it here. I see RemoveArcData checks it. Is that > related the life cycle of ArcSessionManager? RemoveArcData's check seems just redundant. I don't think you need it here. https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:15: #include "components/signin/core/account_id/account_id.h" remove. you already did it in .h (and not using forward decl in .h is fine because of https://google.github.io/styleguide/cppguide.html#Forward_Declarations "Try to avoid forward declarations of entities defined in another project.") https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:18: void OnEmitArcBooted(bool success) { space between L17/L18 https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:21: } same as above https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:22: } } // namespace https://codereview.chromium.org/2693013006/diff/20001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/20001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:371: session_manager_proxy_->CallMethod( This works with both old and new session_manager implementation, correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xzhou@chromium.org to run a CQ dry run
Address Yusuke's comments and test without https://chrome-internal-review.googlesource.com/c/327703/ and https://chromium-review.googlesource.com/c/442049/ https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/1/chromeos/dbus/fake_session_... chromeos/dbus/fake_session_manager_client.cc:195: if (!callback.is_null()) { On 2017/02/16 18:42:57, Yusuke Sato wrote: > On 2017/02/16 18:34:50, xzhou wrote: > > On 2017/02/15 19:02:11, Luis Héctor Chávez wrote: > > > use guard clause pattern: > > > > > > if (callback.is_null()) > > > return; > > > // rest. > > > > > > then again, why would |callback| be null? Why not > DCHECK(!callback.is_null()); > > ? > > > > I am not sure why we check it here. I see RemoveArcData checks it. Is that > > related the life cycle of ArcSessionManager? > > RemoveArcData's check seems just redundant. I don't think you need it here. Done. https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:15: #include "components/signin/core/account_id/account_id.h" On 2017/02/16 18:42:58, Yusuke Sato wrote: > remove. you already did it in .h (and not using forward decl in .h is fine > because of > https://google.github.io/styleguide/cppguide.html#Forward_Declarations "Try to > avoid forward declarations of entities defined in another project.") Done. Thanks for the link. https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:18: void OnEmitArcBooted(bool success) { On 2017/02/16 18:42:58, Yusuke Sato wrote: > space between L17/L18 Done. https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:21: } On 2017/02/16 18:42:58, Yusuke Sato wrote: > same as above Done. https://codereview.chromium.org/2693013006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:22: } On 2017/02/16 18:42:58, Yusuke Sato wrote: > } // namespace Done. https://codereview.chromium.org/2693013006/diff/20001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2693013006/diff/20001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:371: session_manager_proxy_->CallMethod( On 2017/02/16 18:42:58, Yusuke Sato wrote: > This works with both old and new session_manager implementation, correct? I think so. In the old session manager, it will not pop the cryptohome_id and continue execution. However, the cache will be deleted immediately in arc-setup.conf. I tested on minnie and works as expected.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2693013006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:65: } // namespace arc nit: keep the newline above (for symmetry with L26).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
c/b/c/arc/ lgtm
The CQ bit was checked by xzhou@chromium.org to run a CQ dry run
Address Luis's comments. https://codereview.chromium.org/2693013006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc (right): https://codereview.chromium.org/2693013006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc:65: } // namespace arc On 2017/02/16 20:23:46, Luis Héctor Chávez wrote: > nit: keep the newline above (for symmetry with L26). Done.
Address Luis's comments.
xzhou@chromium.org changed reviewers: + derat@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xzhou@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xzhou@google.com changed reviewers: + oshima@chromium.org
xzhou@google.com changed reviewers: - oshima@chromium.org
lgtm for c/b/chromeos and chromeos/dbus
xzhou@google.com changed reviewers: + xzhou@google.com
The CQ bit was checked by xzhou@google.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2693013006/#ps60001 (title: "Address Luis's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xzhou@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xzhou@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xzhou@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": 60001, "attempt_start_ts": 1487435070621060,
"parent_rev": "13fbda637196d8a45059840cfd285cd7c3d85124", "commit_rev":
"e41cdb5e0d670d9b30f4da1e734b01c0c91ad4af"}
Message was sent while issue was closed.
Description was changed from ========== arc: Add cryptohome_id to EmitArcBooted. When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed. This CL notifies session_manager that ARC++ has started and passes the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR. BUG=b/35049414 ========== to ========== arc: Add cryptohome_id to EmitArcBooted. When ARC++ starts, dalvik cache is moved to $ANDROID_OLD_DATA_DIR. When ARC++ is started, the data in $ANDROID_OLD_DATA_DIR should be removed. This CL notifies session_manager that ARC++ has started and passes the cryptohome_id so session_manager can delete $ANDROID_OLD_DATA_DIR. BUG=b/35049414 Review-Url: https://codereview.chromium.org/2693013006 Cr-Commit-Position: refs/heads/master@{#451468} Committed: https://chromium.googlesource.com/chromium/src/+/e41cdb5e0d670d9b30f4da1e734b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e41cdb5e0d670d9b30f4da1e734b... |
