Use fake Input Device Settings with chrome://device-emulator

Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to chrome://device-emulator to
simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a37d858bf932e28599be6bec9bd3f1dbbe9e9874
Cr-Commit-Position: refs/heads/master@{#406390}

[michaelpg, 2016-05-23 23:00:56]

Description was changed from

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to non-official Debug builds for use with
chrome://device-emulator to simulate having attached mouse/touchpad.
==========

to

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to non-official Debug builds for use with
chrome://device-emulator to simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-05-23 23:08:37]

michaelpg@chromium.org changed reviewers:
+ stevenjb@chromium.org

[michaelpg, 2016-05-23 23:08:38]

stevenjb: could you help with this? I'm trying to
add a test file to chromeos on non-official debug builds.
It works in GN but not GYP.

The files to add are:
  browser/chromeos/system/fake_input_device_settings.*

Currently they are included in test_support_common on CrOS
(for use in unit_tests and browser_tests).

I would like them to also be included in
chrome_browser_chromeos, but only on non-official Debug
builds:

* if official OR not debug, include in test_support_common
* if non-official AND debug, include in chromeos (and not
    in test_support_common, because that would result in
    multiple definitions)

The result works in GN, but in GYP I get:

> gyp: sources not allowed in the Debug configuration,
> found in target
> src/chrome/chrome.gyp:browser_chromeos#target

in both official and non-official builds.

[stevenjb, 2016-05-24 18:40:54]

On 2016/05/23 23:08:38, michaelpg wrote:
> stevenjb: could you help with this? I'm trying to
> add a test file to chromeos on non-official debug builds.
> It works in GN but not GYP.
> 
> The files to add are:
>   browser/chromeos/system/fake_input_device_settings.*
> 
> Currently they are included in test_support_common on CrOS
> (for use in unit_tests and browser_tests).
> 
> I would like them to also be included in
> chrome_browser_chromeos, but only on non-official Debug
> builds:
> 
> * if official OR not debug, include in test_support_common
> * if non-official AND debug, include in chromeos (and not
>     in test_support_common, because that would result in
>     multiple definitions)
> 
> The result works in GN, but in GYP I get:
> 
> > gyp: sources not allowed in the Debug configuration,
> > found in target
> > src/chrome/chrome.gyp:browser_chromeos#target
> 
> in both official and non-official builds.

Do we even need to support "official Debug" builds?

My GYP fu is pretty weak, but added some comments that may or may not help.

[stevenjb, 2016-05-24 20:57:25]

https://codereview.chromium.org/2006083002/diff/1/chrome/chrome_browser_chrom...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/2006083002/diff/1/chrome/chrome_browser_chrom...
chrome/chrome_browser_chromeos.gypi:1341: ['buildtype!="Official"', {
Do we even support official Debug builds?

https://codereview.chromium.org/2006083002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/2006083002/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:2969: ],
Isn't this redundant with line 2974?

[michaelpg, 2016-06-03 16:18:34]

https://codereview.chromium.org/2006083002/diff/1/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/2006083002/diff/1/chrome/chrome_tests_unit.gy...
chrome/chrome_tests_unit.gypi:2006: 'sources': [
With 'sources' inside a 'variables' dictionary, GYP doesn't fail, but the
sources aren't built into the resulting library (so it seems the sources
variable is ignored here rather than propagated).

[stevenjb, 2016-06-08 01:03:08]

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc
(right):

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:226:
power_observer_.reset(new PowerObserver(this));
These are also set in OnJavascriptAllowed(), is there a reason we need to do
this twice?

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:363:
CreateFakeInputDeviceSettingsIfNecessary()->SetMouseExists(has_mouse);
Typically we would just call this GetFakeInputDeviceSettings()

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h
(right):

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:49:
#endif
Conditional inheritance like this can be a pain to maintain. Can we implement a
private observer class as a member instead, e.g.
DeviceEmulatorMessageHandle::PointerDeviceObserver?

[michaelpg, 2016-06-08 02:21:01]

Description was changed from

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to non-official Debug builds for use with
chrome://device-emulator to simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
** Closing while prioritizing desktop work. **

Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to non-official Debug builds for use with
chrome://device-emulator to simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-24 18:20:07]

Patchset #3 (id:40001) has been deleted

[michaelpg, 2016-06-29 00:59:54]

Description was changed from

==========
** Closing while prioritizing desktop work. **

Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to non-official Debug builds for use with
chrome://device-emulator to simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to chrome://device-emulator to
simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-29 15:45:07]

PTAL
thakis: just chrome/browser/BUILD.gn

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc
(right):

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:226:
power_observer_.reset(new PowerObserver(this));
On 2016/06/08 01:03:08, stevenjb wrote:
> These are also set in OnJavascriptAllowed(), is there a reason we need to do
> this twice?

Done.

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:363:
CreateFakeInputDeviceSettingsIfNecessary()->SetMouseExists(has_mouse);
On 2016/06/08 01:03:08, stevenjb wrote:
> Typically we would just call this GetFakeInputDeviceSettings()

revamped as GetForTesting()

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h
(right):

https://codereview.chromium.org/2006083002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:49:
#endif
On 2016/06/08 01:03:08, stevenjb wrote:
> Conditional inheritance like this can be a pain to maintain. Can we implement
a
> private observer class as a member instead, e.g.
> DeviceEmulatorMessageHandle::PointerDeviceObserver?

ifdefs removed!

[michaelpg, 2016-06-29 15:45:17]

michaelpg@chromium.org changed reviewers:
+ thakis@chromium.org

[michaelpg, 2016-06-29 15:45:18]

PTAL
thakis: just chrome/browser/BUILD.gn

[stevenjb, 2016-06-30 00:17:30]

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/fake_input_device_settings.cc (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/fake_input_device_settings.cc:11: :
mouse_exists_(true), touchpad_exists_(true) {}
nit: initialize in header

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/fake_input_device_settings.h (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/fake_input_device_settings.h:37: void
SetTouchpadExists(bool exists);
Inline these and name them set_mouse_exists, etc.

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings.h (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings.h:165: static
FakeInputDeviceSettings* GetForTesting();
See my comment in the other CL, this should be merged into Get(). Let me know if
that is more problematic than I imagine.

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings_impl_ozone.cc (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings_impl_ozone.cc:187: }
So all of this logic should just be in input_device_settings.cc (which means it
has to include the ozone or x11 version based on an #ifdef, but I still think
that's better, and it's consistent with what we do elsewhere), except
InputDeviceSettingsOzone::Get().

[Nico, 2016-06-30 17:25:51]

https://codereview.chromium.org/2006083002/diff/80001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:1026:
'browser/chromeos/system/fake_input_device_settings.h',
we usually consider files called fake_foo to be test files and try to not
include them in the shipping binary. maybe this should be renamed to
simulated_input_device_settings now?

[stevenjb, 2016-07-01 18:53:49]

https://codereview.chromium.org/2006083002/diff/80001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:1026:
'browser/chromeos/system/fake_input_device_settings.h',
On 2016/06/30 17:25:51, Nico wrote:
> we usually consider files called fake_foo to be test files and try to not
> include them in the shipping binary. maybe this should be renamed to
> simulated_input_device_settings now?

I don't know about "usually" but for chrome os we set a precedent for "Fake" a
while back for the implementations that run on linux. (Networking, 

Originally it was "Stub" but as the stubs gained more functionality people
didn't like that name so we named them all Fake. "Simulated" is arguably more
descriptive, but "Fake" is consistent with the existing code.

[michaelpg, 2016-07-01 22:01:48]

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/fake_input_device_settings.cc (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/fake_input_device_settings.cc:11: :
mouse_exists_(true), touchpad_exists_(true) {}
On 2016/06/30 00:17:30, stevenjb wrote:
> nit: initialize in header

Done.

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/fake_input_device_settings.h (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/fake_input_device_settings.h:37: void
SetTouchpadExists(bool exists);
On 2016/06/30 00:17:30, stevenjb wrote:
> Inline these and name them set_mouse_exists, etc.

Done.

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings.h (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings.h:165: static
FakeInputDeviceSettings* GetForTesting();
On 2016/06/30 00:17:30, stevenjb wrote:
> See my comment in the other CL, this should be merged into Get(). Let me know
if
> that is more problematic than I imagine.

I would have to git try that to make sure it doesn't break anything else in
CrOS.

But the reason I made GetForTesting() was to use the setters (eg
set_mouse_exists)
which are specific to FakeInputDeviceSettings.

I could create a new FakeInputDeviceSettings and use SetSettingsForTesting in
device_emulator_message_handler.cc, but then opening chrome://device-emulator
would reset those settings, which doesn't make sense.

[stevenjb, 2016-07-02 00:28:26]

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings.h (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings.h:165: static
FakeInputDeviceSettings* GetForTesting();
On 2016/07/01 22:01:48, michaelpg wrote:
> On 2016/06/30 00:17:30, stevenjb wrote:
> > See my comment in the other CL, this should be merged into Get(). Let me
know
> if
> > that is more problematic than I imagine.
> 
> I would have to git try that to make sure it doesn't break anything else in
> CrOS.
> 
> But the reason I made GetForTesting() was to use the setters (eg
> set_mouse_exists)
> which are specific to FakeInputDeviceSettings.
> 
> I could create a new FakeInputDeviceSettings and use SetSettingsForTesting in
> device_emulator_message_handler.cc, but then opening chrome://device-emulator
> would reset those settings, which doesn't make sense.

I agree we shouldn't create a new / additional FakeInputDeviceSettings. That
should be transparent.

Take a look at FakeShillServiceClient::GetTestInterface. This is a model we have
used to allow setting of fake properties without having to expose the details of
the fake class to callers. The fake implementation will return a valid fake
interface and the real implementation will return a null interface.

[michaelpg, 2016-07-07 22:16:54]

PTAL; the important changes in the latest patchsets are in
	chrome/browser/chromeos/system.

The rest of the churn is mostly rebasing and some rearranging
for consistency.

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings.h (right):

https://codereview.chromium.org/2006083002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings.h:165: static
FakeInputDeviceSettings* GetForTesting();
On 2016/07/02 00:28:26, stevenjb wrote:
> On 2016/07/01 22:01:48, michaelpg wrote:
> > On 2016/06/30 00:17:30, stevenjb wrote:
> > > See my comment in the other CL, this should be merged into Get(). Let me
> know
> > if
> > > that is more problematic than I imagine.
> > 
> > I would have to git try that to make sure it doesn't break anything else in
> > CrOS.
> > 
> > But the reason I made GetForTesting() was to use the setters (eg
> > set_mouse_exists)
> > which are specific to FakeInputDeviceSettings.
> > 
> > I could create a new FakeInputDeviceSettings and use SetSettingsForTesting
in
> > device_emulator_message_handler.cc, but then opening
chrome://device-emulator
> > would reset those settings, which doesn't make sense.
> 
> I agree we shouldn't create a new / additional FakeInputDeviceSettings. That
> should be transparent.
> 
> Take a look at FakeShillServiceClient::GetTestInterface. This is a model we
have
> used to allow setting of fake properties without having to expose the details
of
> the fake class to callers. The fake implementation will return a valid fake
> interface and the real implementation will return a null interface.

Done, created InputDeviceSettings::FakeInterface. Definitely simpler.

[stevenjb, 2016-07-07 23:02:00]

lgtm

[michaelpg, 2016-07-11 22:50:35]

Nico: ping.

[michaelpg, 2016-07-15 02:32:33]

On 2016/07/01 18:53:49, stevenjb wrote:
>
https://codereview.chromium.org/2006083002/diff/80001/chrome/chrome_browser_c...
> File chrome/chrome_browser_chromeos.gypi (right):
> 
>
https://codereview.chromium.org/2006083002/diff/80001/chrome/chrome_browser_c...
> chrome/chrome_browser_chromeos.gypi:1026:
> 'browser/chromeos/system/fake_input_device_settings.h',
> On 2016/06/30 17:25:51, Nico wrote:
> > we usually consider files called fake_foo to be test files and try to not
> > include them in the shipping binary. maybe this should be renamed to
> > simulated_input_device_settings now?
> 
> I don't know about "usually" but for chrome os we set a precedent for "Fake" a
> while back for the implementations that run on linux. (Networking, 
> 
> Originally it was "Stub" but as the stubs gained more functionality people
> didn't like that name so we named them all Fake. "Simulated" is arguably more
> descriptive, but "Fake" is consistent with the existing code.

thakis: are you okay with this or would you still prefer "simulated"?

[michaelpg, 2016-07-18 18:58:00]

michaelpg@chromium.org changed reviewers:
+ sky@chromium.org
- thakis@chromium.org

[michaelpg, 2016-07-18 18:58:00]

+sky: PTAL at chrome/browser/BUILD.gn
-thakis (vacation)

[michaelpg, 2016-07-18 19:00:21]

michaelpg@chromium.org changed reviewers:
+ dpranke@chromium.org
- sky@chromium.org

[michaelpg, 2016-07-18 19:00:21]

+dpranke: are you a proper owner for chrome/BUILD.gn?
-sky: nevermind, just realized "per-file BUILD.gn=file://build/OWNERS"

[michaelpg, 2016-07-18 19:00:28]

+dpranke: are you a proper owner for chrome/browser/BUILD.gn?
-sky: nevermind, just realized "per-file BUILD.gn=file://build/OWNERS"

[michaelpg, 2016-07-18 19:03:28]

Description was changed from

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to chrome://device-emulator to
simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to chrome://device-emulator to
simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[Dirk Pranke, 2016-07-18 19:32:21]

dpranke@chromium.org changed reviewers:
+ sky@chromium.org

[Dirk Pranke, 2016-07-18 19:32:21]

I assume you're referring to the per-file directive in //OWNERS, and that only
applies to //BUILD.gn (it doesn't recurse into subdirectories).

So, you still need sky's approval.

[michaelpg, 2016-07-18 19:35:29]

michaelpg@chromium.org changed reviewers:
- dpranke@chromium.org

[michaelpg, 2016-07-18 19:35:30]

+sky: chrome/browser/BUILD.gn (for reals this time)
-dpranke

[sky, 2016-07-18 20:11:12]

BUILD.gn LGTM

[michaelpg, 2016-07-18 20:33:09]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-07-18 20:33:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2006083002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-18 20:33:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 20:33:54]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-18 21:52:20]

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

[commit-bot: I haz the power, 2016-07-18 21:52:22]

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

[michaelpg, 2016-07-19 20:32:32]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-07-19 20:32:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2006083002/#ps180001
(title: "make dependent on a11y fix")

[commit-bot: I haz the power, 2016-07-19 20:33:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 20:33:26]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 21:49:57]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-07-19 21:50:07]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 21:51:29]

Description was changed from

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to chrome://device-emulator to
simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use fake Input Device Settings with chrome://device-emulator

Adds FakeInputDeviceSettings to chrome://device-emulator to
simulate having attached mouse/touchpad.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a37d858bf932e28599be6bec9bd3f1dbbe9e9874
Cr-Commit-Position: refs/heads/master@{#406390}
==========

[commit-bot: I haz the power, 2016-07-19 21:51:31]

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