Add touch events to the protocol, the stub layer, and to the client plugin.

Add touch events to the protocol, the stub layer, and to the client plugin.

- Add TouchEvent and TouchEventPoint messages to the input event protocol.
- Extend the InputStub interface with an InjectTouchEvent() method.
- Update InputEventTracker to track touch points, and to cancel them
  when ReleaseAll() is called.
- Implement touch handling in the client plugin, but off by default.
  The web-app may choose to enable touch handling if desired.
- Add a TouchInputScaler that scales and clamps touch points, similarly to
  the MouseInputFilter, for use by both client and host.

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

[Rintaro Kuroiwa, 2015-01-17 00:19:13]

Patchset #5 (id:80001) has been deleted

[Rintaro Kuroiwa, 2015-01-17 00:20:44]

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

[Rintaro Kuroiwa, 2015-01-17 00:20:45]

I couldn't decide on how to the set sizes for touch_input_filter in
chromoting_instances.cc so the solution is naive.
Not sure if touch_input_filter.{h,cc} should be in protocol/
PTAL :)

[Wez, 2015-01-21 01:39:36]

Please update the CL text:

- Description indicates that we add touch input to "the CRD client", whereas
this CL seems to add touch events to the protocol, stub layer and to the client
plugin.
- Add a BUG= line referring to the bug for this work. :)
- "Touch events will not be forward to the plugin yet" is a confusing comment;
are you saying that this CL adds touch-event support to the plugin but does not
yet enable it (as we previously discussed)?

Thanks!

Wez


On 2015/01/17 00:20:45, rkuroiwa wrote:
> I couldn't decide on how to the set sizes for touch_input_filter in
> chromoting_instances.cc so the solution is naive.
> Not sure if touch_input_filter.{h,cc} should be in protocol/
> PTAL :)

[Wez, 2015-01-21 01:40:15]

Please also use "git cl try" in your checked-out branch, to get a full set of
try-bot runs that will tell you e.g. how badly you've broken Android. :P

[Wez, 2015-01-21 03:08:38]

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

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:219: // TODO(rkuroiwa): Add
PP_INPUTEVENT_CLASS_TOUCH when ready.
Shouldn't you be doing the registration in the HandleSendTouchInput() message
handler?

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:958:
input_handler_.set_send_touch_events(true);
Rather than setting a flag on the PepperInputHandler, just register w/ PPAPI for
touch input here?

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:36: // Crates
protocol::TouchEvent instance from |pp_touch_event|.
s/Creates/Creates

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:37: // Note that only the changed
touches are added to the instance.
s/instance/TouchEvent

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:39: using protocol::TouchEvent;
nit: Blank line after this "using", since it's not logically part of the
following code block.

Chromoting code typically uses "using" at file scope, or not at all, FWIW, so
this usage is a little odd; suggest moving to file scope or removing.

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.h (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.h:40: void
set_send_touch_events(bool send) {
See comments in ChromotingInstance re this

https://codereview.chromium.org/799233004/diff/100001/remoting/host/input_inj...
File remoting/host/input_injector_x11.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/host/input_inj...
remoting/host/input_injector_x11.cc:370: NOTIMPLEMENTED() << "Raw touch event
injection not implemented.";
nit: Why does this one get a message and the others not? :P

https://codereview.chromium.org/799233004/diff/100001/remoting/host/input_inj...
remoting/host/input_injector_x11.cc:371: return;
?

https://codereview.chromium.org/799233004/diff/100001/remoting/host/win/sessi...
File remoting/host/win/session_input_injector.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/host/win/sessi...
remoting/host/win/session_input_injector.cc:251: NOTIMPLEMENTED();
I think you meant to punt the event on to the Core here?

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:85: message Point {
If you define this as TouchPoint or TouchEventPoint at the top level then it is
possible to forward-declare it, to avoid having to #include the definition in
e.g. headers that don't need to know any of its internal details.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:93: // actions.
This is confusingly worded; I think what you mean is that CANCEL lets you
indicate that all the touch points are gone, without executing whatever action
might be triggered by the gesture the user made?

Do CANCEL events need X, Y, radius fields?

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:97: optional TouchPointEventType event_type = 1;
Doesn't this belong in the TouchEvent, rather than in the TouchEvent::Point?

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:104: // as well.
This comment is not relevant to the protocol; at the protocol level, these are
always host-relative, which is what your next comment says.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:110: // This determines the size of the touch region.
Suggest combining these to something like:

The size of the touch point, used to aid hit-testing.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:111: // Unlike x,y coordinates, these will not be
scaled.
Why not?

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/input...
File remoting/protocol/input_event_tracker.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:96:
input_stub_->InjectTouchEvent(event);
Don't you need to track the touch points so you can inject a CANCEL event in
ReleaseAll()?

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
File remoting/protocol/touch_input_filter.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:18: return std::max(0.0f,
std::min(static_cast<float>(output_max), new_value));
This seems a convoluted way to get the effect you want!

You can do the following:

value *= output_max
value /= input_max

Since value is float that shouldn't lose you any precision with the sorts of
values we're dealing with.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:22: TouchInputFilter::TouchInputFilter()
{
You can use c++11 default constructor here (i.e. =default syntax)

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:39: // which is important when
down-scaling.
Thinking Out Loud: It does seem a shame to be constructing a new TouchEvent even
when the two sizes match, and the original event has valid contents; probably
not worth optimizing, though.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:43: if (point->has_x()) {
Is it possible for events to have an X coordinate but not Y, or vice versa? If
not then consider checking those fields in a single if() here, and ignoring the
event entirely if it's malformed.

Alternatively, you might consider sanity-checking the event for the presence of
X & Y fields in the HostEventDispatcher, so that this code needn't bother
(though we typically don't use that approach for the other input types).

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
File remoting/protocol/touch_input_filter.h (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
Remove (c) - that's from some ancient license wording!

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:8: #include "base/macros.h"
What do we need base/macros.h explicitly pulling in for?

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:17: // TODO(rkuroiwa): Consider renaming
this to TouchInputRescaler.
Agreed; suggest TouchInputScaler.

Is this used at the client, or the host, or both?

MouseInputFilter is used at both; at the client it [sometimes] scales, and at
the host it clamps, so that we avoid injecting mouse events that lay outside the
desktop bounds.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:24: void set_input_size(const
webrtc::DesktopSize& size);
These two setters should match the names of the fields they set, and be in-lined
(sorry, can't find the style-guide section that says that...).

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:28: // Scales the coordinates to host x,y
value.
This comment doesn't add anything; this method is just here to override the
InputStub interface for the desired behaviour. It would be useful to document
the behaviour either with a class-level comment, or with comments on the
setters, and just leave a comment in our usual style:

// protocol::InputStub interface.

here.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:34: webrtc::DesktopSize output_max_;
nit: Why the max_ suffix? Surely we always scale to these dimensions?

They should be named to match the setters, e.g. input_size_ and output_size_.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
File remoting/protocol/touch_input_filter_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter_unittest.cc:19: namespace {
nit: Blank line between namespace { and } lines, and the code in-between, plz.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter_unittest.cc:22: // If it's within 1 pixel
diff, it should be acceptable.
What is "it" in this comment? :)

[Rintaro Kuroiwa, 2015-01-28 01:12:30]

PTAL

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

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:219: // TODO(rkuroiwa): Add
PP_INPUTEVENT_CLASS_TOUCH when ready.
On 2015/01/21 03:08:37, Wez wrote:
> Shouldn't you be doing the registration in the HandleSendTouchInput() message
> handler?

Moved. But it *should* check for capabilities as well for performance reasons.
That might be javascript layer's job.

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:958:
input_handler_.set_send_touch_events(true);
On 2015/01/21 03:08:37, Wez wrote:
> Rather than setting a flag on the PepperInputHandler, just register w/ PPAPI
for
> touch input here?

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:36: // Crates
protocol::TouchEvent instance from |pp_touch_event|.
On 2015/01/21 03:08:37, Wez wrote:
> s/Creates/Creates

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:37: // Note that only the changed
touches are added to the instance.
On 2015/01/21 03:08:37, Wez wrote:
> s/instance/TouchEvent

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:39: using protocol::TouchEvent;
On 2015/01/21 03:08:37, Wez wrote:
> nit: Blank line after this "using", since it's not logically part of the
> following code block.
> 
> Chromoting code typically uses "using" at file scope, or not at all, FWIW, so
> this usage is a little odd; suggest moving to file scope or removing.

Not saving any lines, removing :)

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.h (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.h:40: void
set_send_touch_events(bool send) {
On 2015/01/21 03:08:37, Wez wrote:
> See comments in ChromotingInstance re this

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/host/input_inj...
File remoting/host/input_injector_x11.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/host/input_inj...
remoting/host/input_injector_x11.cc:370: NOTIMPLEMENTED() << "Raw touch event
injection not implemented.";
On 2015/01/21 03:08:37, Wez wrote:
> nit: Why does this one get a message and the others not? :P

Oh I wanted to get Win, Mac, and CroS building and forgot that I put a message
for X11. For consistency, I'll add comments to Mac, Win, and CrOS and remove
InputInjectorX11::Core::InjectTouchEvent.

https://codereview.chromium.org/799233004/diff/100001/remoting/host/input_inj...
remoting/host/input_injector_x11.cc:371: return;
On 2015/01/21 03:08:37, Wez wrote:
> ?

Not sure why I did this :P

https://codereview.chromium.org/799233004/diff/100001/remoting/host/win/sessi...
File remoting/host/win/session_input_injector.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/host/win/sessi...
remoting/host/win/session_input_injector.cc:251: NOTIMPLEMENTED();
On 2015/01/21 03:08:37, Wez wrote:
> I think you meant to punt the event on to the Core here?

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:85: message Point {
On 2015/01/21 03:08:37, Wez wrote:
> If you define this as TouchPoint or TouchEventPoint at the top level then it
is
> possible to forward-declare it, to avoid having to #include the definition in
> e.g. headers that don't need to know any of its internal details.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:93: // actions.
On 2015/01/21 03:08:37, Wez wrote:
> This is confusingly worded; I think what you mean is that CANCEL lets you
> indicate that all the touch points are gone, without executing whatever action
> might be triggered by the gesture the user made?
> 
> Do CANCEL events need X, Y, radius fields?

If I understood you correctly, you mean CANCEL cancels all the touch points that
are currently "down".
No, that is not what cancel is. CANCEL is a type of END (or UP for some
platforms) but it is up to the application to distinguish between CANCEL and
END.
Android MotionEvent.ACTION_CANCEL has this documented
http://developer.android.com/reference/android/view/MotionEvent.html#ACTION_C...
I've also seen CANCEL events from PPAPI when I released two finger off the
digitizer almost at the same time. The one that went off was END and latter was
CANCEL.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:97: optional TouchPointEventType event_type = 1;
On 2015/01/21 03:08:37, Wez wrote:
> Doesn't this belong in the TouchEvent, rather than in the TouchEvent::Point?

Yes.
Initially I thought I would need an event that can report some touch points went
up, some moved, and some down at the same time. But after some research, I've
never seen an interface where an event contains a combination of them. It's
always UP MOVE or DOWN.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:104: // as well.
On 2015/01/21 03:08:37, Wez wrote:
> This comment is not relevant to the protocol; at the protocol level, these are
> always host-relative, which is what your next comment says.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:110: // This determines the size of the touch region.
On 2015/01/21 03:08:37, Wez wrote:
> Suggest combining these to something like:
> 
> The size of the touch point, used to aid hit-testing.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:111: // Unlike x,y coordinates, these will not be
scaled.
On 2015/01/21 03:08:37, Wez wrote:
> Why not?

I actually don't have a good reason for or against size scaling. Do you think
the user would expect (and would be a better user experience with) size scaling?

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/input...
File remoting/protocol/input_event_tracker.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:96:
input_stub_->InjectTouchEvent(event);
On 2015/01/21 03:08:37, Wez wrote:
> Don't you need to track the touch points so you can inject a CANCEL event in
> ReleaseAll()?

Oops. I didn't read the header file carefully. Done and added tests.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
File remoting/protocol/touch_input_filter.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:18: return std::max(0.0f,
std::min(static_cast<float>(output_max), new_value));
On 2015/01/21 03:08:38, Wez wrote:
> This seems a convoluted way to get the effect you want!
> 
> You can do the following:
> 
> value *= output_max
> value /= input_max
> 
> Since value is float that shouldn't lose you any precision with the sorts of
> values we're dealing with.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:22: TouchInputFilter::TouchInputFilter()
{
On 2015/01/21 03:08:37, Wez wrote:
> You can use c++11 default constructor here (i.e. =default syntax)

wow, C++11 :)
Done

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:39: // which is important when
down-scaling.
On 2015/01/21 03:08:38, Wez wrote:
> Thinking Out Loud: It does seem a shame to be constructing a new TouchEvent
even
> when the two sizes match, and the original event has valid contents; probably
> not worth optimizing, though.

Since I'm implementing the client side atm, WDYT about only scaling in this
class? Then if the desktop sizes match, I can immediately delegate to
InputFilter::InjectTouchEvent() without making a copy?
Clamping would have to happen on host if the coordinates are outside the range.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.cc:43: if (point->has_x()) {
On 2015/01/21 03:08:38, Wez wrote:
> Is it possible for events to have an X coordinate but not Y, or vice versa? If
> not then consider checking those fields in a single if() here, and ignoring
the
> event entirely if it's malformed.
> 
> Alternatively, you might consider sanity-checking the event for the presence
of
> X & Y fields in the HostEventDispatcher, so that this code needn't bother
> (though we typically don't use that approach for the other input types).

No, there is X iff Y. Making this || and DCHECKing to make sure it is &&.

Actually, none of the fields, except the ID, are necessary for END and CANCEL
events because the host should have all the other information from the previous
events. Realized this when changing InputEventTracker.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
File remoting/protocol/touch_input_filter.h (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/01/21 03:08:38, Wez wrote:
> Remove (c) - that's from some ancient license wording!

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:8: #include "base/macros.h"
On 2015/01/21 03:08:38, Wez wrote:
> What do we need base/macros.h explicitly pulling in for?

DISALLOW_COPY_AND_ASSIGN is defined there. It still compiles without this so
removing.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:17: // TODO(rkuroiwa): Consider renaming
this to TouchInputRescaler.
On 2015/01/21 03:08:38, Wez wrote:
> Agreed; suggest TouchInputScaler.
> 
> Is this used at the client, or the host, or both?
> 
> MouseInputFilter is used at both; at the client it [sometimes] scales, and at
> the host it clamps, so that we avoid injecting mouse events that lay outside
the
> desktop bounds.

Renaming to TouchInputScalar.

Looking at the MouseInputFiter code, why is clamping done on client and host
side? Is it due to a possible race condition where it gets clamped on the client
side but by the time it reached the host, host resolution has changed?

I'll move this to remoting/client/plugin and if we need it on both ends, we can
move it back to remoting/protocol

WDYT?

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:24: void set_input_size(const
webrtc::DesktopSize& size);
On 2015/01/21 03:08:38, Wez wrote:
> These two setters should match the names of the fields they set, and be
in-lined
> (sorry, can't find the style-guide section that says that...).

Yes, I'm aware of it :) Renaming.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:28: // Scales the coordinates to host x,y
value.
On 2015/01/21 03:08:38, Wez wrote:
> This comment doesn't add anything; this method is just here to override the
> InputStub interface for the desired behaviour. It would be useful to document
> the behaviour either with a class-level comment, or with comments on the
> setters, and just leave a comment in our usual style:
> 
> // protocol::InputStub interface.
> 
> here.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter.h:34: webrtc::DesktopSize output_max_;
On 2015/01/21 03:08:38, Wez wrote:
> nit: Why the max_ suffix? Surely we always scale to these dimensions?
> 
> They should be named to match the setters, e.g. input_size_ and output_size_.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
File remoting/protocol/touch_input_filter_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter_unittest.cc:19: namespace {
On 2015/01/21 03:08:38, Wez wrote:
> nit: Blank line between namespace { and } lines, and the code in-between, plz.

Done.

https://codereview.chromium.org/799233004/diff/100001/remoting/protocol/touch...
remoting/protocol/touch_input_filter_unittest.cc:22: // If it's within 1 pixel
diff, it should be acceptable.
On 2015/01/21 03:08:38, Wez wrote:
> What is "it" in this comment? :)

Done.

[Rintaro Kuroiwa, 2015-01-29 07:37:46]

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/100001/remoting/proto/event.pr...
remoting/proto/event.proto:111: // Unlike x,y coordinates, these will not be
scaled.
On 2015/01/28 01:12:29, Rintaro Kuroiwa wrote:
> On 2015/01/21 03:08:37, Wez wrote:
> > Why not?
> 
> I actually don't have a good reason for or against size scaling. Do you think
> the user would expect (and would be a better user experience with) size
scaling?

After giving it some more thought, scaling the size is the only reasonable
solution. I'll work on this tomorrow :)

[Sergey Ulanov, 2015-01-29 18:00:45]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2015-01-29 18:00:46]

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

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:355: } else if (method ==
"sendTouchEvents") {
Why do we need this? Doesn't the plugin get touch events directly from Pepper?

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:22: value /= input_max;
handle the case when  input_max == 0.

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.h (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:17: // Scales the coordinates to
host x,y value. The
finish the comment

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:26: input_size_.set(size.width() -
1, size.height() - 1);
why do you need -1 here?

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:94: // Unlike x,y coordinates, these will not be
scaled.
You also need rotation angle. It's not useful to separate x and y coordinates
without it.

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:95: optional float radius_x = 4;
Do we also need a field for pressure/force?

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:102: TOUCH_POINT_START = 1;
I'm not sure I understand what these types mean. E.g. if user was touch the
screen with one finger and then puts down the second finger, is it START or
MOVE? When is END is sent?

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:112: repeated TouchEventPoint touch_points = 2;
Should this include all current touches or only those that have changed?

[Sergey Ulanov, 2015-01-29 18:06:46]

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

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:355: } else if (method ==
"sendTouchEvents") {
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> Why do we need this? Doesn't the plugin get touch events directly from Pepper?

Ah, so it just enables touch events. Call it enableTouchEvents?

[Rintaro Kuroiwa, 2015-01-30 17:57:03]

ptal

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

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:355: } else if (method ==
"sendTouchEvents") {
On 2015/01/29 18:06:46, Sergey Ulanov wrote:
> On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> > Why do we need this? Doesn't the plugin get touch events directly from
Pepper?
> 
> Ah, so it just enables touch events. Call it enableTouchEvents?

Done.

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:22: value /= input_max;
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> handle the case when  input_max == 0.

is_empty() check as line 36 should handle that. I'll add DCHECKs.

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.h (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:17: // Scales the coordinates to
host x,y value. The
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> finish the comment

Done.

https://codereview.chromium.org/799233004/diff/140001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:26: input_size_.set(size.width() -
1, size.height() - 1);
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> why do you need -1 here?

Added a comment.

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:94: // Unlike x,y coordinates, these will not be
scaled.
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> You also need rotation angle. It's not useful to separate x and y coordinates
> without it.

Done.

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:95: optional float radius_x = 4;
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> Do we also need a field for pressure/force?

wez mentioned that rotation angle and pressure are advanced fields that we could
add them later if we're not clear on how we handle them. Angle makes sense to be
here now because we use radius_{x,y}.
For pressure, most APIs say that the pressure is "usually" within some range.
Until we decide how we use it or what to do when it's not in the range, we could
make them a constant on the host side. WDYT?

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:102: TOUCH_POINT_START = 1;
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> I'm not sure I understand what these types mean. E.g. if user was touch the
> screen with one finger and then puts down the second finger, is it START or
> MOVE? When is END is sent? 

When a user starts touching a screen with a new finger, that is a START. If the
finger that was already on the screen moved then it would be a MOVE event.
END happens when a user lifts one or more fingers off the screen. Similar for
CANCEL but the meaning is slightly different as noted in the comment.
Should I add a comment in the proto?

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:112: repeated TouchEventPoint touch_points = 2;
On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> Should this include all current touches or only those that have changed?

Only those points that changed.
This seems to be the popular type of API for most touch handling APIs, where
it's easier to access the points that has changed since the last event. Since
the host can keep track of all the points (if it needs all of them for
injection), only the changes are on the wire.

Adding comment.

[Sergey Ulanov, 2015-02-02 19:32:05]

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:95: optional float radius_x = 4;
On 2015/01/30 17:57:02, Rintaro Kuroiwa wrote:
> On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> > Do we also need a field for pressure/force?
> 
> wez mentioned that rotation angle and pressure are advanced fields that we
could
> add them later if we're not clear on how we handle them. Angle makes sense to
be
> here now because we use radius_{x,y}.
> For pressure, most APIs say that the pressure is "usually" within some range.
> Until we decide how we use it or what to do when it's not in the range, we
could
> make them a constant on the host side. WDYT?

We do get pressure values on Android, Web and Pepper and they all use the same
range [0, 1].
I think it's still better to add it here even if it's not supported on some host
platforms. It's better to send all data that the client gets for each event and
let the host decide how to inject them depending on feature the target OS
supports (e.g. ignore the pressure if the OS doesn't support it). E.g. initially
we were sending only delta values for the wheel events and that was causing
problems on platforms that need ticks (particularly X11).

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:102: TOUCH_POINT_START = 1;
On 2015/01/30 17:57:02, Rintaro Kuroiwa wrote:
> On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> > I'm not sure I understand what these types mean. E.g. if user was touch the
> > screen with one finger and then puts down the second finger, is it START or
> > MOVE? When is END is sent? 
> 
> When a user starts touching a screen with a new finger, that is a START. If
the
> finger that was already on the screen moved then it would be a MOVE event.
> END happens when a user lifts one or more fingers off the screen. Similar for
> CANCEL but the meaning is slightly different as noted in the comment.

I see.
What's confusing to me is that there are multiple touch_points, and only one
event_type.
If host gets END with multiple touch points then it means that two fingers were
lifted at the same time, right? What if one was lifted while another one moved?
Will the client send two separate events in that case? If so why can't it send
two separate events for two released fingers (i.e. touch_points doesn't need to
be repeated)? 

> Should I add a comment in the proto?

Yes please.

[Rintaro Kuroiwa, 2015-02-02 22:22:56]

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:95: optional float radius_x = 4;
On 2015/02/02 19:32:05, Sergey Ulanov wrote:
> On 2015/01/30 17:57:02, Rintaro Kuroiwa wrote:
> > On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> > > Do we also need a field for pressure/force?
> > 
> > wez mentioned that rotation angle and pressure are advanced fields that we
> could
> > add them later if we're not clear on how we handle them. Angle makes sense
to
> be
> > here now because we use radius_{x,y}.
> > For pressure, most APIs say that the pressure is "usually" within some
range.
> > Until we decide how we use it or what to do when it's not in the range, we
> could
> > make them a constant on the host side. WDYT?
> 
> We do get pressure values on Android, Web and Pepper and they all use the same
> range [0, 1].
> I think it's still better to add it here even if it's not supported on some
host
> platforms. It's better to send all data that the client gets for each event
and
> let the host decide how to inject them depending on feature the target OS
> supports (e.g. ignore the pressure if the OS doesn't support it). E.g.
initially
> we were sending only delta values for the wheel events and that was causing
> problems on platforms that need ticks (particularly X11).

Done.

https://codereview.chromium.org/799233004/diff/140001/remoting/proto/event.pr...
remoting/proto/event.proto:102: TOUCH_POINT_START = 1;
On 2015/02/02 19:32:05, Sergey Ulanov wrote:
> On 2015/01/30 17:57:02, Rintaro Kuroiwa wrote:
> > On 2015/01/29 18:00:46, Sergey Ulanov wrote:
> > > I'm not sure I understand what these types mean. E.g. if user was touch
the
> > > screen with one finger and then puts down the second finger, is it START
or
> > > MOVE? When is END is sent? 
> > 
> > When a user starts touching a screen with a new finger, that is a START. If
> the
> > finger that was already on the screen moved then it would be a MOVE event.
> > END happens when a user lifts one or more fingers off the screen. Similar
for
> > CANCEL but the meaning is slightly different as noted in the comment.
> 
> I see.
> What's confusing to me is that there are multiple touch_points, and only one
> event_type.
> If host gets END with multiple touch points then it means that two fingers
were
> lifted at the same time, right? What if one was lifted while another one
moved?
> Will the client send two separate events in that case? If so why can't it send
> two separate events for two released fingers (i.e. touch_points doesn't need
to
> be repeated)? 

Repeating myself what we discussed offline.

Correct, in terms of this protocol if the event type is END and if there were 2
touch_points, then that means 2 fingers went off the screen at the same time.
For most (all the ones that I looked at) touch event APIs:
- If one finger was lifted and the other has moved, those will be different
events.
- Typically multiple points are reported in a MOVE event (I checked this in
PPAPI and according to Android MotionEvent documentation, there could be
multiple move points in an event).
- But usually START, END, and CANCEL events only report 1 point.

It is true that we can send multiple MOVE events instead of making touch_points
field repeated.

For the APIs that we know I think it makes sense to have event_type here rather
than in TouchEventPoint.
If we find it more efficient to batch START, END, MOVE, and CANCEL in an event,
we could add a field in TouchEventPoint later that would deprecate this field.

> 
> > Should I add a comment in the proto?
> 
> Yes please.

[Wez, 2015-02-05 02:09:08]

Great! This is looking really good. :)

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

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:219: // TODO(rkuroiwa): Add
PP_INPUTEVENT_CLASS_TOUCH when ready.
On 2015/01/28 01:12:29, Rintaro Kuroiwa wrote:
> On 2015/01/21 03:08:37, Wez wrote:
> > Shouldn't you be doing the registration in the HandleSendTouchInput()
message
> > handler?
> 
> Moved. But it *should* check for capabilities as well for performance reasons.
> That might be javascript layer's job.

Check what for capabilities? The client? Or the host?

Either way, would you not still wait until HandleSendTouchInput() is called to
do the registration?

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:36: // Creates
protocol::TouchEvent instance from |pp_touch_event|.
"Creates a protocol::TouchEvent..."

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:40:
TouchInputScaler::~TouchInputScaler() {
nit: You can put the {} on the same line since the entire fn definition fits on
a single line that way. You could even do the same for the ctor if it will still
fit in 80 columns.

(Have you tried using git cl format, BTW?)

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:63: if (point->has_radius_x() ||
point->has_radius_y()) {
nit: Suggest adding a comment to explain why we need to scale the radius as
well.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.h (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:27: // size.
The "-1..." comment doesn't really help clarify anything; I think it's
sufficient to leave the setter with the simple comment and add a note to the
member-variable definitions to clarify that they actually hold the max-X and
max-Y coordinates, not the width & height.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:9: #include
"base/logging.h"
If you remove the CHECKs you shouldn't need this include.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:23: const float
kDefaultRadius = 30.0f;
nit: Suggest moving this down to near the PointInfo type, since that is where
it's used.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:26: //
TouchPointCoordinateEqual is within 1 pixel diff, it should be acceptable.
nit: Suggest "Require touch point coordinates to be correct to within one
pixel."

Note that the way you're asserting that, if you expect a value of e.g. x=4.9,
then you'll allow x=5.5, which will resolve to a different physical pixel when
rounded by the host - is that desirable?

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:91: void
AddDefaultTestCoordinate() { point_infos_.push_back({1.0f, 1.0f}); }
Earlier you defined kDefaultRadius; consider defining kDefaultPointInfo or
kDefaultXCoord and kDefaultYCoord, say.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:94:
CHECK(!point_infos_.empty());
In gtest we use ASSERT rather than CHECK.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:124: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:128: // either dimensions
are not set.
Suggest explicitly setting the dimensions to zero, since that's what the test
name suggests, or changing the test name from zero to unset, or ZeroOrUnset, for
example.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:165: // Make sure clamping
works.
nit: Suggest rewording this to be similar to the wording above, e.g. "the
coordinates fall outside the desktop area".

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:176: point->set_y(1.0f);
Does it matter that the radius of the event may fall partly outside the desktop?
(I'd guess not, but...)

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:183:
TEST_F(TouchInputScalerTest, ClampingMultiplePointsNoScaling) {
Suggest including an un-clamped point in this test, too. :)

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:214: // Verify up scaling
works. All coordinates should fall inside the output
nit: up-scaling

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:215: // dimensions, after
scaling, i.e. no clamping testing.
not sure what "no clamping testing" means

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:227: point->set_y(1.0f *
kUpScalingFactor);
You're calculating the expected output from the same values as the input and
scale factor, which is bad ju-ju - suggest putting the expected values here
explicitly, and getting rid of the kUpScalingFactor constant

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:238: // Verify up scaling
works.
You mean works, with clamping?

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:259:
TEST_F(TouchInputScalerTest, DownScalingNoClamp) {
NoClamping

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:281: // Verify down
scaling works.
As above

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:315: // Scaling up by
factor of 2.
This comment doesn't seem to relate to the test?

Shouldn't you test this as part of the up-scaling test earlier on?

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:330: // Scaling down by
factor of 2.
As above.

https://codereview.chromium.org/799233004/diff/200001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/proto/event.pr...
remoting/proto/event.proto:91: optional float y = 3;
nit: Do these need to be floats? Chromoting works in device-pixel coordinates
(versus logical-pixel), so whereas logical pixel positions can be fractional on
a high-DPI device, ours can't.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
File remoting/protocol/input_event_tracker.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:29: void
InputEventTracker::ReleaseAll() {
Now that this method is longer, I'd suggest a comment before each block, e.g.
"// Release pressed keys.", "// Cancel active touch points." etc

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:31: for (i = pressed_keys_.begin(); i
!= pressed_keys_.end(); ++i) {
nit: While you're in this file, consider auto-izing this loop, for consistency
with your touch events loop, below.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:109: switch (event.event_type()) {
nit: Consider noting (similarly to InjectKeyEvent's comment) that we only need
the touch-point Ids in order to cancel them in ReleaseAll().

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
File remoting/protocol/input_event_tracker_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:281: }
nit: Blank line between this and the next block

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:288: .After(injects);
Why is this not just in the sequence |s|?

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:329: }
nit: Blank line before next logical block.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:335: .After(injects);
Why is this not just in the sequence |s|?

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:351: // All touch points are
released before ReleaseAll() is called.
Do you mean "test that no touch events are generated if all events are released
before releaseall() is called"?

This test feels a little overkill, given the test above; it's just a degenerate
case of injecting and then removing some touch points before releasing?

[Rintaro Kuroiwa, 2015-02-06 23:35:02]

PTAL!

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

https://codereview.chromium.org/799233004/diff/100001/remoting/client/plugin/...
remoting/client/plugin/chromoting_instance.cc:219: // TODO(rkuroiwa): Add
PP_INPUTEVENT_CLASS_TOUCH when ready.
On 2015/02/05 02:09:06, Wez wrote:
> On 2015/01/28 01:12:29, Rintaro Kuroiwa wrote:
> > On 2015/01/21 03:08:37, Wez wrote:
> > > Shouldn't you be doing the registration in the HandleSendTouchInput()
> message
> > > handler?
> > 
> > Moved. But it *should* check for capabilities as well for performance
reasons.
> > That might be javascript layer's job.
> 
> Check what for capabilities? The client? Or the host?
> 
> Either way, would you not still wait until HandleSendTouchInput() is called to
> do the registration?

The capabilities. IIUC, the javascript layer checks the capabilities?
I moved it to HandleSendTouchInput() (renamed to HandleEnableTouchEvents() in
the latest patch).

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:36: // Creates
protocol::TouchEvent instance from |pp_touch_event|.
On 2015/02/05 02:09:06, Wez wrote:
> "Creates a protocol::TouchEvent..."

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:40:
TouchInputScaler::~TouchInputScaler() {
On 2015/02/05 02:09:06, Wez wrote:
> nit: You can put the {} on the same line since the entire fn definition fits
on
> a single line that way. You could even do the same for the ctor if it will
still
> fit in 80 columns.
> 
> (Have you tried using git cl format, BTW?)

oh wow I didn't know about git cl format :)

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:63: if (point->has_radius_x() ||
point->has_radius_y()) {
On 2015/02/05 02:09:06, Wez wrote:
> nit: Suggest adding a comment to explain why we need to scale the radius as
> well.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.h (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:27: // size.
On 2015/02/05 02:09:06, Wez wrote:
> The "-1..." comment doesn't really help clarify anything; I think it's
> sufficient to leave the setter with the simple comment and add a note to the
> member-variable definitions to clarify that they actually hold the max-X and
> max-Y coordinates, not the width & height.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:9: #include
"base/logging.h"
On 2015/02/05 02:09:06, Wez wrote:
> If you remove the CHECKs you shouldn't need this include.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:23: const float
kDefaultRadius = 30.0f;
On 2015/02/05 02:09:07, Wez wrote:
> nit: Suggest moving this down to near the PointInfo type, since that is where
> it's used.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:26: //
TouchPointCoordinateEqual is within 1 pixel diff, it should be acceptable.
On 2015/02/05 02:09:07, Wez wrote:
> nit: Suggest "Require touch point coordinates to be correct to within one
> pixel."
> 
> Note that the way you're asserting that, if you expect a value of e.g. x=4.9,
> then you'll allow x=5.5, which will resolve to a different physical pixel when
> rounded by the host - is that desirable?

I think off by 1 pixel is acceptable.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:91: void
AddDefaultTestCoordinate() { point_infos_.push_back({1.0f, 1.0f}); }
On 2015/02/05 02:09:07, Wez wrote:
> Earlier you defined kDefaultRadius; consider defining kDefaultPointInfo or
> kDefaultXCoord and kDefaultYCoord, say.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:94:
CHECK(!point_infos_.empty());
On 2015/02/05 02:09:06, Wez wrote:
> In gtest we use ASSERT rather than CHECK.

I thought CHECK is more appropriate here because it is a bug in how the test is
written. But the test will fail somewhere else so I'll remove this.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:124: };
On 2015/02/05 02:09:07, Wez wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:128: // either dimensions
are not set.
On 2015/02/05 02:09:07, Wez wrote:
> Suggest explicitly setting the dimensions to zero, since that's what the test
> name suggests, or changing the test name from zero to unset, or ZeroOrUnset,
for
> example.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:165: // Make sure clamping
works.
On 2015/02/05 02:09:07, Wez wrote:
> nit: Suggest rewording this to be similar to the wording above, e.g. "the
> coordinates fall outside the desktop area".

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:176: point->set_y(1.0f);
On 2015/02/05 02:09:06, Wez wrote:
> Does it matter that the radius of the event may fall partly outside the
desktop?
> (I'd guess not, but...)

This test does not due to the matcher that is used.

I added a TODO in TouchInputScaler. I know that Windows touch injection API does
not like any touch point area falling outside the screen. Honestly I'm not sure
about how other platforms work.

But obviously if the ellipse encloses the entire desktop, then that's  a case
where we can make the size (radii) smaller.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:183:
TEST_F(TouchInputScalerTest, ClampingMultiplePointsNoScaling) {
On 2015/02/05 02:09:07, Wez wrote:
> Suggest including an un-clamped point in this test, too. :)

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:214: // Verify up scaling
works. All coordinates should fall inside the output
On 2015/02/05 02:09:07, Wez wrote:
> nit: up-scaling

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:215: // dimensions, after
scaling, i.e. no clamping testing.
On 2015/02/05 02:09:06, Wez wrote:
> not sure what "no clamping testing" means

Meaning this should only test scaling and should not test the combination of
scaling and clamping.
Reworded.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:227: point->set_y(1.0f *
kUpScalingFactor);
On 2015/02/05 02:09:07, Wez wrote:
> You're calculating the expected output from the same values as the input and
> scale factor, which is bad ju-ju - suggest putting the expected values here
> explicitly, and getting rid of the kUpScalingFactor constant

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:238: // Verify up scaling
works.
On 2015/02/05 02:09:06, Wez wrote:
> You mean works, with clamping?

Yes. Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:259:
TEST_F(TouchInputScalerTest, DownScalingNoClamp) {
On 2015/02/05 02:09:07, Wez wrote:
> NoClamping

Done. Renamed to UpScalingNoClamping as well.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:281: // Verify down
scaling works.
On 2015/02/05 02:09:07, Wez wrote:
> As above

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:315: // Scaling up by
factor of 2.
On 2015/02/05 02:09:06, Wez wrote:
> This comment doesn't seem to relate to the test?
> 
> Shouldn't you test this as part of the up-scaling test earlier on?

Yes, the comment is a test detail, not for explaining the test. Changed.

This uses a different matcher that only tests the radii scaling, I wanted to do
test this independently from x,y scaling.

I'll add tests that do both.

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:330: // Scaling down by
factor of 2.
On 2015/02/05 02:09:07, Wez wrote:
> As above.

same.

https://codereview.chromium.org/799233004/diff/200001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/proto/event.pr...
remoting/proto/event.proto:91: optional float y = 3;
On 2015/02/05 02:09:07, Wez wrote:
> nit: Do these need to be floats? Chromoting works in device-pixel coordinates
> (versus logical-pixel), so whereas logical pixel positions can be fractional
on
> a high-DPI device, ours can't.

As discussed offline.
Some touch event APIs mention that some digitizers can detect sub-pixel
coordinates and would be reported as floating point numbers.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
File remoting/protocol/input_event_tracker.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:29: void
InputEventTracker::ReleaseAll() {
On 2015/02/05 02:09:07, Wez wrote:
> Now that this method is longer, I'd suggest a comment before each block, e.g.
> "// Release pressed keys.", "// Cancel active touch points." etc

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:31: for (i = pressed_keys_.begin(); i
!= pressed_keys_.end(); ++i) {
On 2015/02/05 02:09:07, Wez wrote:
> nit: While you're in this file, consider auto-izing this loop, for consistency
> with your touch events loop, below.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker.cc:109: switch (event.event_type()) {
On 2015/02/05 02:09:07, Wez wrote:
> nit: Consider noting (similarly to InjectKeyEvent's comment) that we only need
> the touch-point Ids in order to cancel them in ReleaseAll().

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
File remoting/protocol/input_event_tracker_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:281: }
On 2015/02/05 02:09:07, Wez wrote:
> nit: Blank line between this and the next block

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:288: .After(injects);
On 2015/02/05 02:09:07, Wez wrote:
> Why is this not just in the sequence |s|?

This test works with InSequence. Removed ExpecatationSet.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:329: }
On 2015/02/05 02:09:07, Wez wrote:
> nit: Blank line before next logical block.

Done.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:335: .After(injects);
On 2015/02/05 02:09:07, Wez wrote:
> Why is this not just in the sequence |s|?

This test works with InSequence. Removed ExpecatationSet.

https://codereview.chromium.org/799233004/diff/200001/remoting/protocol/input...
remoting/protocol/input_event_tracker_unittest.cc:351: // All touch points are
released before ReleaseAll() is called.
On 2015/02/05 02:09:08, Wez wrote:
> Do you mean "test that no touch events are generated if all events are
released
> before releaseall() is called"?
> 
> This test feels a little overkill, given the test above; it's just a
degenerate
> case of injecting and then removing some touch points before releasing?

Yes, that's what I meant :)

True, the test above tests that END (I should test CANCEL) event does remove the
point and will not be in ReleaseAll(). Removing this test.

And adding CANCEL event to the test above.

[Wez, 2015-02-13 16:51:46]

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/200001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:26: //
TouchPointCoordinateEqual is within 1 pixel diff, it should be acceptable.
On 2015/02/06 23:35:01, Rintaro Kuroiwa wrote:
> On 2015/02/05 02:09:07, Wez wrote:
> > nit: Suggest "Require touch point coordinates to be correct to within one
> > pixel."
> > 
> > Note that the way you're asserting that, if you expect a value of e.g.
x=4.9,
> > then you'll allow x=5.5, which will resolve to a different physical pixel
when
> > rounded by the host - is that desirable?
> 
> I think off by 1 pixel is acceptable.

Acknowledged.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:41: switch
(pp_touch_event.GetType()) {
nit: Move this switch into a helper function? You'd need to have the NOTREACHED
case return some placeholder value for MakeTouchEvent to catch.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:62: // does not look too big or too
small on the output desktop, compared to the
nit: The normal impact of this will not be on the visual appearance of the
touch, I think, but on the hit-test area within which it'll affect widgets.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:67: // Also reduce the radii if the
ellipse completely encloses the desktop.
Based on the Windows case, I'd suggest choosing between clamping here and
deferring to the individual injectors - I'd suggest clamping here until/unless
we find some platform has a use for touches that exceed the desktop bounds.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.h (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:19: // Also resizes the touch radii.
nit: "touch radii" reads oddly; "touch size" or "touch dimensions" might be
better terminology to use here and elsewhere.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:36:
EXPECT_NEAR(expected_point.x(), arg_point.x(), kEpsilon);
Why do you need both the EXPECT and the if()?

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:55:
EXPECT_NEAR(expected_point.radius_x(), arg_point.radius_x(), kEpsilon);
Ditto

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:354: // Verify that
up-scaling with claming works for x,y coordinates and radii all
clamping

[Rintaro Kuroiwa, 2015-02-17 19:52:12]

ptal

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/pepper_input_handler.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/pepper_input_handler.cc:41: switch
(pp_touch_event.GetType()) {
On 2015/02/13 16:51:46, Wez wrote:
> nit: Move this switch into a helper function? You'd need to have the
NOTREACHED
> case return some placeholder value for MakeTouchEvent to catch.

I added a function that sets the event type instead of return the type, that
eliminates the uninitialized |type| variable.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:62: // does not look too big or too
small on the output desktop, compared to the
On 2015/02/13 16:51:46, Wez wrote:
> nit: The normal impact of this will not be on the visual appearance of the
> touch, I think, but on the hit-test area within which it'll affect widgets.

Done.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:67: // Also reduce the radii if the
ellipse completely encloses the desktop.
On 2015/02/13 16:51:46, Wez wrote:
> Based on the Windows case, I'd suggest choosing between clamping here and
> deferring to the individual injectors - I'd suggest clamping here until/unless
> we find some platform has a use for touches that exceed the desktop bounds.

I changed the TODO so that we can iterate on this.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.h (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.h:19: // Also resizes the touch radii.
On 2015/02/13 16:51:46, Wez wrote:
> nit: "touch radii" reads oddly; "touch size" or "touch dimensions" might be
> better terminology to use here and elsewhere.

Done.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:36:
EXPECT_NEAR(expected_point.x(), arg_point.x(), kEpsilon);
On 2015/02/13 16:51:46, Wez wrote:
> Why do you need both the EXPECT and the if()?

These are mainly for debugging. The error messages for match failures aren't
that useful when it fails in the middle of a matcher. I can remove the EXPECTs

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:55:
EXPECT_NEAR(expected_point.radius_x(), arg_point.radius_x(), kEpsilon);
On 2015/02/13 16:51:46, Wez wrote:
> Ditto

Done.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:354: // Verify that
up-scaling with claming works for x,y coordinates and radii all
On 2015/02/13 16:51:46, Wez wrote:
> clamping

Done.

[Wez, 2015-02-23 21:59:22]

Rintaro, I'd suggest restructuring the CL description a little - have some draft
text below. Up to you. :)

--
Add touch events to the protocol, the stub layer, and to the client plugin.

- Add TouchEvent and TouchEventPoint messages to the input event protocol.
- Extend the InputStub interface with an InjectTouchEvent() method.
- Update InputEventTracker to track touch points, and to cancel them
  when ReleaseAll() is called.
- Implement touch handling in the client plugin, but off by default.
  The web-app may choose to enable touch handling if desired.
- Add a TouchInputScaler that scales and clamps touch points, similarly to
  the MouseInputFilter, for use by both client and host.

BUG=314515
--

[Wez, 2015-02-23 23:24:08]

LGTM w/ nits!

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:67: // Also reduce the radii if the
ellipse completely encloses the desktop.
On 2015/02/17 19:52:11, Rintaro Kuroiwa wrote:
> On 2015/02/13 16:51:46, Wez wrote:
> > Based on the Windows case, I'd suggest choosing between clamping here and
> > deferring to the individual injectors - I'd suggest clamping here
until/unless
> > we find some platform has a use for touches that exceed the desktop bounds.
> 
> I changed the TODO so that we can iterate on this.

Note that we usually file bugs for improvements of this sort, and refer to them
in the TODO comment.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:36:
EXPECT_NEAR(expected_point.x(), arg_point.x(), kEpsilon);
On 2015/02/17 19:52:12, Rintaro Kuroiwa wrote:
> On 2015/02/13 16:51:46, Wez wrote:
> > Why do you need both the EXPECT and the if()?
> 
> These are mainly for debugging. The error messages for match failures aren't
> that useful when it fails in the middle of a matcher. I can remove the EXPECTs

Can you specify your own logging output for the failure cases to work-around
that?

https://codereview.chromium.org/799233004/diff/260001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/260001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:61: // sizes do not match, the
touch size should be scaled for more predicatable
typo: predictable.

It's not clear what it means for the hit-testing to be more "predictable". The
issue here is that if we up-scaled 2x then relative to the host a touch is
correspondingly only 0.5x it's client size, in effect.

[Rintaro Kuroiwa, 2015-02-24 23:43:10]

Done! Thanks!

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:67: // Also reduce the radii if the
ellipse completely encloses the desktop.
On 2015/02/23 23:24:07, Wez wrote:
> On 2015/02/17 19:52:11, Rintaro Kuroiwa wrote:
> > On 2015/02/13 16:51:46, Wez wrote:
> > > Based on the Windows case, I'd suggest choosing between clamping here and
> > > deferring to the individual injectors - I'd suggest clamping here
> until/unless
> > > we find some platform has a use for touches that exceed the desktop
bounds.
> > 
> > I changed the TODO so that we can iterate on this.
> 
> Note that we usually file bugs for improvements of this sort, and refer to
them
> in the TODO comment.

Done. Added a link to this todo.

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler_unittest.cc (right):

https://codereview.chromium.org/799233004/diff/240001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler_unittest.cc:36:
EXPECT_NEAR(expected_point.x(), arg_point.x(), kEpsilon);
On 2015/02/23 23:24:07, Wez wrote:
> On 2015/02/17 19:52:12, Rintaro Kuroiwa wrote:
> > On 2015/02/13 16:51:46, Wez wrote:
> > > Why do you need both the EXPECT and the if()?
> > 
> > These are mainly for debugging. The error messages for match failures aren't
> > that useful when it fails in the middle of a matcher. I can remove the
EXPECTs
> 
> Can you specify your own logging output for the failure cases to work-around
> that?

Done. Added logging functions.

https://codereview.chromium.org/799233004/diff/260001/remoting/client/plugin/...
File remoting/client/plugin/touch_input_scaler.cc (right):

https://codereview.chromium.org/799233004/diff/260001/remoting/client/plugin/...
remoting/client/plugin/touch_input_scaler.cc:61: // sizes do not match, the
touch size should be scaled for more predicatable
On 2015/02/23 23:24:07, Wez wrote:
> typo: predictable.
> 
> It's not clear what it means for the hit-testing to be more "predictable". The
> issue here is that if we up-scaled 2x then relative to the host a touch is
> correspondingly only 0.5x it's client size, in effect.

Changed description and an example where scaling is needed.

[Rintaro Kuroiwa, 2015-02-26 19:29:40]

The CQ bit was checked by rkuroiwa@chromium.org

[Rintaro Kuroiwa, 2015-02-26 19:29:41]

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/799233004/#ps320001
(title: "rebased")

[commit-bot: I haz the power, 2015-02-26 19:30:05]

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

[commit-bot: I haz the power, 2015-02-26 19:42:05]

Committed patchset #16 (id:320001)

[commit-bot: I haz the power, 2015-02-26 19:42:37]

Patchset 16 (id:??) landed as
https://crrev.com/0e68803fd9d34a74646ed1ecc510469c0a98161b
Cr-Commit-Position: refs/heads/master@{#318284}