Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
320223002:
Add device policy: Kiosk Virtual Keyboard Layout. (Closed)
|
Created:
6 years, 6 months ago by rsadam Modified:
6 years, 6 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, joaodasilva+watch_chromium.org, jar+watch_chromium.org, asvitkine+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd device policy: Kiosk Virtual Keyboard Layout.
BUG=376900
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278317
Patch Set 1 : #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 3
Patch Set 4 : Attempt # 2 #
Total comments: 17
Patch Set 5 : #
Total comments: 34
Patch Set 6 : Fix typo. #
Total comments: 4
Patch Set 7 : #Patch Set 8 : Rebase to master. #
Total comments: 1
Messages
Total messages: 35 (0 generated)
Kevers, PTAL!
https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/app_... File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right): https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/app_... chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:397: // TODO(rsadam@): Use the policy specified the layout instead of simply s/specified the layout/specified layout https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/sett... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/sett... chrome/browser/chromeos/settings/device_settings_provider.cc:402: if (value->GetAsString(&layout)) { Drop brackets. https://codereview.chromium.org/320223002/diff/1/components/policy/resources/... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:4687: is given focus.''', Comment is not entirely accurate. Programmatic focus does not trigger the VK to appear. https://codereview.chromium.org/320223002/diff/1/ui/keyboard/keyboard_util.cc File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/320223002/diff/1/ui/keyboard/keyboard_util.cc... ui/keyboard/keyboard_util.cc:114: g_kiosk_keyboard_enabled; Indentation is off.
On 2014/06/09 13:59:12, kevers wrote: > https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/app_... > File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right): > > https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/app_... > chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:397: // TODO(rsadam@): Use > the policy specified the layout instead of simply > s/specified the layout/specified layout > > https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/sett... > File chrome/browser/chromeos/settings/device_settings_provider.cc (right): > > https://codereview.chromium.org/320223002/diff/1/chrome/browser/chromeos/sett... > chrome/browser/chromeos/settings/device_settings_provider.cc:402: if > (value->GetAsString(&layout)) { > Drop brackets. > > https://codereview.chromium.org/320223002/diff/1/components/policy/resources/... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/320223002/diff/1/components/policy/resources/... > components/policy/resources/policy_templates.json:4687: is given focus.''', > Comment is not entirely accurate. Programmatic focus does not trigger the VK to > appear. > > https://codereview.chromium.org/320223002/diff/1/ui/keyboard/keyboard_util.cc > File ui/keyboard/keyboard_util.cc (right): > > https://codereview.chromium.org/320223002/diff/1/ui/keyboard/keyboard_util.cc... > ui/keyboard/keyboard_util.cc:114: g_kiosk_keyboard_enabled; > Indentation is off. I accidentally deleted the patchset so I can't reply to the comments directly. But done for all of them!
https://codereview.chromium.org/320223002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:4678: 'supported_on': ['chrome_os:38-' ], s/38/37 if we're landing before the branch.
https://codereview.chromium.org/320223002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:4678: 'supported_on': ['chrome_os:38-' ], On 2014/06/09 15:51:37, kevers wrote: > s/38/37 if we're landing before the branch. Done.
ui/keyboard lgtm
+bartfab Hi Bartosz, PTAL!
https://codereview.chromium.org/320223002/diff/100001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/100001/components/policy/resou... components/policy/resources/policy_templates.json:4663: 'name': 'KioskVirtualKeyboardLayout', We try not to make special-case policies where a more generic use case exists. A virtual keyboard can be useful in all sessions, so the policy should apply to all session types. Also, I do not think it is a good idea to set the layout of the virtual keyboard by policy. Instead, we should have a policy that sets the keyboard layout (both physical and virtual, a generalization of crbug.com/214904) plus a second policy that toggles the virtual keyboard. https://codereview.chromium.org/320223002/diff/100001/components/policy/resou... components/policy/resources/policy_templates.json:4674: 'value': 'en-us', What is the plan for keeping this list up to date with the full set of keyboard supported on Chrome OS? https://codereview.chromium.org/320223002/diff/100001/components/policy/resou... components/policy/resources/policy_templates.json:4681: 'per_profile': True You need to make this a user policy. As long as it is a device policy, it will apply to the whole device and cannot be per-profile.
PTANL! BTW this CL is based of my refactor, so I'm assuming that will land first!
https://codereview.chromium.org/320223002/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/320223002/diff/120001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:368: { key::kTouchVirtualKeyboardEnabled, This needs to move to the #if defined(OS_CHROMEOS) block below because the pref exists on Chrome OS only. https://codereview.chromium.org/320223002/diff/120001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right): https://codereview.chromium.org/320223002/diff/120001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1717: if (!service->HasPrefPath(prefs::kVirtualKeyboardEnabled)) { Rather than checking whether the pref exists, just #ifdef this method so that it runs on platforms that have the pref only. https://codereview.chromium.org/320223002/diff/120001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1718: keyboard::SetKeyboardShowOverride(keyboard::KEYBOARD_SHOW_OVERRIDE_NONE); This does not look right to me: You take a per-profile setting and apply it to a browser-global setting. What if I log in with two profiles and switch between them? The keyboard setting should follow each profile's setting. https://codereview.chromium.org/320223002/diff/120001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/320223002/diff/120001/chrome/common/pref_name... chrome/common/pref_names.cc:937: const char kVirtualKeyboardEnabled[] = "ui.virtual_keyboard_enabled"; 1: Given that we have two different VKs, the pref name |kVirtualKeyboardEnabled| is ambiguous. I like the policy name you chose. Why not call the pref |kTocuchVirtualKeyboardEnabled| as well? 2: This needs to move to an #if defined(OS_CHROMEOS) block of the file. Also, the order in this file should match that in the corresponding header. Please move this pref right after |kFileSystemProviderMounted|, as in the header. https://codereview.chromium.org/320223002/diff/120001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/320223002/diff/120001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:2286: "TouchVirtualKeyboardEnabled": { You should provide a proper test case for this. Only device policies are exempt from this because they are set via a different mechanism that the test cannot easily invoke. https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:4668: 'dynamic_refresh': False, Can you make the code honor pref changes on the fly? It is good to support dynamic_refresh when there are no technical obstacles. https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:4673: 'caption': '''Enable Virtual Keyboard''', Nit: s/Virtual Keyboard/virtual keyboard/ https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:4674: 'desc': '''If unset this policy has no effect. If true, this enables the virtual keyboard. If false, this disables the virtual keyboard. If the accessibility virtual keyboard policy is set to true, it takes precedence over this.''', Take a look at the other policy descriptions. They are not 100% consistent but they do follow a rough pattern: There should be several paragraph. The first paragraph repeats what the policy is about, in a more verbose way than the pattern. The next paragraphs describe what the different policy values do. They should also state that when the policy is set, the user cannot override it. It is definitely a good idea to mention at that point how the policy interacts with the a11y keyboard. And finally, a last paragraph describes the default behavior when the policy is unset. https://codereview.chromium.org/320223002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/320223002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34889: + <int value="268" label="Enable Virtual Keyboard."/> Nit: s/Enable Virtual Keyboard./Enable virtual keyboard/ (remove capitalization and full stop at the end)
https://codereview.chromium.org/320223002/diff/120001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/320223002/diff/120001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:368: { key::kTouchVirtualKeyboardEnabled, On 2014/06/13 12:15:53, bartfab wrote: > This needs to move to the #if defined(OS_CHROMEOS) block below because the pref > exists on Chrome OS only. Done. https://codereview.chromium.org/320223002/diff/120001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right): https://codereview.chromium.org/320223002/diff/120001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1717: if (!service->HasPrefPath(prefs::kVirtualKeyboardEnabled)) { On 2014/06/13 12:15:53, bartfab wrote: > Rather than checking whether the pref exists, just #ifdef this method so that it > runs on platforms that have the pref only. Won't we need this check even if we ifdef? For example, in ChromeOS, prefs::kVirtualKeyboardEnabled will be defined, but it's not necessary that the administrator will set a value for this policy. https://codereview.chromium.org/320223002/diff/120001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1718: keyboard::SetKeyboardShowOverride(keyboard::KEYBOARD_SHOW_OVERRIDE_NONE); On 2014/06/13 12:15:53, bartfab wrote: > This does not look right to me: You take a per-profile setting and apply it to a > browser-global setting. What if I log in with two profiles and switch between > them? The keyboard setting should follow each profile's setting. I believe that this would follow the profile settings. There's a method ActiveUserChanged in chrome_launcher_controller that will cause the preference to be updated when you switch between profiles. https://codereview.chromium.org/320223002/diff/120001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/320223002/diff/120001/chrome/common/pref_name... chrome/common/pref_names.cc:937: const char kVirtualKeyboardEnabled[] = "ui.virtual_keyboard_enabled"; On 2014/06/13 12:15:53, bartfab wrote: > 1: Given that we have two different VKs, the pref name |kVirtualKeyboardEnabled| > is ambiguous. I like the policy name you chose. Why not call the pref > |kTocuchVirtualKeyboardEnabled| as well? > > 2: This needs to move to an #if defined(OS_CHROMEOS) block of the file. Also, > the order in this file should match that in the corresponding header. Please > move this pref right after |kFileSystemProviderMounted|, as in the header. Done. https://codereview.chromium.org/320223002/diff/120001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/320223002/diff/120001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:2286: "TouchVirtualKeyboardEnabled": { On 2014/06/13 12:15:53, bartfab wrote: > You should provide a proper test case for this. Only device policies are exempt > from this because they are set via a different mechanism that the test cannot > easily invoke. Done. https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:4668: 'dynamic_refresh': False, On 2014/06/13 12:15:53, bartfab wrote: > Can you make the code honor pref changes on the fly? It is good to support > dynamic_refresh when there are no technical obstacles. Done. https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:4673: 'caption': '''Enable Virtual Keyboard''', On 2014/06/13 12:15:53, bartfab wrote: > Nit: s/Virtual Keyboard/virtual keyboard/ Done. https://codereview.chromium.org/320223002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:4674: 'desc': '''If unset this policy has no effect. If true, this enables the virtual keyboard. If false, this disables the virtual keyboard. If the accessibility virtual keyboard policy is set to true, it takes precedence over this.''', On 2014/06/13 12:15:53, bartfab wrote: > Take a look at the other policy descriptions. They are not 100% consistent but > they do follow a rough pattern: There should be several paragraph. The first > paragraph repeats what the policy is about, in a more verbose way than the > pattern. The next paragraphs describe what the different policy values do. They > should also state that when the policy is set, the user cannot override it. It > is definitely a good idea to mention at that point how the policy interacts with > the a11y keyboard. And finally, a last paragraph describes the default behavior > when the policy is unset. Done.
https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2450: // Verify policy can enable the keyboard. Nit: There are two properties to test with each policy settings: 1) The policy should kick in immediately, enabling/disabling the VK. 2) The user should not be able to override. It would be nice to check both properties for both policy settings. Right now, you verify property 1) for policy value |true| and property 2) for policy value |false|. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2458: EXPECT_TRUE(keyboard::IsKeyboardEnabled()); Nit: As noted above, please check that the user cannot override the policy: keyboard::SetTouchKeyboardEnabled(false); EXPECT_TRUE(keyboard::IsKeyboardEnabled()); https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2466: UpdateProviderPolicy(policies); Nit: As noted above, please check that the policy kicks in immediately, before the user has even tried to override anything: EXPECT_FALSE(keyboard::IsKeyboardEnabled()); https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right): https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1716: #if defined(OS_CHROMEOS) Nit: Since this method does nothing on other platforms, why not #ifdef its entire declaration to Chrome OS only? https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1717: PrefService* service = profile_->GetPrefs(); Nit: const. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1721: bool enabled = service->GetBoolean(prefs::kTouchVirtualKeyboardEnabled); Nit: const. https://codereview.chromium.org/320223002/diff/140001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/320223002/diff/140001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:2108: "test_policy": {"TouchVirtualKeyboardEnabled": false}, Nit: pad with spaces inside the curly braces. https://codereview.chromium.org/320223002/diff/140001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:4676: If the policy is set to true, the virtual keyboard will be enabled and cannot be disabled. If set to false, the virtual keyboard will be disabled by default, however may appear if the user explicitly enables the accessibility keyboard. Nit: Maybe it would make sense to differentiate between the regular VK and a11y VK here? E.g. something along the lines of: If the policy is set to true, the on-screen virtual keyboard will always be enabled. If this policy is set to false, the on-screen keyboard will always be disabled. If you set this policy, users cannot change or override it. However, users will still be able to enable/disable an accessibility on-screen keyboard which takes precedence over the virtual keyboard controlled by this policy. See the |VirtualKeyboardEnabled| policy for controlling the accessibility on-screen keyboard. https://codereview.chromium.org/320223002/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:4678: If unset, this policy will have no effect - the virtual keyboard is disabled by default, but may appear if a user enables it. Heuristic rules may also be used to decide when to display the keyboard.''', Very minor nit: You could make this slightly more consistent with other policies, and especially the a11y keyboard policy, by rewording it to: If this policy is left unset, the on-screen keyboard is disabled initially but can be enabled by the user anytime. Heuristic rules may also be used to decide when to display the keyboard. https://codereview.chromium.org/320223002/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/320223002/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34889: + <int value="268" label="Enable virtual keyboard."/> Nit: Remove full stop at the end. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:50: keyboard::KeyboardShowOverride g_keyboard_show_override = Nit: Should this be in an #ifdef? It is not used on any platforms other than Chrome OS after all. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:88: void SetTouchKeyboardEnabled(bool enabled) { What UI is used to control this? Can we gray out this UI if a policy override is in effect? https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:104: if (g_accessibility_keyboard_enabled) Could you check whether this can be used to override the policy: * Admin sets policy to "force-enable VK" * VK shows * User enables accessibility keyboard, which takes precedence * A11y VK shows * I presume the regular VK hides automatically? * User disables accessibility keyboard * A11y VK hides Will the regular VK automatically re-show at that point? https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:112: IsKeyboardUsabilityExperimentEnabled() || g_touch_keyboard_enabled || Nit: Could you reformat this so there is one condition per line? It would be easier to read if g_touch_keyboard_enabled was on its own line. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:51: KEYBOARD_SHOW_OVERRIDE_ENABLED = 1, Nit 1: Why explicitly set the value of this constant to 1? C will automatically increment by 1 for each constant in an enum. Nit 2: Should this be in an #ifdef? It is not used on any platforms other than Chrome OS after all. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:92: // Sets policy override on whether to show the keyboard. I see that this is consistent with the rest of the file but I am stil surprised to see "keyboard" used everywhere. Would "virtual keyboard" not have been the better term? https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:93: KEYBOARD_EXPORT void SetKeyboardShowOverride(KeyboardShowOverride override); Nit: Should this be in an #ifdef? It is not used on any platforms other than Chrome OS after all.
https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2450: // Verify policy can enable the keyboard. On 2014/06/16 10:20:13, bartfab wrote: > Nit: There are two properties to test with each policy settings: > > 1) The policy should kick in immediately, enabling/disabling the VK. > 2) The user should not be able to override. > > It would be nice to check both properties for both policy settings. Right now, > you verify property 1) for policy value |true| and property 2) for policy value > |false|. Done. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2458: EXPECT_TRUE(keyboard::IsKeyboardEnabled()); On 2014/06/16 10:20:13, bartfab wrote: > Nit: As noted above, please check that the user cannot override the policy: > > keyboard::SetTouchKeyboardEnabled(false); > EXPECT_TRUE(keyboard::IsKeyboardEnabled()); Done. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2466: UpdateProviderPolicy(policies); On 2014/06/16 10:20:12, bartfab wrote: > Nit: As noted above, please check that the policy kicks in immediately, before > the user has even tried to override anything: > > EXPECT_FALSE(keyboard::IsKeyboardEnabled()); Done. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right): https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1716: #if defined(OS_CHROMEOS) On 2014/06/16 10:20:13, bartfab wrote: > Nit: Since this method does nothing on other platforms, why not #ifdef its > entire declaration to Chrome OS only? Done. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1717: PrefService* service = profile_->GetPrefs(); On 2014/06/16 10:20:13, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/320223002/diff/140001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1721: bool enabled = service->GetBoolean(prefs::kTouchVirtualKeyboardEnabled); On 2014/06/16 10:20:13, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/320223002/diff/140001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/320223002/diff/140001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:2108: "test_policy": {"TouchVirtualKeyboardEnabled": false}, On 2014/06/16 10:20:13, bartfab wrote: > Nit: pad with spaces inside the curly braces. Done. https://codereview.chromium.org/320223002/diff/140001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:4676: If the policy is set to true, the virtual keyboard will be enabled and cannot be disabled. If set to false, the virtual keyboard will be disabled by default, however may appear if the user explicitly enables the accessibility keyboard. On 2014/06/16 10:20:13, bartfab wrote: > Nit: Maybe it would make sense to differentiate between the regular VK and a11y > VK here? E.g. something along the lines of: > > If the policy is set to true, the on-screen virtual keyboard will always be > enabled. > > If this policy is set to false, the on-screen keyboard will always be disabled. > > If you set this policy, users cannot change or override it. However, users will > still be able to enable/disable an accessibility on-screen keyboard which takes > precedence over the virtual keyboard controlled by this policy. See the > |VirtualKeyboardEnabled| policy for controlling the accessibility on-screen > keyboard. Done. https://codereview.chromium.org/320223002/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:4678: If unset, this policy will have no effect - the virtual keyboard is disabled by default, but may appear if a user enables it. Heuristic rules may also be used to decide when to display the keyboard.''', On 2014/06/16 10:20:13, bartfab wrote: > Very minor nit: You could make this slightly more consistent with other > policies, and especially the a11y keyboard policy, by rewording it to: > > If this policy is left unset, the on-screen keyboard is disabled initially but > can be enabled by the user anytime. Heuristic rules may also be used to decide > when to display the keyboard. Done. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:50: keyboard::KeyboardShowOverride g_keyboard_show_override = On 2014/06/16 10:20:13, bartfab wrote: > Nit: Should this be in an #ifdef? It is not used on any platforms other than > Chrome OS after all. Tthe virtual keyboard is only ever created on CHROME_OS- I believe this entire file is already chromeos only. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:88: void SetTouchKeyboardEnabled(bool enabled) { On 2014/06/16 10:20:13, bartfab wrote: > What UI is used to control this? Can we gray out this UI if a policy override is > in effect? There isn't a UI for this - it's enabled when the user enters TouchView mode, which is triggered on certain combinations of screen rotations. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:104: if (g_accessibility_keyboard_enabled) On 2014/06/16 10:20:13, bartfab wrote: > Could you check whether this can be used to override the policy: > > * Admin sets policy to "force-enable VK" > * VK shows > * User enables accessibility keyboard, which takes precedence > * A11y VK shows > * I presume the regular VK hides automatically? > * User disables accessibility keyboard > * A11y VK hides > > Will the regular VK automatically re-show at that point? The keyboard only shows on text input focus - but yes it does reappear. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:112: IsKeyboardUsabilityExperimentEnabled() || g_touch_keyboard_enabled || On 2014/06/16 10:20:13, bartfab wrote: > Nit: Could you reformat this so there is one condition per line? It would be > easier to read if g_touch_keyboard_enabled was on its own line. Done. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:51: KEYBOARD_SHOW_OVERRIDE_ENABLED = 1, On 2014/06/16 10:20:13, bartfab wrote: > Nit 1: Why explicitly set the value of this constant to 1? C will automatically > increment by 1 for each constant in an enum. Done. > Nit 2: Should this be in an #ifdef? It is not used on any platforms other than > Chrome OS after all. All of keyboard_util is CHROMEOS only in the sense that the keyboard is only ever created on CHROMEOS. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:92: // Sets policy override on whether to show the keyboard. On 2014/06/16 10:20:13, bartfab wrote: > I see that this is consistent with the rest of the file but I am stil surprised > to see "keyboard" used everywhere. Would "virtual keyboard" not have been the > better term? I'm not sure why one was chosen over the other - this file predates me joining the team. I'll ask my TL about it - perhaps it makes sense to have a follow up CL to refactor this file. https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:93: KEYBOARD_EXPORT void SetKeyboardShowOverride(KeyboardShowOverride override); On 2014/06/16 10:20:13, bartfab wrote: > Nit: Should this be in an #ifdef? It is not used on any platforms other than > Chrome OS after all. The keyboard is only instantiated in chromeos so this might not be necessary.
https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/320223002/diff/140001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:50: keyboard::KeyboardShowOverride g_keyboard_show_override = On 2014/06/16 13:45:35, rsadam wrote: > On 2014/06/16 10:20:13, bartfab wrote: > > Nit: Should this be in an #ifdef? It is not used on any platforms other than > > Chrome OS after all. > > Tthe virtual keyboard is only ever created on CHROME_OS- I believe this entire > file is already chromeos only. I see nothing in the gyp files excluding this file from compilation on other ash platforms. https://codereview.chromium.org/320223002/diff/180001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/320223002/diff/180001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2462: // Verify that user cannot set keyboard enabled when policy disables it. Nit: This should be made consistent with the comment in line 2450. Both test cases verify that policy takes effect and then that the user cannot overrule policy. https://codereview.chromium.org/320223002/diff/180001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right): https://codereview.chromium.org/320223002/diff/180001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2054: pref_change_registrar_.Add( This will need to be #ifdef'd as well.
Sorry I should have phrased my initial response better. So it's true that the keyboard code can be run on any ash platform, however, we decided to guard for chromeos only when we create the keyboard - so all the creation code is chromeos only. So in that sense, the keyboard is chromeos only. Given that everything in this file is only intended for chromeos, i'm not sure it makes sense to introduce ifdefs just for this boolean. https://codereview.chromium.org/320223002/diff/180001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/320223002/diff/180001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:2462: // Verify that user cannot set keyboard enabled when policy disables it. On 2014/06/16 14:14:02, bartfab wrote: > Nit: This should be made consistent with the comment in line 2450. Both test > cases verify that policy takes effect and then that the user cannot overrule > policy. Done. https://codereview.chromium.org/320223002/diff/180001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right): https://codereview.chromium.org/320223002/diff/180001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2054: pref_change_registrar_.Add( On 2014/06/16 14:14:02, bartfab wrote: > This will need to be #ifdef'd as well. Done.
histograms lgtm
actually, this has a conflict with https://codereview.chromium.org/309553011/ which looks to be changing the same enum and is already in the cq. You probably want to rebase on that once it lands
On 2014/06/16 14:40:36, Alexei Svitkine wrote: > actually, this has a conflict with https://codereview.chromium.org/309553011/ > which looks to be changing the same enum and is already in the cq. You probably > want to rebase on that once it lands Thanks for the heads up Alexei :) Will do!
On 2014/06/16 14:42:57, rsadam wrote: > On 2014/06/16 14:40:36, Alexei Svitkine wrote: > > actually, this has a conflict with https://codereview.chromium.org/309553011/ > > which looks to be changing the same enum and is already in the cq. You > probably > > want to rebase on that once it lands > > Thanks for the heads up Alexei :) Will do! That's absolutely normal when working on policies. Whoever gets in first gets the next policy number. Just rebase - no new reviews needed.
On 2014/06/16 14:42:57, rsadam wrote: > On 2014/06/16 14:40:36, Alexei Svitkine wrote: > > actually, this has a conflict with https://codereview.chromium.org/309553011/ > > which looks to be changing the same enum and is already in the cq. You > probably > > want to rebase on that once it lands > > Thanks for the heads up Alexei :) Will do! That's absolutely normal when working on policies. Whoever gets in first gets the next policy number. Just rebase - no new reviews needed.
On 2014/06/16 14:42:57, rsadam wrote: > On 2014/06/16 14:40:36, Alexei Svitkine wrote: > > actually, this has a conflict with https://codereview.chromium.org/309553011/ > > which looks to be changing the same enum and is already in the cq. You > probably > > want to rebase on that once it lands > > Thanks for the heads up Alexei :) Will do! That's absolutely normal when working on policies. Whoever gets in first gets the next policy number. Just rebase - no new reviews needed.
lgtm
+davemoore for OWNERS ui/ash/launcher/* chromeos/preferences.cc
+Joao for OWNERS policy_templates.json
Rebase to master.
On 2014/06/16 18:24:06, rsadam wrote: > Rebase to master. ping :)
Thanks for the ping, somehow this escaped my attention yesterday. I'm assuming the per_profile behavior is correct, so lgtm. Please see inline. https://codereview.chromium.org/320223002/diff/220001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/320223002/diff/220001/components/policy/resou... components/policy/resources/policy_templates.json:4705: 'per_profile': True Please confirm that this policy works with multiprofiles on: if I have multiple profiles and switch back and forth, then the policy always applies using the value for the current active profile, is that correct? I think that's the case due to the call to SetVirtualKeyboardBehaviorFromPrefs() from ChromeLauncherController::ActiveUserChanged() but just want to make sure.
On 2014/06/17 16:38:26, Joao da Silva wrote: > Thanks for the ping, somehow this escaped my attention yesterday. > > I'm assuming the per_profile behavior is correct, so lgtm. Please see inline. > > https://codereview.chromium.org/320223002/diff/220001/components/policy/resou... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/320223002/diff/220001/components/policy/resou... > components/policy/resources/policy_templates.json:4705: 'per_profile': True > Please confirm that this policy works with multiprofiles on: if I have multiple > profiles and switch back and forth, then the policy always applies using the > value for the current active profile, is that correct? > > I think that's the case due to the call to SetVirtualKeyboardBehaviorFromPrefs() > from ChromeLauncherController::ActiveUserChanged() but just want to make sure. Yes - in the multiprofile case ActiveUserChanged() would be called, updating the preference based on the profile that's currently being used.
lgtm
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/320223002/220001
The CQ bit was unchecked by rsadam@chromium.org
The CQ bit was checked by rsadam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsadam@chromium.org/320223002/220001
Message was sent while issue was closed.
Change committed as 278317 |
