Created fakes for HID-detection screen testing. Initial browsertest added.

Created fakes for HID-detection screen testing. Initial browsertest added.

BUG=456779
TBR=keybuk@chromium.org

Committed: https://crrev.com/4209a162a2a4a9b49c9096909fbfadb7a2608b72
Cr-Commit-Position: refs/heads/master@{#318667}

[merkulova, 2015-02-10 17:06:00]

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

[merkulova, 2015-02-10 17:10:38]

Nikita, please, have a look.

[Nikita (slow), 2015-02-11 14:24:17]

please update bug #

[Nikita (slow), 2015-02-11 14:53:08]

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device/fake_input_service_proxy.h (right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/fake_input_service_proxy.h:16: 
nit: drop empty line

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device/input_service_proxy_factory.cc (right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/input_service_proxy_factory.cc:14: return
static_cast<InputServiceProxy*>(new FakeInputServiceProxy);
drop static_cast<>

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/input_service_proxy_factory.cc:19: bool
InputServiceProxyFactory::in_fake_mode_ = false;
nit: use_fake_instance_

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/input_service_proxy_factory.cc:22: void
InputServiceProxyFactory::SetFakeForTesting(bool to_fake) {
nit: bool use_fake_instance

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/hid_detection_browsertest.cc:63: 
I suggest at least adding one test case that checks situation when there're
devices available.

https://codereview.chromium.org/913773002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h
(right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:194:
InputServiceProxy* input_service_proxy_;
scoped_ptr<>

[merkulova, 2015-02-13 09:59:39]

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

[merkulova, 2015-02-13 10:00:05]

+keybuk@ as */hid/* owner

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device/fake_input_service_proxy.h (right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/fake_input_service_proxy.h:16: 
On 2015/02/11 14:53:07, Nikita wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device/input_service_proxy_factory.cc (right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/input_service_proxy_factory.cc:14: return
static_cast<InputServiceProxy*>(new FakeInputServiceProxy);
On 2015/02/11 14:53:07, Nikita wrote:
> drop static_cast<>

Done.

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/input_service_proxy_factory.cc:19: bool
InputServiceProxyFactory::in_fake_mode_ = false;
On 2015/02/11 14:53:07, Nikita wrote:
> nit: use_fake_instance_

Done.

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device/input_service_proxy_factory.cc:22: void
InputServiceProxyFactory::SetFakeForTesting(bool to_fake) {
On 2015/02/11 14:53:07, Nikita wrote:
> nit: bool use_fake_instance

Done.

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/hid_detection_browsertest.cc:63: 
On 2015/02/11 14:53:07, Nikita wrote:
> I suggest at least adding one test case that checks situation when there're
> devices available.

Done.

https://codereview.chromium.org/913773002/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h
(right):

https://codereview.chromium.org/913773002/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:194:
InputServiceProxy* input_service_proxy_;
On 2015/02/11 14:53:08, Nikita wrote:
> scoped_ptr<>

Done.

[Nikita (slow), 2015-02-13 12:31:45]

lgtm

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
File device/hid/fake_input_service_linux.cc (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
device/hid/fake_input_service_linux.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
File device/hid/fake_input_service_linux.h (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
device/hid/fake_input_service_linux.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
nit: 2015

https://codereview.chromium.org/913773002/diff/20001/device/hid/hid.gyp
File device/hid/hid.gyp (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/hid.gyp#newco...
device/hid/hid.gyp:22: 'fake_input_service_linux.cc',
You'll need to update device/hid/BUILD.gn as well.

https://codereview.chromium.org/913773002/diff/20001/device/hid/input_service...
File device/hid/input_service_linux.h (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/input_service...
device/hid/input_service_linux.h:83: DeviceMap devices_;
nit: add empty line between methods and members.

[merkulova, 2015-02-13 12:57:46]

New patchsets have been uploaded after l-g-t-m from nkostylev@chromium.org

[merkulova, 2015-02-13 12:58:06]

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
File device/hid/fake_input_service_linux.cc (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
device/hid/fake_input_service_linux.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/02/13 12:31:45, Nikita wrote:
> nit: 2015

Done.

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
File device/hid/fake_input_service_linux.h (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/fake_input_se...
device/hid/fake_input_service_linux.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2015/02/13 12:31:45, Nikita wrote:
> nit: 2015

Done.

https://codereview.chromium.org/913773002/diff/20001/device/hid/hid.gyp
File device/hid/hid.gyp (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/hid.gyp#newco...
device/hid/hid.gyp:22: 'fake_input_service_linux.cc',
On 2015/02/13 12:31:45, Nikita wrote:
> You'll need to update device/hid/BUILD.gn as well.

Already done.

https://codereview.chromium.org/913773002/diff/20001/device/hid/input_service...
File device/hid/input_service_linux.h (right):

https://codereview.chromium.org/913773002/diff/20001/device/hid/input_service...
device/hid/input_service_linux.h:83: DeviceMap devices_;
On 2015/02/13 12:31:45, Nikita wrote:
> nit: add empty line between methods and members.

Done.

[merkulova, 2015-02-18 14:15:59]

friendly ping

[merkulova, 2015-02-20 09:15:05]

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

[merkulova, 2015-02-20 09:15:32]

+rockot@ as owner of src/device/

[Ken Rockot(use gerrit already), 2015-02-20 17:15:03]

It would be unfortunate to introduce all these virtual methods into
InputServiceLinux just to get around thread restrictions in tests.

What if you left all the methods as-is (i.e. no overrides), and just moved the
AssertIOAllowed() from the InputServiceLinux constructor into the
InputServiceLinuxImpl constructor?

There should be no problem with all the base methods asserting that they were
called on the same thread as the constructor, right? Then we could avoid all
these virtual functions.

https://codereview.chromium.org/913773002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right):

https://codereview.chromium.org/913773002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/hid_detection_browsertest.cc:46: typedef
device::InputServiceLinux::InputDeviceInfo InputDeviceInfo;
Please use aliases instead of typedef for new code:

using InputDeviceinfo = device::InputServiceLinux::InputDeviceInfo;

https://codereview.chromium.org/913773002/diff/40001/device/hid/input_service...
File device/hid/input_service_linux.h (right):

https://codereview.chromium.org/913773002/diff/40001/device/hid/input_service...
device/hid/input_service_linux.h:17: 
nit: no blank line

https://codereview.chromium.org/913773002/diff/40001/device/hid/input_service...
device/hid/input_service_linux.h:48: typedef base::hash_map<std::string,
InputDeviceInfo> DeviceMap;
nit: alias instead of typedef

[merkulova, 2015-02-24 09:10:16]

Thanks for the advice.
Moved thread checks back. Removed virtual methods.

https://codereview.chromium.org/913773002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right):

https://codereview.chromium.org/913773002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/hid_detection_browsertest.cc:46: typedef
device::InputServiceLinux::InputDeviceInfo InputDeviceInfo;
On 2015/02/20 17:15:03, Ken Rockot wrote:
> Please use aliases instead of typedef for new code:
> 
> using InputDeviceinfo = device::InputServiceLinux::InputDeviceInfo;

Done.

https://codereview.chromium.org/913773002/diff/40001/device/hid/input_service...
File device/hid/input_service_linux.h (right):

https://codereview.chromium.org/913773002/diff/40001/device/hid/input_service...
device/hid/input_service_linux.h:17: 
On 2015/02/20 17:15:03, Ken Rockot wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/913773002/diff/40001/device/hid/input_service...
device/hid/input_service_linux.h:48: typedef base::hash_map<std::string,
InputDeviceInfo> DeviceMap;
On 2015/02/20 17:15:03, Ken Rockot wrote:
> nit: alias instead of typedef

Done.

[Ken Rockot(use gerrit already), 2015-02-24 13:42:50]

lgtm

[merkulova, 2015-02-24 13:46:07]

merkulova@chromium.org changed reviewers:
- keybuk@chromium.org

[merkulova, 2015-02-24 13:46:41]

Moving keybuk@ to TBR as rockot@ made the review.

[merkulova, 2015-02-24 13:46:58]

The CQ bit was checked by merkulova@chromium.org

[merkulova, 2015-02-24 13:46:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/913773002/#ps60001 (title:
"Thread checking moved back to InputServiceLinux.")

[commit-bot: I haz the power, 2015-02-24 13:48:02]

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

[commit-bot: I haz the power, 2015-02-24 17:23:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-24 17:23:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2015-02-24 17:26:49]

https://codereview.chromium.org/913773002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/hid_detection_browsertest.cc (right):

https://codereview.chromium.org/913773002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/hid_detection_browsertest.cc:83:
device::FakeInputServiceLinux input_service_linux_;
Actually I think you might want to make this a scoped_ptr, and initialize it
with a new instance from within SetUpOnMainThread.

[merkulova, 2015-02-25 10:02:21]

I think we'll have to return to the scheme with thread checks only in
InputServiceLinuxImpl class but not in the InputServiceLinux.

SetUpOnMainThread doesn't fit as InputServiceProxy posts tasks for
InputServiceLinux on the File thread. So we should create the InputServiceLinux
on the same file thread.

If we post a task to the file thread from within SetUpOnMainThread, it's
executed too late (at least in test), the test instance of browser manages to
recognize devices of the machine it's running on. And test fails as
configuration is not done on time.

Ken, do you agree with that move-back?

[Ken Rockot(use gerrit already), 2015-02-25 18:57:50]

On 2015/02/25 10:02:21, merkulova wrote:
> I think we'll have to return to the scheme with thread checks only in
> InputServiceLinuxImpl class but not in the InputServiceLinux.
> 
> SetUpOnMainThread doesn't fit as InputServiceProxy posts tasks for
> InputServiceLinux on the File thread. So we should create the
InputServiceLinux
> on the same file thread.
> 
> If we post a task to the file thread from within SetUpOnMainThread, it's
> executed too late (at least in test), the test instance of browser manages to
> recognize devices of the machine it's running on. And test fails as
> configuration is not done on time.
> 
> Ken, do you agree with that move-back?

Nope, it's not OK to move back. After some more thought, I see that the problem
is more than just a matter of style or convention. The tests in their current
form are actually incorrect because they use a non-thread-safe object
(FakeInputServiceLinux) from multiple threads. Moving the checks into the impl
is just a way to hide this fact.

Nothing stops InputServiceProxy from calling InputServiceLinux::GetDevices on
the FILE thread while someone on the main thread is calling AddDeviceForTesting.
This can lead to GetDevices reading the not-thread-safe device map in an
inconsistent state.

Even if these specific tests were carefully written to avoid that scenario,
ignoring this problem is just asking for subtle flakiness bugs later. The
service is not thread-safe, and it should not be accessed from multiple threads.

Could you construct InputServiceProxy over a base::TaskRunner instead of
assuming it should always post to the FILE loop? You would have to add some
testing support to HIDDetectionScreen as well to let tests override how it
constructs/acquires its proxy.

[merkulova, 2015-02-27 08:52:14]

Updated the InputServiceProxy.

[Ken Rockot(use gerrit already), 2015-02-27 16:03:40]

Thank you. I think this should work. LGTM

[merkulova, 2015-03-02 07:55:16]

The CQ bit was checked by merkulova@chromium.org

[merkulova, 2015-03-02 07:55:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/913773002/#ps80001 (title:
"TaskRunner used for InputServiceProxy.")

[commit-bot: I haz the power, 2015-03-02 07:55:44]

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

[commit-bot: I haz the power, 2015-03-02 08:46:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-02 08:46:44]

Patchset 5 (id:??) landed as
https://crrev.com/4209a162a2a4a9b49c9096909fbfadb7a2608b72
Cr-Commit-Position: refs/heads/master@{#318667}