Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524673003:
arc: Stop/start ARC++ kiosk app when maintenance session started/finished. (Closed)
|
Created:
4 years ago by Sergey Poromov Modified:
4 years ago CC:
Wen ZHANG, chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Stop/start ARC++ kiosk app when maintenance session started/finished.
ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc
because it uses parts of chrome/browser/*
BUG=b/32370502
Committed: https://crrev.com/ed90a6771ec79da9f1ab8f9a47fdede7dc45e4e4
Cr-Commit-Position: refs/heads/master@{#439775}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Pass BrowserContext from ArcServiceManager. #
Total comments: 10
Patch Set 3 : Fix includes and usages. #
Total comments: 6
Patch Set 4 : Add ArcKioskBridge::Delegate. #
Total comments: 14
Patch Set 5 : Remove ArcAppListPrefs* from ArcKioskAppService and nits. #
Total comments: 8
Patch Set 6 : nits and includes #
Total comments: 6
Patch Set 7 : Populate app_id always when preconditions change. #Patch Set 8 : revert "Patch set 7" #Patch Set 9 : Hidehiko comments #Patch Set 10 : Cleanup includes. #Patch Set 11 : rebase #Messages
Total messages: 48 (21 generated)
poromov@chromium.org changed reviewers: + alexchau@chromium.org, lhchavez@chromium.org, nkostylev@chromium.org, rickyz@chromium.org
lhchavez@chromium.org: Please review changes in components/arc/* chrome/browser/chromeos/arc/* nkostylev@chromium.org: Please review changes in chrome/browser/chromeos/* rickyz@chromium.org: Please review changes in components/arc/*
lhchavez@chromium.org changed reviewers: + dcheng@chromium.org - rickyz@chromium.org
Other components have taken dependencies on content/public/browser, so the suggestion I mentioned in the comment should be okay. also, substituting rickyz@ for dcheng@ since the former is no longer on the team. https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc (right): https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc:43: chromeos::ArcKioskAppService::Get(GetProfile())->MaintenanceSessionCreated(); Seems like you don't *need* the real Profile. A BrowserContext can do. You can pass it into the KioskBridge through https://cs.chromium.org/chromium/src/components/arc/arc_service_manager.cc?sq... (where you should add the BrowserContext as parameter).
Is there something in particular you'd like me to look at in components/arc?
https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc (right): https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc:43: chromeos::ArcKioskAppService::Get(GetProfile())->MaintenanceSessionCreated(); On 2016/11/22 21:15:30, Luis Héctor Chávez wrote: > Seems like you don't *need* the real Profile. A BrowserContext can do. You can > pass it into the KioskBridge through > https://cs.chromium.org/chromium/src/components/arc/arc_service_manager.cc?sq... > (where you should add the BrowserContext as parameter). There can't be any BrowserContext in components/arc/arc_service_manager due to violating deps rules. As well ArcKioskAppService should access to chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h and that's why it was moved from there to chrome/browser/chromeos/arc/... and now it's created by ArcServiceLauncher that doesn't have any information about Profile/BrowserContext
The CQ bit was checked by poromov@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_...)
On 2016/11/23 13:45:34, Sergey Poromov wrote: > https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... > File chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc (right): > > https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc:43: > chromeos::ArcKioskAppService::Get(GetProfile())->MaintenanceSessionCreated(); > On 2016/11/22 21:15:30, Luis Héctor Chávez wrote: > > Seems like you don't *need* the real Profile. A BrowserContext can do. You can > > pass it into the KioskBridge through > > > https://cs.chromium.org/chromium/src/components/arc/arc_service_manager.cc?sq... > > (where you should add the BrowserContext as parameter). > > There can't be any BrowserContext in components/arc/arc_service_manager due to > violating deps rules. > As well ArcKioskAppService should access to > chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h and that's why it > was moved from there to chrome/browser/chromeos/arc/... > and now it's created by ArcServiceLauncher that doesn't have any information > about Profile/BrowserContext You can add "+content/public/browser/browser_context.h", to components/arc/DEPS.
On 2016/11/23 17:11:01, Luis Héctor Chávez wrote: > On 2016/11/23 13:45:34, Sergey Poromov wrote: > > > https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... > > File chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc (right): > > > > > https://codereview.chromium.org/2524673003/diff/1/chrome/browser/chromeos/arc... > > chrome/browser/chromeos/arc/kiosk/arc_kiosk_bridge.cc:43: > > chromeos::ArcKioskAppService::Get(GetProfile())->MaintenanceSessionCreated(); > > On 2016/11/22 21:15:30, Luis Héctor Chávez wrote: > > > Seems like you don't *need* the real Profile. A BrowserContext can do. You > can > > > pass it into the KioskBridge through > > > > > > https://cs.chromium.org/chromium/src/components/arc/arc_service_manager.cc?sq... > > > (where you should add the BrowserContext as parameter). > > > > There can't be any BrowserContext in components/arc/arc_service_manager due to > > violating deps rules. > > As well ArcKioskAppService should access to > > chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h and that's why it > > was moved from there to chrome/browser/chromeos/arc/... > > and now it's created by ArcServiceLauncher that doesn't have any information > > about Profile/BrowserContext > > You can add "+content/public/browser/browser_context.h", to components/arc/DEPS. Done. Daniel, In first http://crrev.com/2449043003 CL Ricky requested to add security reviewer when adding the implementation. Mainly, please, look at the mojo bridge.
https://codereview.chromium.org/2524673003/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2524673003/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:19: #include "content/public/browser/browser_context.h" nit: Can you drop this and just forward-declare content::BrowserContext? https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.cc (right): https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.cc:10: #include "content/public/browser/browser_context.h" Seems like you don't need this here either. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.h (right): https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:14: #include "content/public/browser/browser_context.h" Same here, drop and forward-declare BrowserContext. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:17: using content::BrowserContext; Avoid global "using" in .h files at all costs. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:41: BrowserContext* browser_context_; nit: content::BrowserContext* const browser_context_;
https://codereview.chromium.org/2524673003/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2524673003/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:19: #include "content/public/browser/browser_context.h" On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > nit: Can you drop this and just forward-declare content::BrowserContext? Done. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.cc (right): https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.cc:10: #include "content/public/browser/browser_context.h" On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > Seems like you don't need this here either. Done. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.h (right): https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:14: #include "content/public/browser/browser_context.h" On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > Same here, drop and forward-declare BrowserContext. Done. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:17: using content::BrowserContext; On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > Avoid global "using" in .h files at all costs. Done. https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:41: BrowserContext* browser_context_; On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > nit: content::BrowserContext* const browser_context_; Done.
On 2016/11/23 18:05:57, Sergey Poromov wrote: > https://codereview.chromium.org/2524673003/diff/20001/components/arc/arc_serv... > File components/arc/arc_service_manager.h (right): > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/arc_serv... > components/arc/arc_service_manager.h:19: #include > "content/public/browser/browser_context.h" > On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > > nit: Can you drop this and just forward-declare content::BrowserContext? > > Done. > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... > File components/arc/kiosk/arc_kiosk_bridge.cc (right): > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... > components/arc/kiosk/arc_kiosk_bridge.cc:10: #include > "content/public/browser/browser_context.h" > On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > > Seems like you don't need this here either. > > Done. > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... > File components/arc/kiosk/arc_kiosk_bridge.h (right): > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... > components/arc/kiosk/arc_kiosk_bridge.h:14: #include > "content/public/browser/browser_context.h" > On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > > Same here, drop and forward-declare BrowserContext. > > Done. > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... > components/arc/kiosk/arc_kiosk_bridge.h:17: using content::BrowserContext; > On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > > Avoid global "using" in .h files at all costs. > > Done. > > https://codereview.chromium.org/2524673003/diff/20001/components/arc/kiosk/ar... > components/arc/kiosk/arc_kiosk_bridge.h:41: BrowserContext* browser_context_; > On 2016/11/23 17:42:09, Luis Héctor Chávez wrote: > > nit: content::BrowserContext* const browser_context_; > > Done. Hey all, friendly ping.
After hidehiko@ refactored ArcServiceLauncher, if you implement the changes I propose, your change should be simplified. https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:20: return new ArcKioskAppService(profile, prefs); I just realized that since both ArcKioskAppService and ArcAppListPrefs are both BrowserContextKeyedServices, you don't need to explicitly pass in the ref to ArcAppListPrefs*. Please substitute all references to prefs_ with ArcAppListPrefs::Get(profile_); https://codereview.chromium.org/2524673003/diff/40001/components/arc/kiosk/DEPS File components/arc/kiosk/DEPS (right): https://codereview.chromium.org/2524673003/diff/40001/components/arc/kiosk/DE... components/arc/kiosk/DEPS:2: "+chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h", oh, I'm terribly sorry about this, I should have been moer explicit: Code under components/ is not allowed to depend on chrome/, because it would create a dependency cycle. In order to break the cycle one thing you can do is introduce class Delegate { public: virtual ~Delegate() = default; virtual void MaintenanceSessionCreated() = 0; virtual void MaintenanceSessionFinished() = 0; } in the ArcKioskBridge, and that delegate to the constructor of ArcKioskBridge. You can make ArcKioskAppService implement that Delegate. You can then remove the code from ArcAppListPrefs that creates (and holds) the reference to ArcKioskAppService, and instead have the ArcServiceLauncher create the ArcKioskBridge with the same condition it has right now: https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l...
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
My mail filter is hit my name :-). Drive-by. https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:45: void MaintenanceSessionCreated(); Maybe: OnMaintenanceSessionCreated() for naming consistency?
Description was changed from ========== arc: Stop/start ARC++ kiosk app when maintenance session started/finished. ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc because it uses parts of chrome/browser/* BUG=b/32370502 ========== to ========== arc: Stop/start ARC++ kiosk app when maintenance session started/finished. ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc because it uses parts of chrome/browser/* BUG=b/32370502 ==========
poromov@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:20: return new ArcKioskAppService(profile, prefs); On 2016/11/30 19:11:54, Luis Héctor Chávez wrote: > I just realized that since both ArcKioskAppService and ArcAppListPrefs are both > BrowserContextKeyedServices, you don't need to explicitly pass in the ref to > ArcAppListPrefs*. > > Please substitute all references to prefs_ with ArcAppListPrefs::Get(profile_); Done. https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:45: void MaintenanceSessionCreated(); On 2016/12/01 01:18:39, hidehiko wrote: > Maybe: OnMaintenanceSessionCreated() for naming consistency? Done. https://codereview.chromium.org/2524673003/diff/40001/components/arc/kiosk/DEPS File components/arc/kiosk/DEPS (right): https://codereview.chromium.org/2524673003/diff/40001/components/arc/kiosk/DE... components/arc/kiosk/DEPS:2: "+chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h", On 2016/11/30 19:11:54, Luis Héctor Chávez wrote: > oh, I'm terribly sorry about this, I should have been moer explicit: > > Code under components/ is not allowed to depend on chrome/, because it would > create a dependency cycle. In order to break the cycle one thing you can do is > introduce > > class Delegate { > public: > virtual ~Delegate() = default; > virtual void MaintenanceSessionCreated() = 0; > virtual void MaintenanceSessionFinished() = 0; > } > > in the ArcKioskBridge, and that delegate to the constructor of ArcKioskBridge. > You can make ArcKioskAppService implement that Delegate. You can then remove the > code from ArcAppListPrefs that creates (and holds) the reference to > ArcKioskAppService, and instead have the ArcServiceLauncher create the > ArcKioskBridge with the same condition it has right now: > https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l... Done.
thanks, this looks much better :) https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:23: ArcKioskAppService* ArcKioskAppService::Get(Profile* context) { This doesn't need to change. Profile is-a content::BrowserContext, so let's keep it with the least amount of specialization. https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:79: prefs_(ArcAppListPrefs::Get(profile)), Remove this line and then s/prefs_/ArcAppListPrefs::Get(profile_)/g https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:100: prefs_->RemoveObserver(this); It's probably better if you override KeyedService::Shutdown() and remove the observer for the prefs service there. https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h" nit: can you remove this? https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:59: ArcAppListPrefs* const prefs_; nit: can you remove this? https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.cc (right): https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.cc:29: if (delegate_) { under which circumstances would |delegate_| be nullptr? If there aren't any (I think there shouldn't), please add a DCHECK(delegate_); in L14. https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.h (right): https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:24: class Delegate { Please add a short comment.
https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:23: ArcKioskAppService* ArcKioskAppService::Get(Profile* context) { On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > This doesn't need to change. Profile is-a content::BrowserContext, so let's keep > it with the least amount of specialization. Done. https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:79: prefs_(ArcAppListPrefs::Get(profile)), On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > Remove this line and then s/prefs_/ArcAppListPrefs::Get(profile_)/g Done. https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:100: prefs_->RemoveObserver(this); On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > It's probably better if you override KeyedService::Shutdown() and remove the > observer for the prefs service there. Done. https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h" On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > nit: can you remove this? The class still overrides ArcAppListPrefs::Observer, so probably better to leave the include here. https://codereview.chromium.org/2524673003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:59: ArcAppListPrefs* const prefs_; On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > nit: can you remove this? Done. https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.cc (right): https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.cc:29: if (delegate_) { On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > under which circumstances would |delegate_| be nullptr? If there aren't any (I > think there shouldn't), please add a DCHECK(delegate_); in L14. Done. https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... File components/arc/kiosk/arc_kiosk_bridge.h (right): https://codereview.chromium.org/2524673003/diff/60001/components/arc/kiosk/ar... components/arc/kiosk/arc_kiosk_bridge.h:24: class Delegate { On 2016/12/01 20:13:33, Luis Héctor Chávez wrote: > Please add a short comment. Done.
lgtm with two more nits. https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:85: : profile_(profile), maintenance_session_running_(false) { nit: you can initialize |maintentance_session_running_ = false| in the header file (like |task_id_|). https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/profiles/profile.h" nit: You can remove this (as well as L16) since you only have pointers to those things, and forward declare them: namespace content { class BrowserContext; } // namespace content namespace chromeos { class Profile; ...
https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:85: : profile_(profile), maintenance_session_running_(false) { On 2016/12/16 16:33:46, Luis Héctor Chávez wrote: > nit: you can initialize |maintentance_session_running_ = false| in the header > file (like |task_id_|). Done. https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/profiles/profile.h" On 2016/12/16 16:33:46, Luis Héctor Chávez wrote: > nit: You can remove this (as well as L16) since you only have pointers to those > things, and forward declare them: > > namespace content { > class BrowserContext; > } // namespace content > > namespace chromeos { > > class Profile; > > ... Can't get rid of including profile.h as I need to keep inheritance from BrowserContext
Just FYI. Please feel free to move forward with other reviewer's LGTM. https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/profiles/profile.h" On 2016/12/16 17:00:30, Sergey Poromov wrote: > On 2016/12/16 16:33:46, Luis Héctor Chávez wrote: > > nit: You can remove this (as well as L16) since you only have pointers to > those > > things, and forward declare them: > > > > namespace content { > > class BrowserContext; > > } // namespace content > > > > namespace chromeos { > > > > class Profile; > > > > ... > > Can't get rid of including profile.h as I need to keep inheritance from > BrowserContext You can use forward decl in the header file, and then include the profile.h in .cc file, instead. https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:119: app_manager_->GetAppByAccountId(account_id); (maybe off topic): app_manager_ looks to be able to be nullptr, according to ctor and Shutdown()'s code? https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:146: arc_service_manager_->AddService(base::MakeUnique<ArcKioskBridge>( I think it's better to comment life cycle of ArcKioskBridge::Delegate. ArcKioskAppService is BrowserContextKeyedService so that it is destroyed on destroying the Profile, which is after ArcServiceLauncher::Shutdown(). https://codereview.chromium.org/2524673003/diff/100001/components/arc/kiosk/a... File components/arc/kiosk/arc_kiosk_bridge.h (right): https://codereview.chromium.org/2524673003/diff/100001/components/arc/kiosk/a... components/arc/kiosk/arc_kiosk_bridge.h:32: ArcKioskBridge(ArcBridgeService* bridge_service, Delegate* delegate); Could you briefly comment that |delegate| needs to be alive while this instance is alive?
On 2016/11/22 12:17:32, Sergey Poromov wrote: > mailto:nkostylev@chromium.org: Please review changes in > chrome/browser/chromeos/* lgtm
The CQ bit was checked by poromov@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.
https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/profiles/profile.h" On 2016/12/19 13:53:33, hidehiko wrote: > On 2016/12/16 17:00:30, Sergey Poromov wrote: > > On 2016/12/16 16:33:46, Luis Héctor Chávez wrote: > > > nit: You can remove this (as well as L16) since you only have pointers to > > those > > > things, and forward declare them: > > > > > > namespace content { > > > class BrowserContext; > > > } // namespace content > > > > > > namespace chromeos { > > > > > > class Profile; > > > > > > ... > > > > Can't get rid of including profile.h as I need to keep inheritance from > > BrowserContext > > You can use forward decl in the header file, and then include the profile.h in > .cc file, instead. In that case I get ../../chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:37:51: error: invalid static_cast from type 'content::BrowserContext*' to type 'chromeos::Profile*' Profile* profile = static_cast<Profile*>(context); https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:119: app_manager_->GetAppByAccountId(account_id); On 2016/12/19 13:53:33, hidehiko wrote: > (maybe off topic): app_manager_ looks to be able to be nullptr, according to > ctor and Shutdown()'s code? Actually it couldn't be nullptr as it's unconditionally created in ChromeBrowserMainPartsChromeos::PreProfileInit() I changed code above to DCHECK on app_manager_ instead of null check. https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_service_launcher.cc (right): https://codereview.chromium.org/2524673003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_service_launcher.cc:146: arc_service_manager_->AddService(base::MakeUnique<ArcKioskBridge>( On 2016/12/19 13:53:33, hidehiko wrote: > I think it's better to comment life cycle of ArcKioskBridge::Delegate. > ArcKioskAppService is BrowserContextKeyedService so that it is destroyed on > destroying the Profile, which is after ArcServiceLauncher::Shutdown(). Done. https://codereview.chromium.org/2524673003/diff/100001/components/arc/kiosk/a... File components/arc/kiosk/arc_kiosk_bridge.h (right): https://codereview.chromium.org/2524673003/diff/100001/components/arc/kiosk/a... components/arc/kiosk/arc_kiosk_bridge.h:32: ArcKioskBridge(ArcBridgeService* bridge_service, Delegate* delegate); On 2016/12/19 13:53:33, hidehiko wrote: > Could you briefly comment that |delegate| needs to be alive while this instance > is alive? Done.
https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/profiles/profile.h" On 2016/12/19 17:28:38, Sergey Poromov wrote: > On 2016/12/19 13:53:33, hidehiko wrote: > > On 2016/12/16 17:00:30, Sergey Poromov wrote: > > > On 2016/12/16 16:33:46, Luis Héctor Chávez wrote: > > > > nit: You can remove this (as well as L16) since you only have pointers to > > > those > > > > things, and forward declare them: > > > > > > > > namespace content { > > > > class BrowserContext; > > > > } // namespace content > > > > > > > > namespace chromeos { > > > > > > > > class Profile; > > > > > > > > ... > > > > > > Can't get rid of including profile.h as I need to keep inheritance from > > > BrowserContext > > > > You can use forward decl in the header file, and then include the profile.h in > > .cc file, instead. > > In that case I get > ../../chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:37:51: > error: invalid static_cast from type 'content::BrowserContext*' to type > 'chromeos::Profile*' > Profile* profile = static_cast<Profile*>(context); Profile lives in a global namespace. https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile.h?q=Prof...
lgtm
https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h (right): https://codereview.chromium.org/2524673003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.h:11: #include "chrome/browser/profiles/profile.h" On 2016/12/19 17:39:18, hidehiko wrote: > On 2016/12/19 17:28:38, Sergey Poromov wrote: > > On 2016/12/19 13:53:33, hidehiko wrote: > > > On 2016/12/16 17:00:30, Sergey Poromov wrote: > > > > On 2016/12/16 16:33:46, Luis Héctor Chávez wrote: > > > > > nit: You can remove this (as well as L16) since you only have pointers > to > > > > those > > > > > things, and forward declare them: > > > > > > > > > > namespace content { > > > > > class BrowserContext; > > > > > } // namespace content > > > > > > > > > > namespace chromeos { > > > > > > > > > > class Profile; > > > > > > > > > > ... > > > > > > > > Can't get rid of including profile.h as I need to keep inheritance from > > > > BrowserContext > > > > > > You can use forward decl in the header file, and then include the profile.h > in > > > .cc file, instead. > > > > In that case I get > > > ../../chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service_factory.cc:37:51: > > error: invalid static_cast from type 'content::BrowserContext*' to type > > 'chromeos::Profile*' > > Profile* profile = static_cast<Profile*>(context); > > Profile lives in a global namespace. > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile.h?q=Prof... I realized that the problem was originally because arc_service_launcher.cc didn't include "profile.h" and didn't know about inheritance. Fixed now.
The CQ bit was checked by poromov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, nkostylev@chromium.org, hidehiko@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2524673003/#ps180001 (title: "Cleanup includes.")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by poromov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, nkostylev@chromium.org, lhchavez@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2524673003/#ps200001 (title: "rebase")
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": 200001, "attempt_start_ts": 1482233703567610,
"parent_rev": "476b7a3187c3784990be334546a02405da97a0e3", "commit_rev":
"91fad40f7f8d3ecdeb430ca8ef17f7e19b84b200"}
Message was sent while issue was closed.
Description was changed from ========== arc: Stop/start ARC++ kiosk app when maintenance session started/finished. ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc because it uses parts of chrome/browser/* BUG=b/32370502 ========== to ========== arc: Stop/start ARC++ kiosk app when maintenance session started/finished. ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc because it uses parts of chrome/browser/* BUG=b/32370502 Review-Url: https://codereview.chromium.org/2524673003 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== arc: Stop/start ARC++ kiosk app when maintenance session started/finished. ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc because it uses parts of chrome/browser/* BUG=b/32370502 Review-Url: https://codereview.chromium.org/2524673003 ========== to ========== arc: Stop/start ARC++ kiosk app when maintenance session started/finished. ARC Kiosk bridge is also moved from components/arc into chrome/browser/chromeos/arc because it uses parts of chrome/browser/* BUG=b/32370502 Committed: https://crrev.com/ed90a6771ec79da9f1ab8f9a47fdede7dc45e4e4 Cr-Commit-Position: refs/heads/master@{#439775} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ed90a6771ec79da9f1ab8f9a47fdede7dc45e4e4 Cr-Commit-Position: refs/heads/master@{#439775} |
