Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725113002:
Enable ARC kiosk mode always when ARC is available (Closed)
|
Created:
3 years, 9 months ago by elijahtaylor1 Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable ARC kiosk mode always when ARC is available
This removes 'installed-only-kiosk-supported' from arc-availability
BUG=697741
TEST=./unit_tests --gtest_filter="ChromeArcUtilTest.*"
TEST=./components_unittests --gtest_filter="ArcUtilTest.*"
Review-Url: https://codereview.chromium.org/2725113002
Cr-Commit-Position: refs/heads/master@{#454360}
Committed: https://chromium.googlesource.com/chromium/src/+/67e99111e67ac5aeae011a14154e054160bf98c4
Patch Set 1 #
Total comments: 10
Patch Set 2 : feedback #
Total comments: 2
Patch Set 3 : feedback2 #
Messages
Total messages: 26 (19 generated)
The CQ bit was checked by elijahtaylor@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 ========== Enable ARC kiosk mode always when ARC is available This removes 'installed-only-kiosk-supported' from arc-availability BUG=697741 TEST=./unit_tests --gtest_filter="ChromeArcUtilTest.*" TEST=./components_unittests --gtest_filter="ArcUtilTest.*" ========== to ========== Enable ARC kiosk mode always when ARC is available This removes 'installed-only-kiosk-supported' from arc-availability BUG=697741 TEST=./unit_tests --gtest_filter="ChromeArcUtilTest.*" TEST=./components_unittests --gtest_filter="ArcUtilTest.*" ==========
elijahtaylor@chromium.org changed reviewers: + hidehiko@chromium.org
PTAL, this is the change we discussed yesterday. It will need to be re-implemented in M57 slightly differently, but I want to get this in ahead of the branch if possible
https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:214: TEST_F(ChromeArcUtilTest, IsArcAllowedForProfile_KioskArcNotEnabled) { Could you avoid to use "enable". The word "enabled" are used in multiple meaning and we're getting rid of it at all. Maybe KioskArcInstalled? https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:226: TEST_F(ChromeArcUtilTest, IsArcAllowedForProfile_KioskArcEnabled) { Ditto. Maybe KioskArcAvailable? https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:228: {"", "--arc-availability=installed", "--enable-arc"}); This is already covered by KioskArcNotEnabled case. Could you use "--arc-availability=officially-supported" instead? https://codereview.chromium.org/2725113002/diff/1/components/arc/arc_util.cc File components/arc/arc_util.cc (right): https://codereview.chromium.org/2725113002/diff/1/components/arc/arc_util.cc#... components/arc/arc_util.cc:65: return true; Could you add "return IsArcAvailable();" here, too, with comment? If ArcAvailability is set, old flags are ignored. Let's keep consistency. https://codereview.chromium.org/2725113002/diff/1/components/arc/arc_util.cc#... components/arc/arc_util.cc:69: if (command_line->HasSwitch(chromeos::switches::kArcAvailable)) { nit: could you elide brace for consistency?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by elijahtaylor@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...
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
lgtm with one nit https://codereview.chromium.org/2725113002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2725113002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util_unittest.cc:228: {"", "--arc-availability=officially-supported", "--enable-arc"}); Does this work even after leaving out the "--enable-arc" flag? We're trying to get rid of it, and the arc-availability flag supersedes it: https://cs.chromium.org/chromium/src/components/arc/arc_util.cc?q=arc_util.cc... (or did you mean to use --arc-available?)
https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:214: TEST_F(ChromeArcUtilTest, IsArcAllowedForProfile_KioskArcNotEnabled) { On 2017/03/02 06:38:49, hidehiko wrote: > Could you avoid to use "enable". The word "enabled" are used in multiple meaning > and we're getting rid of it at all. > Maybe KioskArcInstalled? Done. https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:226: TEST_F(ChromeArcUtilTest, IsArcAllowedForProfile_KioskArcEnabled) { On 2017/03/02 06:38:49, hidehiko wrote: > Ditto. Maybe KioskArcAvailable? Done. *Supported https://codereview.chromium.org/2725113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util_unittest.cc:228: {"", "--arc-availability=installed", "--enable-arc"}); On 2017/03/02 06:38:49, hidehiko wrote: > This is already covered by KioskArcNotEnabled case. > Could you use "--arc-availability=officially-supported" instead? Done. the above case does =installed but not --arc-enabled. But using =officially-supported seems better still, because that's where we're transitioning to https://codereview.chromium.org/2725113002/diff/1/components/arc/arc_util.cc File components/arc/arc_util.cc (right): https://codereview.chromium.org/2725113002/diff/1/components/arc/arc_util.cc#... components/arc/arc_util.cc:65: return true; On 2017/03/02 06:38:49, hidehiko wrote: > Could you add "return IsArcAvailable();" here, too, with comment? > If ArcAvailability is set, old flags are ignored. Let's keep consistency. I will do this, but IMO this adds a cleanup task. Once the code at L68 is removed, this will be redundant and confusing unless changed https://codereview.chromium.org/2725113002/diff/1/components/arc/arc_util.cc#... components/arc/arc_util.cc:69: if (command_line->HasSwitch(chromeos::switches::kArcAvailable)) { On 2017/03/02 06:38:49, hidehiko wrote: > nit: could you elide brace for consistency? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by elijahtaylor@chromium.org to run a CQ dry run
https://codereview.chromium.org/2725113002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util_unittest.cc (right): https://codereview.chromium.org/2725113002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util_unittest.cc:228: {"", "--arc-availability=officially-supported", "--enable-arc"}); On 2017/03/02 18:42:00, Luis Héctor Chávez wrote: > Does this work even after leaving out the "--enable-arc" flag? We're trying to > get rid of it, and the arc-availability flag supersedes it: > https://cs.chromium.org/chromium/src/components/arc/arc_util.cc?q=arc_util.cc... > > (or did you mean to use --arc-available?) yes, you're right I should remove --enable-arc, everything works fine (since it does a fallback to IsArcAvailable)
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.
The CQ bit was checked by elijahtaylor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2725113002/#ps40001 (title: "feedback2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488485214432100,
"parent_rev": "7af696b5a11fcbdae617d237dbc06aaca6807f94", "commit_rev":
"67e99111e67ac5aeae011a14154e054160bf98c4"}
Message was sent while issue was closed.
Description was changed from ========== Enable ARC kiosk mode always when ARC is available This removes 'installed-only-kiosk-supported' from arc-availability BUG=697741 TEST=./unit_tests --gtest_filter="ChromeArcUtilTest.*" TEST=./components_unittests --gtest_filter="ArcUtilTest.*" ========== to ========== Enable ARC kiosk mode always when ARC is available This removes 'installed-only-kiosk-supported' from arc-availability BUG=697741 TEST=./unit_tests --gtest_filter="ChromeArcUtilTest.*" TEST=./components_unittests --gtest_filter="ArcUtilTest.*" Review-Url: https://codereview.chromium.org/2725113002 Cr-Commit-Position: refs/heads/master@{#454360} Committed: https://chromium.googlesource.com/chromium/src/+/67e99111e67ac5aeae011a14154e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/67e99111e67ac5aeae011a14154e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
