Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(127)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -1 line) Patch
M chrome/browser/chromeos/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/login/ui/login_display_host_impl.cc View 1 2 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h View 1 1 chunk +28 lines, -0 lines 0 comments Download
A chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc View 1 2 3 1 chunk +54 lines, -0 lines 0 comments Download
A chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc View 1 2 3 4 1 chunk +84 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/oobe_ui.h View 1 2 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/oobe_ui.cc View 1 2 3 4 5 6 6 chunks +30 lines, -0 lines 0 comments Download

Messages

Total messages: 33 (18 generated)
Felix Ekblom
3 years, 7 months ago (2017-05-08 13:50:35 UTC) #4
jdufault
https://codereview.chromium.org/2865003003/diff/1/chrome/browser/chromeos/login/ui/login_display_host_impl.cc File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/chromeos/login/ui/login_display_host_impl.cc#newcode979 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/chromeos/login/oobe_display_chooser.cc File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc#newcode41 ...
3 years, 7 months ago (2017-05-08 17:44:58 UTC) #6
oshima
what is expected behavior if a user connected keyboard (and mouse) after it is booted? ...
3 years, 7 months ago (2017-05-09 05:58:45 UTC) #7
Felix Ekblom
https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc#newcode37 chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:37: screen_(screen) {} On 2017/05/09 05:58:45, oshima wrote: > Just ...
3 years, 7 months ago (2017-05-09 08:54:50 UTC) #8
Felix Ekblom
On 2017/05/09 05:58:45, oshima wrote: > what is expected behavior if a user connected keyboard ...
3 years, 7 months ago (2017-05-09 08:58:11 UTC) #9
jdufault
lgtm https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2865003003/diff/20001/chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc#newcode37 chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:37: auto& displays = nit: auto& type is not ...
3 years, 7 months ago (2017-05-09 18:59:38 UTC) #10
oshima
lgtm this CL once you address the following You may want to support keyboard removal ...
3 years, 7 months ago (2017-05-10 00:14:31 UTC) #11
Alexander Alekseev
lgtm with oshimas comments addressed.
3 years, 7 months ago (2017-05-10 00:41:55 UTC) #12
Felix Ekblom
On 2017/05/10 00:14:31, oshima wrote: > > You may want to support keyboard removal scenario, ...
3 years, 7 months ago (2017-05-10 09:54:51 UTC) #13
Felix Ekblom
https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chromeos/login/oobe_ui.h File chrome/browser/ui/webui/chromeos/login/oobe_ui.h (right): https://codereview.chromium.org/2865003003/diff/1/chrome/browser/ui/webui/chromeos/login/oobe_ui.h#newcode174 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: ...
3 years, 7 months ago (2017-05-10 10:59:13 UTC) #14
Felix Ekblom
Patch set 6 add a guard to prevent OobeDisplayChooser creation in mus+ash / MASH mode ...
3 years, 7 months ago (2017-05-10 19:57:07 UTC) #19
jdufault
https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webui/chromeos/login/oobe_ui.cc File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webui/chromeos/login/oobe_ui.cc#newcode358 chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:358: if (!ash_util::IsRunningInMash() && !IsKeyboardConnected()) File a bug, add TODO
3 years, 7 months ago (2017-05-10 20:29:02 UTC) #20
jdufault
On 2017/05/10 20:29:02, jdufault wrote: > https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webui/chromeos/login/oobe_ui.cc > File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): > > https://codereview.chromium.org/2865003003/diff/100001/chrome/browser/ui/webui/chromeos/login/oobe_ui.cc#newcode358 > ...
3 years, 7 months ago (2017-05-10 21:37:10 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2865003003/120001
3 years, 7 months ago (2017-05-11 08:28:46 UTC) #30
commit-bot: I haz the power
3 years, 7 months ago (2017-05-11 10:54:09 UTC) #33
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bcbc0c40b5987b07425a9bad2f9d...

Powered by Google App Engine
This is Rietveld 408576698