Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2758593002:
Enable screen capture for ARC++ Kiosk (Closed)
|
Created:
3 years, 9 months ago by Ivan Šandrk Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, poromov+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable screen capture for ARC++ Kiosk
Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks.
BUG=677216
TEST=Manually tested new behaviour.
Review-Url: https://codereview.chromium.org/2758593002
Cr-Commit-Position: refs/heads/master@{#458389}
Committed: https://chromium.googlesource.com/chromium/src/+/d2dea3d9ee934aca2815a667861c15c711e673f0
Patch Set 1 #
Total comments: 9
Patch Set 2 : Second version, set flag in ArcKioskAppManager::UpdateApps #
Total comments: 7
Patch Set 3 : Luis' suggestions #Patch Set 4 : Fixed test #
Total comments: 4
Patch Set 5 : Achuith's nits #
Total comments: 2
Messages
Total messages: 48 (31 generated)
Description was changed from ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. BUG=677216 ========== to ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. BUG=677216 TEST=Manually tested new behaviour. ==========
Description was changed from ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. BUG=677216 TEST=Manually tested new behaviour. ========== to ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. P.S. I've also fixed some linter complaints (missing includes). BUG=677216 TEST=Manually tested new behaviour. ==========
isandrk@chromium.org changed reviewers: + bartfab@chromium.org, peletskyi@chromium.org, poromov@chromium.org
Hey guys, I'm guessing all of you have touched parts of this code base and I'm not very familiar with it so I'm suggesting Bartosz choose the right reviewer(s) (OWNERs can chime in later, right?). I've used the approach that you can see, but I'm wondering how much is auto_launch_account_id_ reliable? I think I can discard most of the code if I rely on it and just set a boolean flag. PTAL and chime in with your opinion please :-) https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/app... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:180: return currently_auto_launched_with_zero_delay_account_id_; Second option is I don't set a boolean flag at all and just check kAccountsPrefDeviceLocalAccountAutoLoginDelay here. https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/app... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/app... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h:96: AccountId auto_launch_account_id_; Here's the auto_launch_account_id_ https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/arc_kiosk_controller.cc:48: if (is_auto_launch) { If I rely on auto_launch_account_id_, I can skip the whole part where I wire is_auto_launch around the code, and just set a boolean here.
So I meant Bartosz should decide who of you three should review which part of this (or you can all review it all) :-)
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/arc_kiosk_controller.cc:49: int auto_launch_delay = -1; nit: why not do int auto_launch_delay = 0; CrosSettings::Get()->GetInteger(...); if (auto_launch_delay == 0) ... ? https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/arc_kiosk_controller.cc:51: kAccountsPrefDeviceLocalAccountAutoLoginDelay, dumb question: can you not get this from ArcKioskAppManager::UpdateApps()? That way you can expose the launch delay from ArcKioskAppManager instead of exposing an account id which is seems like it might drift from the value of ArcKioskAppManager::GetAutoLaunchAccountId(). https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:259: !user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp()) nit: you cannot elide the brace here since the condition spans more than one line.
The CQ bit was checked by isandrk@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.
Thanks for the drive-by Luis! :-) https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/arc_kiosk_controller.cc (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/arc_kiosk_controller.cc:49: int auto_launch_delay = -1; On 2017/03/16 19:42:30, Luis Héctor Chávez wrote: > nit: why not do > > int auto_launch_delay = 0; > CrosSettings::Get()->GetInteger(...); > > if (auto_launch_delay == 0) > ... > > ? I guess I was going by this code and tried replicating it (and also wanted to add clarifying question but forgot in the meantime) https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/app_launch... Is it true what it says there? (Kiosks do not support non-zero auto-login delays) https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/arc_kiosk_controller.cc:51: kAccountsPrefDeviceLocalAccountAutoLoginDelay, On 2017/03/16 19:42:30, Luis Héctor Chávez wrote: > dumb question: can you not get this from ArcKioskAppManager::UpdateApps()? That > way you can expose the launch delay from ArcKioskAppManager instead of exposing > an account id which is seems like it might drift from the value of > ArcKioskAppManager::GetAutoLaunchAccountId(). Good question! It did cross my mind actually, that's probably the best place to put it. Thanks for the suggestion, I'll make the code changes tomorrow from the office. https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:259: !user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp()) On 2017/03/16 19:42:30, Luis Héctor Chávez wrote: > nit: you cannot elide the brace here since the condition spans more than one > line. Is there a rule about this in the style guide? I never saw it and didn't manage to find it now. And I've actually seen other code written like this.
The CQ bit was checked by isandrk@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...
ptal!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks, this looks much nicer! https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:210: if (auto_launch_delay == 0) nit: auto_launched_with_zero_delay_ = auto_launch_delay == 0; https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h:84: bool CurrentAppWasAutoLaunchedWithZeroDelay() const; this is a trivial accessor, so you can inline it: bool current_app_was_...() const { return auto_launched_...; } https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:258: !user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp()) nit: add braces. re: style guide, I think it's one of those things where the guide is silent about but most reviewers tend to request it. I'll try to see if there is consensus to amend the Chromium C++ style guide to explicitly mention this.
The CQ bit was checked by isandrk@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...
Hey Luis, ptal again! Bartosz, please take a look! https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:210: if (auto_launch_delay == 0) On 2017/03/17 17:46:54, Luis Héctor Chávez wrote: > nit: auto_launched_with_zero_delay_ = auto_launch_delay == 0; Done. https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h:84: bool CurrentAppWasAutoLaunchedWithZeroDelay() const; On 2017/03/17 17:46:54, Luis Héctor Chávez wrote: > this is a trivial accessor, so you can inline it: > > bool current_app_was_...() const { return auto_launched_...; } Done. https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:258: !user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp()) On 2017/03/17 17:46:54, Luis Héctor Chávez wrote: > nit: add braces. > > re: style guide, I think it's one of those things where the guide is silent > about but most reviewers tend to request it. I'll try to see if there is > consensus to amend the Chromium C++ style guide to explicitly mention this. Done. I'm just wondering what's the reasoning on this one (I like knowing all the why's behind the rules) :-)
isandrk@chromium.org changed reviewers: - peletskyi@chromium.org, poromov@chromium.org
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 isandrk@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 https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:258: !user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp()) On 2017/03/20 13:22:58, Ivan Šandrk wrote: > On 2017/03/17 17:46:54, Luis Héctor Chávez wrote: > > nit: add braces. > > > > re: style guide, I think it's one of those things where the guide is silent > > about but most reviewers tend to request it. I'll try to see if there is > > consensus to amend the Chromium C++ style guide to explicitly mention this. > > Done. I'm just wondering what's the reasoning on this one (I like knowing all > the why's behind the rules) :-) The rule for eliding braces is that all statements in the if/else chain should be one-liners for both condition and body. You can think of brace elision as something to reduce the number of lines needed to express something, but only when it is totally unambiguous and fits a certain "visual pattern" that is easily identifiable at a glance. If the condition does not fit in one line, that visual pattern is lost.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
poromov@chromium.org changed reviewers: + poromov@chromium.org
lgtm
isandrk@chromium.org changed reviewers: + achuith@chromium.org
Achuith, ptal!
On 2017/03/20 17:01:40, Ivan Šandrk wrote: > Achuith, ptal! Drive-by/FYI: In the CL description, > P.S. I've also fixed some linter complaints (missing includes). looks out-dated? (The code LGTM).
lgtm https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h (right): https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h:98: bool auto_launched_with_zero_delay_; nit: initialize this here https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:697: if ((chromeos::KioskAppManager::Get()->GetApp(account->kiosk_app_id, What's the difference between these two conditions? Adding some temporary booleans would make this more readable.
Description was changed from ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. P.S. I've also fixed some linter complaints (missing includes). BUG=677216 TEST=Manually tested new behaviour. ========== to ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. BUG=677216 TEST=Manually tested new behaviour. ==========
The CQ bit was checked by isandrk@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...
Thanks everyone! https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h (right): https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.h:98: bool auto_launched_with_zero_delay_; On 2017/03/21 00:30:16, achuithb wrote: > nit: initialize this here Done. https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:697: if ((chromeos::KioskAppManager::Get()->GetApp(account->kiosk_app_id, On 2017/03/21 00:30:16, achuithb wrote: > What's the difference between these two conditions? > > Adding some temporary booleans would make this more readable. The first one queries the regular kiosk app manager for the current app, and gets back a structure containing info about the currently running app, and one of those is whether it was auto launched. The second one queries the ARC kiosk app manager and directly gets the info about the currently auto launched app. So basically the first one is just for regular (legacy) kiosk apps, and the second one is just for arc kiosk apps. Done.
The CQ bit was unchecked by isandrk@chromium.org
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, lhchavez@chromium.org, poromov@chromium.org, achuith@chromium.org Link to the patchset: https://codereview.chromium.org/2758593002/#ps80001 (title: "Achuith's nits")
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": 80001, "attempt_start_ts": 1490100526094910,
"parent_rev": "9ca8fc91927b335661ab095f925b1337486e64d0", "commit_rev":
"d2dea3d9ee934aca2815a667861c15c711e673f0"}
Message was sent while issue was closed.
Description was changed from ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. BUG=677216 TEST=Manually tested new behaviour. ========== to ========== Enable screen capture for ARC++ Kiosk Screenshots weren't being taken in ARC++ Kiosks because they take different code path compared to regular Kiosks. This CL adds code that enables screen capture also in ARC++ Kiosks. BUG=677216 TEST=Manually tested new behaviour. Review-Url: https://codereview.chromium.org/2758593002 Cr-Commit-Position: refs/heads/master@{#458389} Committed: https://chromium.googlesource.com/chromium/src/+/d2dea3d9ee934aca2815a667861c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d2dea3d9ee934aca2815a667861c...
Message was sent while issue was closed.
https://codereview.chromium.org/2758593002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2758593002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:697: bool regular_app_auto_launched_with_zero_delay = nit: const https://codereview.chromium.org/2758593002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector.cc:701: bool arc_app_auto_launched_with_zero_delay = nit: const
Message was sent while issue was closed.
You don't have to fix these unless you're planning any further changes to this file |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
