Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599333002:
arc: Try to restart ARC++ kiosk app if the task was closed. (Closed)
|
Created:
3 years, 12 months ago by Sergey Poromov Modified:
3 years, 11 months ago CC:
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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Try to restart ARC++ kiosk app if the task was closed.
As ARC++ kiosk should always be running during the ARC++ kiosk session,
it make sense to restart it when there is notification that the task was closed.
However, it won't restart the app if some preconditions are not yet satisfied,
e.g. policy is not compliant, app is not ready.
BUG=676825
TEST=Manual, Run Kiosk on ARC++ device
Review-Url: https://codereview.chromium.org/2599333002
Cr-Commit-Position: refs/heads/master@{#441932}
Committed: https://chromium.googlesource.com/chromium/src/+/86cbdf53ee55b9373f2836f79fa4424a92cb36d6
Patch Set 1 #
Total comments: 1
Messages
Total messages: 19 (11 generated)
poromov@chromium.org changed reviewers: + khmel@chromium.org, nkostylev@chromium.org
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/2599333002/diff/1/chrome/browser/chromeos/app... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): https://codereview.chromium.org/2599333002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:64: PreconditionsChanged(); I found 2 handlers: ArcKioskAppLauncher::OnTaskCreated ArcKioskAppService::OnTaskCreated And it seems that both do the same thing, checking target app if (!app || app->package_name != package_name || app->activity != activity) and setting task_id_. If so, would it be possible to get rid of duplicate logic? However this looks outside of scope of this CL so I would be ok in this case to do it in separate CL, todo b/ is preferred. nit: please also add TEST= section in desc. lgtm as task described.
Description was changed from ========== arc: Try to restart ARC++ kiosk app if the task was closed. As ARC++ kiosk should always be running during the ARC++ kiosk session, it make sense to restart it when there is notification that the task was closed. However, it won't restart the app if some preconditions are not yet satisfied, e.g. policy is not compliant, app is not ready. BUG=676825 ========== to ========== arc: Try to restart ARC++ kiosk app if the task was closed. As ARC++ kiosk should always be running during the ARC++ kiosk session, it make sense to restart it when there is notification that the task was closed. However, it won't restart the app if some preconditions are not yet satisfied, e.g. policy is not compliant, app is not ready. BUG=676825 TEST=Manual, Run Kiosk on ARC++ device ==========
On 2016/12/26 20:29:38, khmel wrote: > https://codereview.chromium.org/2599333002/diff/1/chrome/browser/chromeos/app... > File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc (right): > > https://codereview.chromium.org/2599333002/diff/1/chrome/browser/chromeos/app... > chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc:64: > PreconditionsChanged(); > I found 2 handlers: > > ArcKioskAppLauncher::OnTaskCreated > ArcKioskAppService::OnTaskCreated > And it seems that both do the same thing, checking target app > if (!app || app->package_name != package_name || app->activity != activity) > and setting task_id_. > If so, would it be possible to get rid of duplicate logic? > However this looks outside of scope of this CL so I would be ok in this case to > do it in separate CL, todo b/ is preferred. > > nit: please also add TEST= section in desc. > > lgtm as task described. Thanks for review. Yes, there is some duplication between ArcKioskAppLauncher and ArcKioskAppService - they are doing the same observing, but for different purposes. It makes sense to remove it, but it's sure something for another CL, as this one is fixing a bug and going to be merged into M56 as well.
The CQ bit was checked by poromov@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by nkostylev@chromium.org
lgtm
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": 1, "attempt_start_ts": 1483710344116220, "parent_rev":
"66cd34190886e221b7d79beac0b19cf6b2ebf633", "commit_rev":
"86cbdf53ee55b9373f2836f79fa4424a92cb36d6"}
Message was sent while issue was closed.
Description was changed from ========== arc: Try to restart ARC++ kiosk app if the task was closed. As ARC++ kiosk should always be running during the ARC++ kiosk session, it make sense to restart it when there is notification that the task was closed. However, it won't restart the app if some preconditions are not yet satisfied, e.g. policy is not compliant, app is not ready. BUG=676825 TEST=Manual, Run Kiosk on ARC++ device ========== to ========== arc: Try to restart ARC++ kiosk app if the task was closed. As ARC++ kiosk should always be running during the ARC++ kiosk session, it make sense to restart it when there is notification that the task was closed. However, it won't restart the app if some preconditions are not yet satisfied, e.g. policy is not compliant, app is not ready. BUG=676825 TEST=Manual, Run Kiosk on ARC++ device Review-Url: https://codereview.chromium.org/2599333002 Cr-Commit-Position: refs/heads/master@{#441932} Committed: https://chromium.googlesource.com/chromium/src/+/86cbdf53ee55b9373f2836f79fa4... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/86cbdf53ee55b9373f2836f79fa4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
