Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865003003:
Put OOBE UI on touch display if no keyboard detected (Closed)
|
Created:
3 years, 7 months ago by Felix Ekblom Modified:
3 years, 7 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPut OOBE UI on touch display if no keyboard detected
This will enable devices without a keyboard but with a touch display to go
through the OOBE setup.
BUG=668449
Review-Url: https://codereview.chromium.org/2865003003
Cr-Commit-Position: refs/heads/master@{#470912}
Committed: https://chromium.googlesource.com/chromium/src/+/bcbc0c40b5987b07425a9bad2f9dadfeb4612182
Patch Set 1 #
Total comments: 26
Patch Set 2 : Address most (all?) code comments up to #7 #
Total comments: 6
Patch Set 3 : Handle display removal case + rename method as suggested by jdufault@ #Patch Set 4 : Remove non-obvious auto usage #Patch Set 5 : Add <memory> include for std::unique_ptr #Patch Set 6 : OobeDisplayChooser can't handle mustash #
Total comments: 1
Patch Set 7 : Add TODO and link to bug #
Messages
Total messages: 33 (18 generated)
Description was changed from ========== Put OOBE UI on touch display if no keyboard detected This will enable devices without a keyboard but with a touch display to go through the OOBE setup. BUG=668449 ========== to ========== Put OOBE UI on touch display if no keyboard detected This will enable devices without a keyboard but with a touch display to go through the OOBE setup. BUG=668449 ==========
felixe@chromium.org changed reviewers: + alemate@chromium.org, jdufault@chromium.org, oshima@chromium.org
felixe@chromium.org changed reviewers: - jdufault@chromium.org
jdufault@chromium.org changed reviewers: + jdufault@chromium.org
https://codereview.chromium.org/2865003003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:979: GetOobeUI()->OnDisplayAdded(new_display); nit: drop {} https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:41: void OobeDisplayChooser::TryToPlaceUIOnTouchDisplay() { nit: UI -> Ui https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:42: auto primary_display = screen_->GetPrimaryDisplay(); nit: auto type is not obvious https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:44: MoveToTouchDisplay(); nit: drop {} https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:48: void OobeDisplayChooser::OnDisplayAdded(const display::Display& new_display) { |new_display| is never used, so remove this function and call TryToPlaceUiOnTouchDisplay directly https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:53: auto& displays = display_manager_->active_only_display_list(); nit: auto type is not obvious https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:55: if (displays.size() <= 1) { nit: drop {} https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:62: return; break? https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h:29: OobeDisplayChooser(ash::DisplayConfigurationController* dcc, Add comment https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:19: class OobeDisplayChooserTest : public ash::test::AshTestBase { Put test fixture in anonymous namespace. https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:20: protected: protected => public/private, add DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:27: void TearDown() override { ash::test::AshTestBase::TearDown(); } Remove? https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:81: auto ids = display_manager()->GetCurrentDisplayIdList(); nit: auto type is not obvious (for this entire file) https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:215: auto& keyboards = ui::InputDeviceManager::GetInstance()->GetKeyboardDevices(); nit: auto type is not obvious https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:587: oobe_display_chooser_->TryToPlaceUIOnTouchDisplay(); nit: no {} https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:639: oobe_display_chooser_->OnDisplayAdded(new_display); nit: no {} https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_ui.h (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.h:174: void OnDisplayAdded(const display::Display& new_display); |new_display| is unused, remove it. Is OnDisplayConfigurationChanged a better name? What happens when we remove a display? https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.h:174: void OnDisplayAdded(const display::Display& new_display); Add comment. https://codereview.chromium.org/2865003003/diff/1/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/2865003003/diff/1/chromeos/chromeos_switches.... chromeos/chromeos_switches.cc:420: const char kOobePreferTouchDisplay[] = "oobe-prefer-touch-display"; Why not make this the default and remove the flag?
what is expected behavior if a user connected keyboard (and mouse) after it is booted? https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:37: screen_(screen) {} Just getting these from Shell in the methods below would be simpler. https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: nuke "(c)" It's old style and obsolete. Same for the other files. https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h:27: static std::unique_ptr<OobeDisplayChooser> CreateDefault(); Since this is the only usage, you can just call it "Create()" https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h:31: display::Screen* screen); does this have to be public?
https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:37: screen_(screen) {} On 2017/05/09 05:58:45, oshima wrote: > Just getting these from Shell in the methods below would be simpler. Yes, fixed now. I was hoping to mock them for the unit tests but it turned out I had to use AshTestbase, making this unnecessary. https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_ui.h (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.h:174: void OnDisplayAdded(const display::Display& new_display); On 2017/05/08 17:44:58, jdufault wrote: > |new_display| is unused, remove it. Is OnDisplayConfigurationChanged a better > name? What happens when we remove a display? I've assumed only one touch display for the initial work on this task to simplify as much as possible. Reacting to OnDisplayRemoved() could be interesting for a follow up CL, as well as using a better display choosing heuristic, if we assume multiple connected touch displays.
On 2017/05/09 05:58:45, oshima wrote: > what is expected behavior if a user connected keyboard (and mouse) after it is > booted? From my side, I expect the UI to remain on the touch display until changed by the user (or the app started after OOBE has finished). If there are devices on which this code will trigger but where we don't want to leave the touch display as primary after OOBE then we can of course reconsider the current approach of leaving the touch display as primary.
lgtm https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:37: auto& displays = nit: auto& type is not obvious https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.h (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.h:248: std::unique_ptr<OobeDisplayChooser> oobe_display_chooser_; Consider using base::Optional, up to you.
lgtm this CL once you address the following You may want to support keyboard removal scenario, but you can do it in a separate CL. https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:980: } I think you need to handle removed case. - You have one non touch + two touch displays - Removing a touch display that is used as a primary will change the display configuration. It may or may not use the touch capable one. https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:47: ->SetPrimaryDisplayId(display.id()); IIRC that this change will be saved locally, so this will become a default when it's rebooted even with keyboard connected. Just FYI.
lgtm with oshimas comments addressed.
On 2017/05/10 00:14:31, oshima wrote: > > You may want to support keyboard removal scenario, but you can do it in a > separate CL. Great, I'll treat that as a separate task then.
https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_ui.h (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.h:174: void OnDisplayAdded(const display::Display& new_display); On 2017/05/08 17:44:58, jdufault wrote: > |new_display| is unused, remove it. Is OnDisplayConfigurationChanged a better > name? What happens when we remove a display? > I renamed the method according to your suggestion as a part of Patch set 3. https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:980: } On 2017/05/10 00:14:31, oshima wrote: > I think you need to handle removed case. > > - You have one non touch + two touch displays > - Removing a touch display that is used as a primary will change > the display configuration. It may or may not use the touch capable one. Addressed in Patch set 3 by triggering a re-evaluation of the display usage on display removal as well. https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:47: ->SetPrimaryDisplayId(display.id()); On 2017/05/10 00:14:31, oshima wrote: > IIRC that this change will be saved locally, so > this will become a default when it's rebooted even with keyboard connected. > > Just FYI. Thanks for pointing it out. I believe this to be OK for our use cases. If some problematic edge case turn up I'll revisit this in a bug fix.
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 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...
Patch set 6 add a guard to prevent OobeDisplayChooser creation in mus+ash / MASH mode as there's currently no to implement the object in that mode (concluded in discussion with kylechar). Many similar guards already exist in LoginDisplayHostImpl.
https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:358: if (!ash_util::IsRunningInMash() && !IsKeyboardConnected()) File a bug, add TODO
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/10 20:29:02, jdufault wrote: > https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): > > https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webu... > chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:358: if > (!ash_util::IsRunningInMash() && !IsKeyboardConnected()) > File a bug, add TODO (still lgtm)
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by felixe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2865003003/#ps120001 (title: "Add TODO and link to bug")
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": 120001, "attempt_start_ts": 1494491285828790,
"parent_rev": "8e533cacdaf5b39f3d961490878d7a64c7368892", "commit_rev":
"bcbc0c40b5987b07425a9bad2f9dadfeb4612182"}
Message was sent while issue was closed.
Description was changed from ========== Put OOBE UI on touch display if no keyboard detected This will enable devices without a keyboard but with a touch display to go through the OOBE setup. BUG=668449 ========== to ========== Put OOBE UI on touch display if no keyboard detected This will enable devices without a keyboard but with a touch display to go through the OOBE setup. BUG=668449 Review-Url: https://codereview.chromium.org/2865003003 Cr-Commit-Position: refs/heads/master@{#470912} Committed: https://chromium.googlesource.com/chromium/src/+/bcbc0c40b5987b07425a9bad2f9d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/bcbc0c40b5987b07425a9bad2f9d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
