Issue 991643002: Windows Host Touch Injection

Issue 991643002: Windows Host Touch Injection (Closed)

Created:
5 years, 9 months ago by Rintaro Kuroiwa

Modified:
5 years, 9 months ago

Reviewers:
Wez

CC:
chromium-reviews, chromoting-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Windows Host Touch Injection - Update SessionInputInjectorWin and InputInjectorWin to handle touch events. - Add TouchInjectorWin which converts TouchEvents to structs for Windows touch injection API. BUG=314515 Committed: https://crrev.com/883da7ed21c90439fbb6683626efe06409dde176 Cr-Commit-Position: refs/heads/master@{#322116}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : unit tests #

Patch Set 4 : clean up #

Patch Set 5 : more clean up #

Patch Set 6 : #

Patch Set 7 : Add Deinitialize() and change test #

Total comments: 40

Patch Set 8 : address comments #

Patch Set 9 : fix comments and check pressure clamping in unit test #

Total comments: 16

Patch Set 10 : changes from review #

Patch Set 11 : rebased #

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+937 lines, -2 lines)			Patch
M	remoting/host/input_injector_win.cc	View	1 2 3 4 5 6	8 chunks	+21 lines, -1 line	0 comments	Download
A	remoting/host/touch_injector_win.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+106 lines, -0 lines	0 comments	Download
A	remoting/host/touch_injector_win.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+285 lines, -0 lines	0 comments	Download
A	remoting/host/touch_injector_win_unittest.cc	View	1 2 3 4 5 6 7 8	1 chunk	+514 lines, -0 lines	0 comments	Download
M	remoting/host/win/session_input_injector.cc	View	1 2 3 4	1 chunk	+8 lines, -1 line	0 comments	Download
M	remoting/remoting_host_srcs.gypi	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	remoting/remoting_test.gypi	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 11 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Wez

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_injector_win.cc File remoting/host/touch_injector_win.cc (right): https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_injector_win.cc#newcode21 remoting/host/touch_injector_win.cc:21: const uint32_t kMaxSimlutaneousTouchCount = 10; typo: Simultaneous https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_injector_win.cc#newcode24 remoting/host/touch_injector_win.cc:24: ...

5 years, 9 months ago (2015-03-17 03:06:24 UTC) #3

Rintaro Kuroiwa

ptal https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_injector_win.cc File remoting/host/touch_injector_win.cc (right): https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_injector_win.cc#newcode21 remoting/host/touch_injector_win.cc:21: const uint32_t kMaxSimlutaneousTouchCount = 10; On 2015/03/17 03:06:23, ...

5 years, 9 months ago (2015-03-18 01:33:31 UTC) #4

ptal

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
File remoting/host/touch_injector_win.cc (right):

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:21: const uint32_t
kMaxSimlutaneousTouchCount = 10;
On 2015/03/17 03:06:23, Wez wrote:
> typo: Simultaneous

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:24: // into |output_vector|.
On 2015/03/17 03:06:23, Wez wrote:
> Why are you changing them in the map?
> 
> Isn't the point here just to pre-populate the vector<> with MOVE info for each
> of the currently-down touch points?
> 
> It feels like you should be able to have a single function that takes whatever
> is in the touches map and injects it, and then each of of the
> add/move/end/cancel functions just updates the map, e.g. marking some points
as
> cancelled, does the injection, and then does any necessary cleanup, e.g.
> removing cancelled points.  Potentially slightly less efficient, but easier to
> read!

Done.
As discussed offline, the map will always contain POINTER_TOUCH_INFO as "move"
points. So this function is renamed.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:34: DCHECK(output_vector);
On 2015/03/17 03:06:23, Wez wrote:
> You are about to dereference these anyway, below, which will crash if they're
> NULL, so it's not really worthwhile DCHECKing them first here.

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:47: DCHECK(pointer_touch_info);
On 2015/03/17 03:06:23, Wez wrote:
> Same here

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:54: // the default value will set the area
correctly.
On 2015/03/17 03:06:23, Wez wrote:
> Why will the default value set it correctly? That relies on radius_{x,y} being
> zero if un-set, and that the Windows API treats that as "default", right?

Yes it does rely on get on unset values return 0.
According to MSDN if the digitizer does not report the size then rcContact
should be 0 by 0 (size) centered at the touch coordinate.
I'll add a comment here.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:68: const int pressure =
touch_point.pressure() * kMaxTouchPressure;
On 2015/03/17 03:06:23, Wez wrote:
> Is there a danger of this wrapping? Should we instead range-check
> touch_point.pressure() before multiplying?
> 
> Should we bother clamping out-of-range pressures, or just ignore events with
> bogus values in them?

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:79: // POINTER_TOUCH_INFO is memset() to 0.
On 2015/03/17 03:06:23, Wez wrote:
> Is there a reason you don't both memset()ting it?

I was thinking if I memset(0) that's an extra write. Probably not a big
performance problem so I'll memset.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:97: bool TouchInjectFunctionsWin::Init() {
On 2015/03/17 03:06:23, Wez wrote:
> You can use base::NativeLibrary, which wraps all this up in a cleaner way with
> proper library ref-counting.

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:102: inject_touch_input_func_ = nullptr;
On 2015/03/17 03:06:23, Wez wrote:
> Why are you NULLing these? You just checked that they're null, and they're
just
> bare pointers, and the code below can't set one without setting the other.

N/A in the new patch.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:137: DWORD dw_mode) {
On 2015/03/17 03:06:23, Wez wrote:
> Indentation

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.cc:158: PLOG(INFO) << "Failed to initialize
touch injection.";
On 2015/03/17 03:06:23, Wez wrote:
> If you null the delegate_ here then you can check |delegate_| to determine
> whether injection will work, and you don't need an Initialized() checker on
the
> delegate then.

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
File remoting/host/touch_injector_win.h (right):

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:23: // The methods are virtual for mocking.
On 2015/03/17 03:06:24, Wez wrote:
> If you make this a pure interface, with a TouchInjectFunctionsWin
implementation
> inside the cc file, then you can avoid the need for the Init() and
> IsInitialized() methods, I think?

It wasn't necessary to remove the virtual init* functions.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:24: class TouchInjectFunctionsWin {
On 2015/03/17 03:06:24, Wez wrote:
> Suggest TouchInjectorWinDelegate, or TouchInjectorWin::Delegate

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:32: virtual bool Init();
On 2015/03/17 03:06:23, Wez wrote:
> Suggest Initialized()

Moved to private scope.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:35: virtual bool Initialized();
On 2015/03/17 03:06:24, Wez wrote:
> IsInitialized()

Dropped.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:38: virtual BOOL
InitializeTouchInjection(UINT32 max_count, DWORD dw_mode);
On 2015/03/17 03:06:23, Wez wrote:
> It's confusing to have both this and the Initialize method above. Would it
> complicate the TouchInjectorWin impl to have the InjectFunctions be an object
> with a Create() method that either returns nullptr, or a valid object
containing
> the relevant functions?
> 
> The TouchInjectorWin ctor would always Create() the delegate, but tests can
then
> replace it via SetInputInjectFunctionsForTest().

That is a great idea.
Then the capabilities can be checked just by calling Create() method and check
whether it is null.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:52: // This class does not handle bad inputs.
For example, a touch end event
On 2015/03/17 03:06:24, Wez wrote:
> I think you mean that it expects good inputs, and does not sanity-check the
> inputs it receives?

Right. Reworded.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:61: // may silently fail.
On 2015/03/17 03:06:24, Wez wrote:
> I think it's fine to say "Returns false if initialization of input injection
> APIs fails."  It should be pretty obvious that if it's failed, the object
won't
> work.

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:76: void CancelTouchPoints(const
protocol::TouchEvent& event);
On 2015/03/17 03:06:24, Wez wrote:
> Need a brief comment on this block to explain how these handler functions are
> used.

Done.

https://codereview.chromium.org/991643002/diff/120001/remoting/host/touch_inj...
remoting/host/touch_injector_win.h:78: scoped_ptr<TouchInjectFunctionsWin>
delegate_;
On 2015/03/17 03:06:24, Wez wrote:
> nit: Blank line between this and the subsequent comment

Done.

Wez

lgtm https://codereview.chromium.org/991643002/diff/160001/remoting/host/touch_injector_win.cc File remoting/host/touch_injector_win.cc (right): https://codereview.chromium.org/991643002/diff/160001/remoting/host/touch_injector_win.cc#newcode25 remoting/host/touch_injector_win.cc:25: // This is used to reinject all points ...

5 years, 9 months ago (2015-03-21 01:10:09 UTC) #5

Rintaro Kuroiwa

Done. Thanks. https://codereview.chromium.org/991643002/diff/160001/remoting/host/touch_injector_win.cc File remoting/host/touch_injector_win.cc (right): https://codereview.chromium.org/991643002/diff/160001/remoting/host/touch_injector_win.cc#newcode25 remoting/host/touch_injector_win.cc:25: // This is used to reinject all ...

5 years, 9 months ago (2015-03-24 20:34:47 UTC) #6

commit-bot: I haz the power

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

5 years, 9 months ago (2015-03-24 22:41:43 UTC) #9

commit-bot: I haz the power

5 years, 9 months ago (2015-03-25 05:23:12 UTC) #11

Message was sent while issue was closed.

Patchset 11 (id:??) landed as
https://crrev.com/883da7ed21c90439fbb6683626efe06409dde176
Cr-Commit-Position: refs/heads/master@{#322116}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages