Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964823002:
Remove confusing keyboard test & focus on input device (Closed)
|
Created:
3 years, 5 months ago by Felix Ekblom Modified:
3 years, 5 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, mnilsson Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove confusing keyboard test & inspect input device
In order to make the decision for when to trigger the OOBE display
chooser less confusing for end users I'm removing the keyboard check.
Instead an explicit remora requisition check is used to limit the
effects.
BUG=738885
Review-Url: https://codereview.chromium.org/2964823002
Cr-Commit-Position: refs/heads/master@{#485019}
Committed: https://chromium.googlesource.com/chromium/src/+/c923b817665a649b4e18243116c31c68404af7dd
Patch Set 1 #Patch Set 2 : Limit scope of the window moving code #Patch Set 3 : Update tests. Using TouchscreenDevice made the tests a bit more complex #Patch Set 4 : Simplify device ids to minimize noise #Patch Set 5 : Break out device check code #
Total comments: 5
Patch Set 6 : Use curly braces constructor #Patch Set 7 : Clean up matching #
Total comments: 3
Patch Set 8 : Code structure improvements #
Total comments: 11
Patch Set 9 : Remove unnecessary check & include. Rename vector #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== Remove confusing keyboard test & focus on input device BUG=738885 ========== to ========== Remove confusing keyboard test & focus on input device BUG=738885 ==========
felixe@chromium.org changed reviewers: + jdufault@chromium.org
Description was changed from ========== Remove confusing keyboard test & focus on input device BUG=738885 ========== to ========== Remove confusing keyboard test & focus on input device Make OobeDisplayChooser run only for remora. BUG=738885 ==========
The CQ bit was checked by felixe@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 ========== Remove confusing keyboard test & focus on input device Make OobeDisplayChooser run only for remora. BUG=738885 ========== to ========== Remove confusing keyboard test & inspect input device In order to make the decision for when to trigger the OOBE display chooser less confusing for end users I'm removing the keyboard check. Instead an explicit remora requisition check is used to limit the effects. BUG=738885 ==========
Description was changed from ========== Remove confusing keyboard test & inspect input device In order to make the decision for when to trigger the OOBE display chooser less confusing for end users I'm removing the keyboard check. Instead an explicit remora requisition check is used to limit the effects. BUG=738885 ========== to ========== Remove confusing keyboard test & inspect input device In order to make the decision for when to trigger the OOBE display chooser less confusing for end users I'm removing the keyboard check. Instead an explicit remora requisition check is used to limit the effects. BUG=738885 ==========
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 felixe@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.
Please also have oshima@ take a look. https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:37: std::vector<ui::TouchscreenDevice> vec; nit: Use {} constructor, ie, std::vector<ui::TouchscreenDevice> devices { touchscreen }; https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:374: if (!ash_util::IsRunningInMash() && IsRemoraRequisitioned()) Why do we want to restrict this?
felixe@chromium.org changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:37: std::vector<ui::TouchscreenDevice> vec; On 2017/07/06 18:19:35, jdufault wrote: > nit: Use {} constructor, ie, > > std::vector<ui::TouchscreenDevice> devices { touchscreen }; Done. https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:374: if (!ash_util::IsRunningInMash() && IsRemoraRequisitioned()) On 2017/07/06 18:19:35, jdufault wrote: > Why do we want to restrict this? I added this as a way to minimize the risk of changed behavior on Chromebooks and other non-touch devices on which you can attach displays. It fills the same purpose (and same filter effect) as the keyboard check it replaces, although not in quite the same nice, general way. I'm hoping to land this in M60 so I'm erring on the side of caution as to side effects in other products. Is there another preferred way to get the same effect?
https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2964823002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:374: if (!ash_util::IsRunningInMash() && IsRemoraRequisitioned()) On 2017/07/06 19:37:10, Felix Ekblom wrote: > On 2017/07/06 18:19:35, jdufault wrote: > > Why do we want to restrict this? > > I added this as a way to minimize the risk of changed behavior on Chromebooks > and other non-touch devices on which you can attach displays. It fills the same > purpose (and same filter effect) as the keyboard check it replaces, although not > in quite the same nice, general way. I'm hoping to land this in M60 so I'm > erring on the side of caution as to side effects in other products. > > Is there another preferred way to get the same effect? Not that I'm aware of. https://codereview.chromium.org/2964823002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2964823002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:8: nit: merge these two include blocks https://codereview.chromium.org/2964823002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:70: if (base::ContainsValue(kDeviceIds, device.vendor_id)) { This code may be simpler with inverted ifs, ie, for (...) { if (...) continue; if (...) continue; SetPrimaryDisplayId(...); break; } https://codereview.chromium.org/2964823002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:102: touchscreen.vendor_id = 0x266e; Please extract into named constant (ideally shared with oobe_display_chooser.cc)
I assume PMs are okay with this? We may also need to communicate with vendor/devs, in case they're using something else. https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:62: if (!ui::DeviceDataManager::HasInstance()) can this happen? https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:41: manager->OnTouchscreenDevicesUpdated(vec); just inline GetInstnace(). It should fit single online.
https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:62: if (!ui::DeviceDataManager::HasInstance()) On 2017/07/06 20:53:36, oshima wrote: > can this happen? In theory, yes, since GetInstance() does not create the instance. In practice, I don't think so but I don't know for sure. Are you suggesting it should be safe to remove it? https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:41: manager->OnTouchscreenDevicesUpdated(vec); On 2017/07/06 20:53:36, oshima wrote: > just inline GetInstnace(). It should fit single online. OnTouchscreenDevicesUpdated() is declared public in DeviceHotplugEventObserver but DeviceDataManager has re-declared it as protected. Removing the (implicit) cast breaks compilation.
https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:8: #include <algorithm> is this necessary? https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:62: if (!ui::DeviceDataManager::HasInstance()) On 2017/07/07 08:41:01, Felix Ekblom wrote: > On 2017/07/06 20:53:36, oshima wrote: > > can this happen? > > In theory, yes, since GetInstance() does not create the instance. In practice, I > don't think so but I don't know for sure. > > Are you suggesting it should be safe to remove it? If it should not happen, then remove check and fix if it happens. Handling erroneous condition isn't recommended because it'll eventually leads to wrong behavior anyway. You may add DCHECK(device_manager) below to help diagnose if you want to troubleshoot such case in bots, or local build. https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:41: manager->OnTouchscreenDevicesUpdated(vec); On 2017/07/07 08:41:01, Felix Ekblom wrote: > On 2017/07/06 20:53:36, oshima wrote: > > just inline GetInstnace(). It should fit single online. > > OnTouchscreenDevicesUpdated() is declared public in DeviceHotplugEventObserver > but DeviceDataManager has re-declared it as protected. Removing the (implicit) > cast breaks compilation. That's unfortunate. Okay, let's blame C++. (it doesn't make sense and not possible in java, but anyway...)
lgtm https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:37: std::vector<ui::TouchscreenDevice> vec{touchscreen}; nit: rename vec to devices
https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:8: #include <algorithm> On 2017/07/07 14:22:10, oshima wrote: > is this necessary? Removed. https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:62: if (!ui::DeviceDataManager::HasInstance()) On 2017/07/07 14:22:10, oshima wrote: > On 2017/07/07 08:41:01, Felix Ekblom wrote: > > On 2017/07/06 20:53:36, oshima wrote: > > > can this happen? > > > > In theory, yes, since GetInstance() does not create the instance. In practice, > I > > don't think so but I don't know for sure. > > > > Are you suggesting it should be safe to remove it? > > If it should not happen, then remove check and fix if it happens. > Handling erroneous condition isn't recommended because it'll eventually leads to > wrong behavior anyway. > > You may add DCHECK(device_manager) below to help diagnose if you want to > troubleshoot such case in bots, or local build. Very good point. Done. https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2964823002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:37: std::vector<ui::TouchscreenDevice> vec{touchscreen}; On 2017/07/07 16:56:10, jdufault wrote: > nit: rename vec to devices Done.
lgtm
The CQ bit was checked by felixe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2964823002/#ps160001 (title: "Remove unnecessary check & include. Rename vector")
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": 160001, "attempt_start_ts": 1499456311991220,
"parent_rev": "fe34199b2b62a9ed333cd09b227bd9e1bf5a070e", "commit_rev":
"c923b817665a649b4e18243116c31c68404af7dd"}
Message was sent while issue was closed.
Description was changed from ========== Remove confusing keyboard test & inspect input device In order to make the decision for when to trigger the OOBE display chooser less confusing for end users I'm removing the keyboard check. Instead an explicit remora requisition check is used to limit the effects. BUG=738885 ========== to ========== Remove confusing keyboard test & inspect input device In order to make the decision for when to trigger the OOBE display chooser less confusing for end users I'm removing the keyboard check. Instead an explicit remora requisition check is used to limit the effects. BUG=738885 Review-Url: https://codereview.chromium.org/2964823002 Cr-Commit-Position: refs/heads/master@{#485019} Committed: https://chromium.googlesource.com/chromium/src/+/c923b817665a649b4e18243116c3... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c923b817665a649b4e18243116c3...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2977293002/ by felixe@chromium.org. The reason for reverting is: A follow up fix for a race condition on cold boot was reverted in https://chromium-review.googlesource.com/c/574731/. With only this patch the functionality is broken in the user facing scenario. The code state without either this cl or the follow up is better than with only this cl.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
