HID fix for device info check.

HID fix for device info check.

BUG=436912
Committed: https://crrev.com/ce6a9e9d824549e476644cf611fea6a9b44750ba
Cr-Commit-Position: refs/heads/master@{#315295}

[merkulova, 2014-12-04 11:43:50]

merkulova@chromium.org changed reviewers:
+ ygorshenin@chromium.org

[merkulova, 2014-12-11 11:07:14]

ping :)

[ygorshenin1, 2014-12-24 09:34:47]

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(left):

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:146: /**
Why this code is removed?

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(right):

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:84: if
($('hid-mouse-block').classList.contains('connected') ||
I thought it would be better to rewrite predicate like this:
if ((blockId == 'hid-mouse-block' || blockId == 'hid-keyboard-block') &&
    (state == 'connected' || state == 'paired')) {
  ...
} else {
  ...
}

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:84: if
($('hid-mouse-block').classList.contains('connected') ||
Also, it would be nice to declare a dictionary for device block state and for
possible values of blockId.

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:65:
return info.is_keyboard || info.is_touchscreen || info.is_tablet;
I disagree about touchscreen, because presence of touchscreen doesn't mean
presence of (virtual) keyboard.

[merkulova, 2014-12-24 13:47:46]

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(left):

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:146: /**
On 2014/12/24 09:34:47, ygorshenin1 wrote:
> Why this code is removed?

1. Its logic is wrong: if list of connected devices is already processed
(initial check) and for example keyboard was found and mouse wasn't, this might
lead to setting 'searching' state to an already connected device. And as far as
block for connected device isn't updated unless device disconnects, we get wrong
UI.
2. Initial state ('searching' or 'connected') is set via screen handler. At Show
method.

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(right):

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:84: if
($('hid-mouse-block').classList.contains('connected') ||
On 2014/12/24 09:34:47, ygorshenin1 wrote:
> I thought it would be better to rewrite predicate like this:
> if ((blockId == 'hid-mouse-block' || blockId == 'hid-keyboard-block') &&
>     (state == 'connected' || state == 'paired')) {
>   ...
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:84: if
($('hid-mouse-block').classList.contains('connected') ||
On 2014/12/24 09:34:47, ygorshenin1 wrote:
> Also, it would be nice to declare a dictionary for device block state and for
> possible values of blockId.

Done.

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/770563003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:65:
return info.is_keyboard || info.is_touchscreen || info.is_tablet;
On 2014/12/24 09:34:47, ygorshenin1 wrote:
> I disagree about touchscreen, because presence of touchscreen doesn't mean
> presence of (virtual) keyboard.

Left to be discussed.

[merkulova, 2015-02-05 12:48:01]

merkulova@chromium.org changed reviewers:
+ antrim@chromium.org
- ygorshenin@chromium.org

[merkulova, 2015-02-05 12:48:02]

Changing reviewer.

[Denis Kuznetsov (DE-MUC), 2015-02-05 13:26:17]

https://chromiumcodereview.appspot.com/770563003/diff/40001/chrome/browser/re...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(right):

https://chromiumcodereview.appspot.com/770563003/diff/40001/chrome/browser/re...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:94:
deviceBlock.classList.remove(stateCase);
You can use .toggle(name, value) here.

[Denis Kuznetsov (DE-MUC), 2015-02-09 12:03:59]

lgtm

[merkulova, 2015-02-09 12:31:18]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-09 12:32:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/770563003/60001

[commit-bot: I haz the power, 2015-02-09 14:18:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-09 14:18:43]

Patchset 4 (id:??) landed as
https://crrev.com/ce6a9e9d824549e476644cf611fea6a9b44750ba
Cr-Commit-Position: refs/heads/master@{#315295}