Touch Events capability negotiation

Touch Events capability negotiation

- Send "touchEvents" capability from the host if the host is capable
  of injecting raw touch inputs.
- Client is always capable of "touchEvents".
- Moved capability names to remoting/protocol/capability_names.h.

BUG=314515
Committed: https://crrev.com/a7815e75ec645c78465c5dc32c5b0ebfb4372c0c
Cr-Commit-Position: refs/heads/master@{#333139}

[Rintaro Kuroiwa, 2015-03-26 22:29:09]

rkuroiwa@chromium.org changed reviewers:
+ wez@chromium.org

[Rintaro Kuroiwa, 2015-03-26 22:29:10]

PTAL

[Wez, 2015-03-30 17:10:07]

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:183: "desktopShape multiTouch";
Do you mean to add multiTouch capability here? You've already got the web-app
handling the capability negotiation.

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/ipc_deskt...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/ipc_deskt...
remoting/host/ipc_desktop_environment_unittest.cc:463:
TEST_F(IpcDesktopEnvironmentTest, Capabilities) {
CapabilitiesNoTouch?

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
File remoting/host/multi_touch_capability.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
remoting/host/multi_touch_capability.cc:17: return
TouchInjectorWinDelegate::Create().get() != nullptr;
nit: You can test a scoped_ptr<> directly as a bool, so it'd be clearer to
assign this to a local scoped_ptr<> |foo| and then return |foo|, I think.

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
File remoting/host/multi_touch_capability.h (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
remoting/host/multi_touch_capability.h:12: bool MultiTouchCapable();
Do we need a whole header for this little constant and this method?

e.g. Should this be a property of the InputStub interface, indicating whether
the InjectTouchInput() call works?  Or a (static?) method of InputInjector?

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:822:
desktop_environment_factory_->SetEnableMultiTouch(MultiTouchCapable());
Do you need to have the Me2Me host do this? You're just enabling/disabling based
on whether the implementation supports touch - you can do that my exposing that
information on the InputStub or InputInjector interfaces, and letting the
DesktopEnvironment or ClientSession then use that to construct the capabilities
it reports.

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_plugin.js:104: * Notifies the plugin that touch
events should be handled by the plugin.
nit: No need for two mentions of "plugin". ;) e.g. "Requests that the plugin
handle touch input natively."

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/client_plugin_impl.js (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_plugin_impl.js:644: JSON.stringify({method:
'enableTouchEvents', data: {}}));
This makes enabling touch events a one-way function - once disabled you can
never disable them.

If you allow a bool enable/disable flag then we'd have the option of switching
touch events on or off based on user preferences, UX state, etc mid-session.

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_session.js:214: // mouse events from PPAPI.
Suggest rewording:

"Host & client support touch. If the host does not support touch then the client
will let PPAPI translate touch input into mouse events, for compatibility with
non-touch-aware UI."

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_session.js:475: 
nit: No need for blank line.

[Rintaro Kuroiwa, 2015-04-08 03:27:43]

PTAL

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:183: "desktopShape multiTouch";
On 2015/03/30 17:10:07, Wez wrote:
> Do you mean to add multiTouch capability here? You've already got the web-app
> handling the capability negotiation.

I'm not very good at javascript :(
I found out that I could put it in webapp/base/js/application.js or just put it
here. I'll put it in application.js, where all the other app capabilities are
set.

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/ipc_deskt...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/ipc_deskt...
remoting/host/ipc_desktop_environment_unittest.cc:463:
TEST_F(IpcDesktopEnvironmentTest, Capabilities) {
On 2015/03/30 17:10:07, Wez wrote:
> CapabilitiesNoTouch?

Renamed to NoTouchCapabilities and moved these tests to
client_session_unittest.cc

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
File remoting/host/multi_touch_capability.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
remoting/host/multi_touch_capability.cc:17: return
TouchInjectorWinDelegate::Create().get() != nullptr;
On 2015/03/30 17:10:07, Wez wrote:
> nit: You can test a scoped_ptr<> directly as a bool, so it'd be clearer to
> assign this to a local scoped_ptr<> |foo| and then return |foo|, I think.

Done. Also moved this to InputInjector<platform>::CanInjectTouch().

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
File remoting/host/multi_touch_capability.h (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/multi_tou...
remoting/host/multi_touch_capability.h:12: bool MultiTouchCapable();
On 2015/03/30 17:10:07, Wez wrote:
> Do we need a whole header for this little constant and this method?
> 
> e.g. Should this be a property of the InputStub interface, indicating whether
> the InjectTouchInput() call works?  Or a (static?) method of InputInjector?

I couldn't think of a good place but static method for InputInjector sounds good
to me. Then I don't need the ifdef too :)
Removed this file.

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:822:
desktop_environment_factory_->SetEnableMultiTouch(MultiTouchCapable());
On 2015/03/30 17:10:07, Wez wrote:
> Do you need to have the Me2Me host do this? You're just enabling/disabling
based
> on whether the implementation supports touch - you can do that my exposing
that
> information on the InputStub or InputInjector interfaces, and letting the
> DesktopEnvironment or ClientSession then use that to construct the
capabilities
> it reports.

All I really wanted to do is unit test that "multiTouch" is added to the host
capabilities. Adding a (non-static) CanInjectTouch() method to InputInjector, I
should be able to do this in client_session_unittest.cc :)

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_plugin.js:104: * Notifies the plugin that touch
events should be handled by the plugin.
On 2015/03/30 17:10:07, Wez wrote:
> nit: No need for two mentions of "plugin". ;) e.g. "Requests that the plugin
> handle touch input natively."

Done.

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/client_plugin_impl.js (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_plugin_impl.js:644: JSON.stringify({method:
'enableTouchEvents', data: {}}));
On 2015/03/30 17:10:07, Wez wrote:
> This makes enabling touch events a one-way function - once disabled you can
> never disable them.
> 
> If you allow a bool enable/disable flag then we'd have the option of switching
> touch events on or off based on user preferences, UX state, etc mid-session.

Done.

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_session.js:214: // mouse events from PPAPI.
On 2015/03/30 17:10:07, Wez wrote:
> Suggest rewording:
> 
> "Host & client support touch. If the host does not support touch then the
client
> will let PPAPI translate touch input into mouse events, for compatibility with
> non-touch-aware UI."

Done.

https://codereview.chromium.org/1033913003/diff/60001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/client_session.js:475: 
On 2015/03/30 17:10:07, Wez wrote:
> nit: No need for blank line.

Done.

[Wez, 2015-04-09 22:59:00]

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:183: "desktopShape multiTouch";
On 2015/04/08 03:27:42, Rintaro Kuroiwa wrote:
> On 2015/03/30 17:10:07, Wez wrote:
> > Do you mean to add multiTouch capability here? You've already got the
web-app
> > handling the capability negotiation.
> 
> I'm not very good at javascript :(
> I found out that I could put it in webapp/base/js/application.js or just put
it
> here. I'll put it in application.js, where all the other app capabilities are
> set.

My point is more that there are two sets of capability negotiations:
1. client <-> host (i.e. protocol level)
2. web-app <-> plugin (we needed this because the plugin used to be in-built to
Chrome).

If you add a capability here then it's #2, whereas I think you want #1?

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/client_s...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/client_s...
remoting/host/client_session_unittest.cc:776: // Check multiTouch in
capabilities is set when InputInjector can inject touch
nit: "Check multiTouch capability is set when..."

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/host_moc...
File remoting/host/host_mock_objects.h (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/host_moc...
remoting/host/host_mock_objects.h:120: MOCK_METHOD0(CanInjectTouch, bool());
Feels like this belongs immediately after the StartPtr mock method; we tend to
keep the mocks in one block and the delegates for special-cases like StartPtr in
a separate block (see above).

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
File remoting/host/input_injector.h (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
remoting/host/input_injector.h:35: virtual bool CanInjectTouch() = 0;
Add a comment e.g. "Returns true iff the InputInjector implements
InjectTouchPoints()."

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
remoting/host/input_injector.h:39: static bool DefaultCanInjectTouch();
If "most" implementations will call this then why not have ConInjectTouch() have
a default impl that calls it?

What does it actually do, though? Does it return true e.g. iff there is a
platform-specific input injection implementation?

Looking at the InputInjector impls, there doesn't seem to be any value to this
static, since the impls all just call through to the "default" impl implemented
in the same code file? Why not put the implementations in-line?

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
File remoting/host/input_injector_win.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
remoting/host/input_injector_win.cc:76: virtual bool CanInjectTouch() override;
This file will need the virtuals stripping out of it, otherwise I think it'll
fail compile checks; we should have only virtual or override, not both.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/ipc_inpu...
File remoting/host/ipc_input_injector.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/ipc_inpu...
remoting/host/ipc_input_injector.cc:46: return
InputInjector::DefaultCanInjectTouch();
Can you extend the DesktopSessionProxy to get this information from the
DesktopSession when it starts up...?

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/single_w...
File remoting/host/single_window_input_injector_mac.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/single_w...
remoting/host/single_window_input_injector_mac.cc:123: return
InputInjector::DefaultCanInjectTouch();
This is wrong; InjectTouchEvent() above is NOTIMPLEMENTED().

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/win/sess...
File remoting/host/win/session_input_injector.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/win/sess...
remoting/host/win/session_input_injector.cc:205: return
nested_executor_->CanInjectTouch();
You're accessing the |nested_executor_| from the caller thread, rather than from
the |input_task_runner_| thread.

Since we never expect the result to change at run-time, I'd suggest instead
calling CanInjectTouch() on the |input_task_runner_| and posting the result back
to the SessionInputInjectorWin (NOT the core) on the caller thread, to cache to
return from its own CanInjectTouch().

https://codereview.chromium.org/1033913003/diff/140001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin.js:104: * Requests that the plugin handle
touch input natively.
Didn't we say we were adding an |enable| parameter, so this can both enable and
disable touch?

[Rintaro Kuroiwa, 2015-04-20 18:08:57]

Patchset #9 (id:160001) has been deleted

[Rintaro Kuroiwa, 2015-04-20 18:20:03]

As discussed offline, w/o sync IPC, it might not make sense to put
CanInjectTouch() on InputStub. So I reverted to patchset 4. So it probably makes
sense to look at diff between patchsets 4 and 9.

I tried to make patch 9 for reviews from 4 and 8, sorry if I missed anything
from 4.

PTAL.

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/1033913003/diff/60001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:183: "desktopShape multiTouch";
On 2015/04/09 22:58:59, Wez wrote:
> On 2015/04/08 03:27:42, Rintaro Kuroiwa wrote:
> > On 2015/03/30 17:10:07, Wez wrote:
> > > Do you mean to add multiTouch capability here? You've already got the
> web-app
> > > handling the capability negotiation.
> > 
> > I'm not very good at javascript :(
> > I found out that I could put it in webapp/base/js/application.js or just put
> it
> > here. I'll put it in application.js, where all the other app capabilities
are
> > set.
> 
> My point is more that there are two sets of capability negotiations:
> 1. client <-> host (i.e. protocol level)
> 2. web-app <-> plugin (we needed this because the plugin used to be in-built
to
> Chrome).
> 
> If you add a capability here then it's #2, whereas I think you want #1?

Oh I see. Yes I meant to do #1.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/client_s...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/client_s...
remoting/host/client_session_unittest.cc:776: // Check multiTouch in
capabilities is set when InputInjector can inject touch
On 2015/04/09 22:58:59, Wez wrote:
> nit: "Check multiTouch capability is set when..."

Done.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/host_moc...
File remoting/host/host_mock_objects.h (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/host_moc...
remoting/host/host_mock_objects.h:120: MOCK_METHOD0(CanInjectTouch, bool());
On 2015/04/09 22:58:59, Wez wrote:
> Feels like this belongs immediately after the StartPtr mock method; we tend to
> keep the mocks in one block and the delegates for special-cases like StartPtr
in
> a separate block (see above).

N/A

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
File remoting/host/input_injector.h (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
remoting/host/input_injector.h:35: virtual bool CanInjectTouch() = 0;
On 2015/04/09 22:58:59, Wez wrote:
> Add a comment e.g. "Returns true iff the InputInjector implements
> InjectTouchPoints()."

Done.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
remoting/host/input_injector.h:39: static bool DefaultCanInjectTouch();
On 2015/04/09 22:58:59, Wez wrote:
> If "most" implementations will call this then why not have ConInjectTouch()
have
> a default impl that calls it?
> 
> What does it actually do, though? Does it return true e.g. iff there is a
> platform-specific input injection implementation?
> 
> Looking at the InputInjector impls, there doesn't seem to be any value to this
> static, since the impls all just call through to the "default" impl
implemented
> in the same code file? Why not put the implementations in-line?

Removed.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
File remoting/host/input_injector_win.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/input_in...
remoting/host/input_injector_win.cc:76: virtual bool CanInjectTouch() override;
On 2015/04/09 22:58:59, Wez wrote:
> This file will need the virtuals stripping out of it, otherwise I think it'll
> fail compile checks; we should have only virtual or override, not both.
Done.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/ipc_inpu...
File remoting/host/ipc_input_injector.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/ipc_inpu...
remoting/host/ipc_input_injector.cc:46: return
InputInjector::DefaultCanInjectTouch();
On 2015/04/09 22:58:59, Wez wrote:
> Can you extend the DesktopSessionProxy to get this information from the
> DesktopSession when it starts up...?

As discussed offline, we need a synchronous IPC to do this.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/single_w...
File remoting/host/single_window_input_injector_mac.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/single_w...
remoting/host/single_window_input_injector_mac.cc:123: return
InputInjector::DefaultCanInjectTouch();
On 2015/04/09 22:58:59, Wez wrote:
> This is wrong; InjectTouchEvent() above is NOTIMPLEMENTED().

Reverted.

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/win/sess...
File remoting/host/win/session_input_injector.cc (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/host/win/sess...
remoting/host/win/session_input_injector.cc:205: return
nested_executor_->CanInjectTouch();
On 2015/04/09 22:58:59, Wez wrote:
> You're accessing the |nested_executor_| from the caller thread, rather than
from
> the |input_task_runner_| thread.
> 
> Since we never expect the result to change at run-time, I'd suggest instead
> calling CanInjectTouch() on the |input_task_runner_| and posting the result
back
> to the SessionInputInjectorWin (NOT the core) on the caller thread, to cache
to
> return from its own CanInjectTouch().

N/A for latest patch.

https://codereview.chromium.org/1033913003/diff/140001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/1033913003/diff/140001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin.js:104: * Requests that the plugin handle
touch input natively.
On 2015/04/09 22:59:00, Wez wrote:
> Didn't we say we were adding an |enable| parameter, so this can both enable
and
> disable touch?

Oh I see. I thought you meant we could disable it by passing a flag to the
plugin, and the webapp layer could have something like disableTouchEvents() to
disable it.

Added a flag.

[Wez, 2015-04-21 02:12:12]

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:44: void SetEnableMultiTouch(bool
enable);
Why is this called multi-touch here and touch-events elsewhere?

Since this is a simple setter it should be in-lined and named
set_enable_multi_touch().

Does the setter really need to be public?

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/desktop_...
File remoting/host/desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/desktop_...
remoting/host/desktop_environment.h:87: virtual void SetEnableMultiTouch(bool
enable) {}
See comment re naming of this feature.

IMO this setter doesn't make sense as part of the interface, though - it's
really an implementation feature, surely, i.e. should be part of the
BasicDesktopEnvironmentFactory, etc implementation's interfaces?

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/input_in...
File remoting/host/input_injector.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/input_in...
remoting/host/input_injector.h:32: // InputInjector implements
InjectTouchEvent().
Suggest "Returns true if the InputInjector returned by Create() supports
InjectTouchEvent() on this platform."

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/input_in...
remoting/host/input_injector.h:33: static bool CanInjectTouch();
nit: We typically name these in the style SupportsTouchEvents() or
SupportsInjectTouchEvent().

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.h:36: class IpcDesktopEnvironment : public
DesktopEnvironment {
Blech. Not for this CL, but this class declaration should live in the cc, not
the header.

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.cc:74: InputInjector::CanInjectTouch());
I thought we were holding off on enabling this for IT2Me, initially?

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.cc:92:
desktop_environment->SetEnableMultiTouch(multi_touch_enabled());
As elsewhere, didn't think we were enabling touch for IT2Me yet?

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:29: const char
kMultiTouchCapability[] = "multiTouch";
We're calling this touchEvents elsewhere, e.g. the stub API is InjectTouchEvent
- why not call the capability touchEvents as well?

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:47: result = result + " "  +
kMultiTouchCapability;
Since you're only adding this in Me2MeDesktopEnvironment, rather than in
BasicDesktopEnvironment, you won't get it set e.g. in the
IT2MeDesktopEnvironment, even though you're still telling it that it supports
multi-touch.

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin.js:104: * Notifies the plugin that touch
events should be handled by the plugin.
Suggest "Notifies the plugin whether send touch events to the host." and "True
if touch events should be sent."

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin.js:107: * @return {void} Nothing.
nit: No need for Nothing, AFAIK

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_plugin_impl.js (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin_impl.js:638: * Notifies the plugin that
touch events should be handled by the plugin.
See comments re wording on the interface,.

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_session.js:212: // compatibility with
non-touch-aware UI.
Suggest just "Indicates native touch input support. If the host does not support
touch then the client will let Chrome sythesize mouse events from touch input,
for compatibility with non-touch-aware systems."

[Rintaro Kuroiwa, 2015-04-22 19:56:00]

PTAL

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:44: void SetEnableMultiTouch(bool
enable);
On 2015/04/21 02:12:12, Wez wrote:
> Why is this called multi-touch here and touch-events elsewhere?
> 
> Since this is a simple setter it should be in-lined and named
> set_enable_multi_touch().
> 
> Does the setter really need to be public?

Since I'm changing the capabilities string to rawTouchEvent (see other comment),
I'll make this set_supports_raw_touch_event().
It still doesn't quite align with InjectTouchEvent(). Maybe I should rename it
:/

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/desktop_...
File remoting/host/desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/desktop_...
remoting/host/desktop_environment.h:87: virtual void SetEnableMultiTouch(bool
enable) {}
On 2015/04/21 02:12:12, Wez wrote:
> See comment re naming of this feature.
> 
> IMO this setter doesn't make sense as part of the interface, though - it's
> really an implementation feature, surely, i.e. should be part of the
> BasicDesktopEnvironmentFactory, etc implementation's interfaces?

Removed.

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/input_in...
File remoting/host/input_injector.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/input_in...
remoting/host/input_injector.h:32: // InputInjector implements
InjectTouchEvent().
On 2015/04/21 02:12:12, Wez wrote:
> Suggest "Returns true if the InputInjector returned by Create() supports
> InjectTouchEvent() on this platform."

Done.

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/input_in...
remoting/host/input_injector.h:33: static bool CanInjectTouch();
On 2015/04/21 02:12:12, Wez wrote:
> nit: We typically name these in the style SupportsTouchEvents() or
> SupportsInjectTouchEvent().

Will go with SupportsRawTouchEvents().

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.h:36: class IpcDesktopEnvironment : public
DesktopEnvironment {
On 2015/04/21 02:12:12, Wez wrote:
> Blech. Not for this CL, but this class declaration should live in the cc, not
> the header.

Ack. Will do a follow up patch.

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.cc:74: InputInjector::CanInjectTouch());
On 2015/04/21 02:12:12, Wez wrote:
> I thought we were holding off on enabling this for IT2Me, initially?

Removed.

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.cc:92:
desktop_environment->SetEnableMultiTouch(multi_touch_enabled());
On 2015/04/21 02:12:12, Wez wrote:
> As elsewhere, didn't think we were enabling touch for IT2Me yet?

Removed

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:29: const char
kMultiTouchCapability[] = "multiTouch";
On 2015/04/21 02:12:12, Wez wrote:
> We're calling this touchEvents elsewhere, e.g. the stub API is
InjectTouchEvent
> - why not call the capability touchEvents as well?

I remember some discussion in the design doc. We didn't want something too
generic that means 'touch' so that if we want 'touchGestures' then it's not
ambiguous what 'touchEvents' mean. WDYT about rawTouchEvent?

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:47: result = result + " "  +
kMultiTouchCapability;
On 2015/04/21 02:12:12, Wez wrote:
> Since you're only adding this in Me2MeDesktopEnvironment, rather than in
> BasicDesktopEnvironment, you won't get it set e.g. in the
> IT2MeDesktopEnvironment, even though you're still telling it that it supports
> multi-touch.

BasicDE does implement GetCapabilities and checks the flag to generate the
capabilities string :)
(And IT2MeDE inherits BasicDesktopEnvironment and IT2MeDE doesn't override the
method).

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_plugin.js (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin.js:104: * Notifies the plugin that touch
events should be handled by the plugin.
On 2015/04/21 02:12:12, Wez wrote:
> Suggest "Notifies the plugin whether send touch events to the host." and "True
> if touch events should be sent."

Done.

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin.js:107: * @return {void} Nothing.
On 2015/04/21 02:12:12, Wez wrote:
> nit: No need for Nothing, AFAIK

Done.

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_plugin_impl.js (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_plugin_impl.js:638: * Notifies the plugin that
touch events should be handled by the plugin.
On 2015/04/21 02:12:12, Wez wrote:
> See comments re wording on the interface,.

Done.

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_session.js:212: // compatibility with
non-touch-aware UI.
On 2015/04/21 02:12:12, Wez wrote:
> Suggest just "Indicates native touch input support. If the host does not
support
> touch then the client will let Chrome sythesize mouse events from touch input,
> for compatibility with non-touch-aware systems."

Done.

[Wez, 2015-04-23 22:10:25]

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:29: const char
kMultiTouchCapability[] = "multiTouch";
On 2015/04/22 19:56:00, Rintaro Kuroiwa wrote:
> On 2015/04/21 02:12:12, Wez wrote:
> > We're calling this touchEvents elsewhere, e.g. the stub API is
> InjectTouchEvent
> > - why not call the capability touchEvents as well?
> 
> I remember some discussion in the design doc. We didn't want something too
> generic that means 'touch' so that if we want 'touchGestures' then it's not
> ambiguous what 'touchEvents' mean. WDYT about rawTouchEvent?

I'm inclining back towards just touchEvents; we can call a later higher-level
addition, if any, touchGestures, touchActions, etc.

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:47: result = result + " "  +
kMultiTouchCapability;
On 2015/04/22 19:56:00, Rintaro Kuroiwa wrote:
> On 2015/04/21 02:12:12, Wez wrote:
> > Since you're only adding this in Me2MeDesktopEnvironment, rather than in
> > BasicDesktopEnvironment, you won't get it set e.g. in the
> > IT2MeDesktopEnvironment, even though you're still telling it that it
supports
> > multi-touch.
> 
> BasicDE does implement GetCapabilities and checks the flag to generate the
> capabilities string :)
> (And IT2MeDE inherits BasicDesktopEnvironment and IT2MeDE doesn't override the
> method).

Aha, missed that. I think it'd be cleaner to have this implementation call the
BasicDE one and add kRateLimitResizeRequests to it, rather than hard-coding in
the multi-touch-enabled explicitly. You could even get rid of the
supports_raw_touch_events() getter once you do that.

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.cc:26: const char
kRawTouchEventsCapability[] = "rawTouchEvents";
Why "raw" touch events?

I think it'd be reasonable to call this touchEvents and we can then call a
subsequent addition touchEventsV2, touchGestures, touchActions, etc, as
necessary. :)

nit: Is there really no common location we can move this definition to? :-/

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:126: protected:
No need for another protected: here; the block above is already protected.

[Rintaro Kuroiwa, 2015-04-24 01:55:02]

PTAL

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:47: result = result + " "  +
kMultiTouchCapability;
On 2015/04/23 22:10:25, Wez wrote:
> On 2015/04/22 19:56:00, Rintaro Kuroiwa wrote:
> > On 2015/04/21 02:12:12, Wez wrote:
> > > Since you're only adding this in Me2MeDesktopEnvironment, rather than in
> > > BasicDesktopEnvironment, you won't get it set e.g. in the
> > > IT2MeDesktopEnvironment, even though you're still telling it that it
> supports
> > > multi-touch.
> > 
> > BasicDE does implement GetCapabilities and checks the flag to generate the
> > capabilities string :)
> > (And IT2MeDE inherits BasicDesktopEnvironment and IT2MeDE doesn't override
the
> > method).
> 
> Aha, missed that. I think it'd be cleaner to have this implementation call the
> BasicDE one and add kRateLimitResizeRequests to it, rather than hard-coding in
> the multi-touch-enabled explicitly. You could even get rid of the
> supports_raw_touch_events() getter once you do that.

Done.

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.cc:26: const char
kRawTouchEventsCapability[] = "rawTouchEvents";
On 2015/04/23 22:10:25, Wez wrote:
> Why "raw" touch events?
> 
> I think it'd be reasonable to call this touchEvents and we can then call a
> subsequent addition touchEventsV2, touchGestures, touchActions, etc, as
> necessary. :)
> 
> nit: Is there really no common location we can move this definition to? :-/

Make sense :) will go with touchEvents.

I saw kRateLimitResizeRequests being duplicated so I also duplicated, maybe I'm
wrong because I only see kRateLimitResizeRequests in Me2MeDE and
DesktopSessionProxy (essentially IpcDE) which are both Me2Me. I guess the
translation unit scope prevents from accidentally setting it for It2Me sessions.

Seems overkill to add 'host_capabilities.h' just for this capability. Do you
think input_injector.h would be a reasonable place?

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:126: protected:
On 2015/04/23 22:10:25, Wez wrote:
> No need for another protected: here; the block above is already protected.

Oops. Removed.

[Wez, 2015-04-24 19:56:20]

This is a protocol capability; both client and host have to support it, so
I'd expect it to live e.g. in remoting/protocol/capability_names.h.

Adding sergeyu@ in case he feels differently.

On 23 April 2015 at 18:55, <rkuroiwa@chromium.org> wrote:

> PTAL
>
>
>
>
https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
> File remoting/host/me2me_desktop_environment.cc (right):
>
>
>
https://codereview.chromium.org/1033913003/diff/180001/remoting/host/me2me_de...
> remoting/host/me2me_desktop_environment.cc:47: result = result + " "  +
> kMultiTouchCapability;
> On 2015/04/23 22:10:25, Wez wrote:
>
>> On 2015/04/22 19:56:00, Rintaro Kuroiwa wrote:
>> > On 2015/04/21 02:12:12, Wez wrote:
>> > > Since you're only adding this in Me2MeDesktopEnvironment, rather
>>
> than in
>
>> > > BasicDesktopEnvironment, you won't get it set e.g. in the
>> > > IT2MeDesktopEnvironment, even though you're still telling it that
>>
> it
>
>> supports
>> > > multi-touch.
>> >
>> > BasicDE does implement GetCapabilities and checks the flag to
>>
> generate the
>
>> > capabilities string :)
>> > (And IT2MeDE inherits BasicDesktopEnvironment and IT2MeDE doesn't
>>
> override the
>
>> > method).
>>
>
>  Aha, missed that. I think it'd be cleaner to have this implementation
>>
> call the
>
>> BasicDE one and add kRateLimitResizeRequests to it, rather than
>>
> hard-coding in
>
>> the multi-touch-enabled explicitly. You could even get rid of the
>> supports_raw_touch_events() getter once you do that.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
> File remoting/host/basic_desktop_environment.cc (right):
>
>
>
https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
> remoting/host/basic_desktop_environment.cc:26: const char
> kRawTouchEventsCapability[] = "rawTouchEvents";
> On 2015/04/23 22:10:25, Wez wrote:
>
>> Why "raw" touch events?
>>
>
>  I think it'd be reasonable to call this touchEvents and we can then
>>
> call a
>
>> subsequent addition touchEventsV2, touchGestures, touchActions, etc,
>>
> as
>
>> necessary. :)
>>
>
>  nit: Is there really no common location we can move this definition
>>
> to? :-/
>
> Make sense :) will go with touchEvents.
>
> I saw kRateLimitResizeRequests being duplicated so I also duplicated,
> maybe I'm wrong because I only see kRateLimitResizeRequests in Me2MeDE
> and DesktopSessionProxy (essentially IpcDE) which are both Me2Me. I
> guess the translation unit scope prevents from accidentally setting it
> for It2Me sessions.
>
> Seems overkill to add 'host_capabilities.h' just for this capability. Do
> you think input_injector.h would be a reasonable place?
>
>
>
https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
> File remoting/host/basic_desktop_environment.h (right):
>
>
>
https://codereview.chromium.org/1033913003/diff/200001/remoting/host/basic_de...
> remoting/host/basic_desktop_environment.h:126: protected:
> On 2015/04/23 22:10:25, Wez wrote:
>
>> No need for another protected: here; the block above is already
>>
> protected.
>
> Oops. Removed.
>
> https://codereview.chromium.org/1033913003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Rintaro Kuroiwa, 2015-05-17 21:25:52]

Added capability_names.h

PTAL

[Wez, 2015-05-21 00:59:14]

Looking great - just a few comment nits to clean up, I think.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:67: // Enables or disables touch
events capability.
nit: This comment doesn't really add anything.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:68: void
set_supports_touch_events(bool enable) {
Do you need this setter, or can you just make it a ctor parameter?

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:90: // True if the desktop environment
supports touch events capability.
nit: True if the touch events capability should be offered.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:121: // Enables or disables touch
events capability.
It actually configures whether DesktopEnvironments created by this factory
advertise the capability, right?

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:139: // events.
Suggest phrasing this the same as the equivalent comment in the
DesktopEnvironment.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:482: main_run_loop_.Run();
nit: Consider moving these three lines into a RunMainLoopUntilDone() helper on
the test base?

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:487:
TEST_F(IpcDesktopEnvironmentTest, RawTouchEventsCapabilities) {
nit: Rename test, since capability has changed.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.cc:92: return
desktop_environment.Pass();
Did you mean to make this change? It doesn't seem to change any actual
semantics?

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/me2me_de...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:46: return
protocol::kRateLimitResizeRequests;
Suggest restructuring this as:

std::string capabilities = BasicDesktopEnvironment...
if (!capabilities.empty())
  capabilities.append(" ");
capabilities.append(kRateLimit...)

return capabilities;

https://codereview.chromium.org/1033913003/diff/280001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_session.js:211: // touch then the client will let
Chrome sythesize mouse events from touch
typo: synthesize

[Rintaro Kuroiwa, 2015-05-21 21:53:30]

PTAL :)

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
File remoting/host/basic_desktop_environment.h (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:67: // Enables or disables touch
events capability.
On 2015/05/21 00:59:14, Wez wrote:
> nit: This comment doesn't really add anything.

Done. REmoved.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:68: void
set_supports_touch_events(bool enable) {
On 2015/05/21 00:59:14, Wez wrote:
> Do you need this setter, or can you just make it a ctor parameter?

No we don't. It just requires changes to classes that inherit this. changed.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:90: // True if the desktop environment
supports touch events capability.
On 2015/05/21 00:59:14, Wez wrote:
> nit: True if the touch events capability should be offered.

Done.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:121: // Enables or disables touch
events capability.
On 2015/05/21 00:59:14, Wez wrote:
> It actually configures whether DesktopEnvironments created by this factory
> advertise the capability, right?

Removed this comment since these are just accessors.  changed the comment for
the bool.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/basic_de...
remoting/host/basic_desktop_environment.h:139: // events.
On 2015/05/21 00:59:14, Wez wrote:
> Suggest phrasing this the same as the equivalent comment in the
> DesktopEnvironment.

Done.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:482: main_run_loop_.Run();
On 2015/05/21 00:59:14, Wez wrote:
> nit: Consider moving these three lines into a RunMainLoopUntilDone() helper on
> the test base?

Oh, this 3 lines are duplicated in all tests. Maybe even call the method in 
Teardown then?

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:487:
TEST_F(IpcDesktopEnvironmentTest, RawTouchEventsCapabilities) {
On 2015/05/21 00:59:14, Wez wrote:
> nit: Rename test, since capability has changed.

Done.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/it2me_de...
File remoting/host/it2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/it2me_de...
remoting/host/it2me_desktop_environment.cc:92: return
desktop_environment.Pass();
On 2015/05/21 00:59:14, Wez wrote:
> Did you mean to make this change? It doesn't seem to change any actual
> semantics?

I haven't rebased so I must have made this change.
But I don't remember why I did this :/ Maybe I was playing around with the
interface and forgot to revert.
Done.

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/me2me_de...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/me2me_de...
remoting/host/me2me_desktop_environment.cc:46: return
protocol::kRateLimitResizeRequests;
On 2015/05/21 00:59:14, Wez wrote:
> Suggest restructuring this as:
> 
> std::string capabilities = BasicDesktopEnvironment...
> if (!capabilities.empty())
>   capabilities.append(" ");
> capabilities.append(kRateLimit...)
> 
> return capabilities;

Done.

https://codereview.chromium.org/1033913003/diff/280001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/webapp/crd/js...
remoting/webapp/crd/js/client_session.js:211: // touch then the client will let
Chrome sythesize mouse events from touch
On 2015/05/21 00:59:14, Wez wrote:
> typo: synthesize

Done.

[Wez, 2015-05-21 22:10:45]

LGTM - up to you re the test TearDown change.

Please have a chat w/ Jamie about actually pushing the feature. :)

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:482: main_run_loop_.Run();
On 2015/05/21 21:53:29, Rintaro Kuroiwa wrote:
> On 2015/05/21 00:59:14, Wez wrote:
> > nit: Consider moving these three lines into a RunMainLoopUntilDone() helper
on
> > the test base?
> 
> Oh, this 3 lines are duplicated in all tests. Maybe even call the method in 
> Teardown then?

That's a good idea, yes.

https://codereview.chromium.org/1033913003/diff/320001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/320001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:181: base::RunLoop
main_run_loop_;
nit: Make this private, so that the derived tests can't mistakenly try to use
it?

[Rintaro Kuroiwa, 2015-05-22 01:03:43]

On 2015/05/21 22:10:45, Wez wrote:
> LGTM - up to you re the test TearDown change.
> 
> Please have a chat w/ Jamie about actually pushing the feature. :)
Will do. Thanks! :)

> 
>
https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
> File remoting/host/ipc_desktop_environment_unittest.cc (right):
> 
>
https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
> remoting/host/ipc_desktop_environment_unittest.cc:482: main_run_loop_.Run();
> On 2015/05/21 21:53:29, Rintaro Kuroiwa wrote:
> > On 2015/05/21 00:59:14, Wez wrote:
> > > nit: Consider moving these three lines into a RunMainLoopUntilDone()
helper
> on
> > > the test base?
> > 
> > Oh, this 3 lines are duplicated in all tests. Maybe even call the method in 
> > Teardown then?
> 
> That's a good idea, yes.
> 
>
https://codereview.chromium.org/1033913003/diff/320001/remoting/host/ipc_desk...
> File remoting/host/ipc_desktop_environment_unittest.cc (right):
> 
>
https://codereview.chromium.org/1033913003/diff/320001/remoting/host/ipc_desk...
> remoting/host/ipc_desktop_environment_unittest.cc:181: base::RunLoop
> main_run_loop_;
> nit: Make this private, so that the derived tests can't mistakenly try to use
> it?

[Rintaro Kuroiwa, 2015-05-22 01:04:05]

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/280001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:482: main_run_loop_.Run();
On 2015/05/21 22:10:45, Wez wrote:
> On 2015/05/21 21:53:29, Rintaro Kuroiwa wrote:
> > On 2015/05/21 00:59:14, Wez wrote:
> > > nit: Consider moving these three lines into a RunMainLoopUntilDone()
helper
> on
> > > the test base?
> > 
> > Oh, this 3 lines are duplicated in all tests. Maybe even call the method in 
> > Teardown then?
> 
> That's a good idea, yes.

Done.

https://codereview.chromium.org/1033913003/diff/320001/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_unittest.cc (right):

https://codereview.chromium.org/1033913003/diff/320001/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_unittest.cc:181: base::RunLoop
main_run_loop_;
On 2015/05/21 22:10:45, Wez wrote:
> nit: Make this private, so that the derived tests can't mistakenly try to use
> it?

Done.

[Wez, 2015-06-02 19:53:03]

Planning to land this?

[Rintaro Kuroiwa, 2015-06-02 20:11:36]

On 2015/06/02 19:53:03, Wez wrote:
> Planning to land this?

Yes, but trying to make sure that this works :)

[Rintaro Kuroiwa, 2015-06-05 21:11:03]

The CQ bit was checked by rkuroiwa@chromium.org

[Rintaro Kuroiwa, 2015-06-05 21:11:04]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1033913003/#ps420001
(title: "change setter to public and add license header")

[commit-bot: I haz the power, 2015-06-05 21:12:29]

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

[commit-bot: I haz the power, 2015-06-05 21:39:14]

Committed patchset #21 (id:420001)

[commit-bot: I haz the power, 2015-06-05 21:40:47]

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