Chromium side of maxTouchPoints implementation.

Chromium side of maxTouchPoints implementation.

Intent to implement-and-ship: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/maxTouchPoints/blink-dev/ayzxdztUlOQ/rd-z_Jo3ocIJ

navigator.maxTouchPoints is defined in the W3C Pointer Events
standard draft:
http://www.w3.org/TR/pointerevents/#widl-Navigator-maxTouchPoints

Depends on blink revision 160153:
https://src.chromium.org/viewvc/blink?view=revision&revision=160153

BUG=248918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231203

[Rick Byers, 2013-10-10 22:42:40]

https://codereview.chromium.org/26764002/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/26764002/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:604:
prefs.pointer_events_max_touch_points_enabled = prefs.touch_enabled &&
I don't think you want to look at touch_enabled here.  If touch isn't enabled,
then the API should return 0 not undefined, right?  At least I assume that's
what IE does.

https://codereview.chromium.org/26764002/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:604:
prefs.pointer_events_max_touch_points_enabled = prefs.touch_enabled &&
Does this override the status=experimental property in
RuntimeEnabledFeatures.in?  I.e. does this cause the API to be exposed even when
we're not running with --enable-experimental-web-platform-features?  I don't
think we want that - let's get it right as experimental, then launch it to all
users by flipping it to stable in a future CL.

If we're going to land support for all platforms before it goes stable (which I
think we can do), then you can just avoid having a pref for this entirely. 
We'll just flip it to stable once all the platforms that support touch supply
some value and then have no need to disable it in some cases.

https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device.cc
File ui/base/touch/touch_device.cc (right):

https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device.cc...
ui/base/touch/touch_device.cc:20: return -1;
I think you can safely return 0 here (and true above).  Touch isn't supported,
so we know the answer.

https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_an...
File ui/base/touch/touch_device_android.cc (right):

https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_an...
ui/base/touch/touch_device_android.cc:25: // find one that's supported and
return its value.
Oh, I'm surprised!  That sucks.

Let's leave the discussion for how best to deal with Android in a separate CL.

https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_oz...
File ui/base/touch/touch_device_ozone.cc (right):

https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_oz...
ui/base/touch/touch_device_ozone.cc:15: return false;
Hmm, I forgot about Ozone.  I'll ask Sadrul if he can easily implement this too
(or perhaps if we're going to lie above, we can just lie here too).

[Scott Blomquist (MS OPEN TECH), 2013-10-17 20:48:35]

On 2013/10/10 22:42:40, Rick Byers wrote:
>
https://codereview.chromium.org/26764002/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_impl.cc:604:
> prefs.pointer_events_max_touch_points_enabled = prefs.touch_enabled &&
> I don't think you want to look at touch_enabled here.  If touch isn't enabled,
> then the API should return 0 not undefined, right?  At least I assume that's
> what IE does.

Fixed.

>
https://codereview.chromium.org/26764002/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_impl.cc:604:
> prefs.pointer_events_max_touch_points_enabled = prefs.touch_enabled &&
> Does this override the status=experimental property in
> RuntimeEnabledFeatures.in?  I.e. does this cause the API to be exposed even
when
> we're not running with --enable-experimental-web-platform-features?  I don't
> think we want that - let's get it right as experimental, then launch it to all
> users by flipping it to stable in a future CL.

It does override that. I've removed this logic.

> If we're going to land support for all platforms before it goes stable (which
I
> think we can do), then you can just avoid having a pref for this entirely. 
> We'll just flip it to stable once all the platforms that support touch supply
> some value and then have no need to disable it in some cases.

Sounds good to me.

> https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device.cc
> File ui/base/touch/touch_device.cc (right):
> 
>
https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device.cc...
> ui/base/touch/touch_device.cc:20: return -1;
> I think you can safely return 0 here (and true above).  Touch isn't supported,
> so we know the answer.

Fixed.

>
https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_an...
> File ui/base/touch/touch_device_android.cc (right):
> 
>
https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_an...
> ui/base/touch/touch_device_android.cc:25: // find one that's supported and
> return its value.
> Oh, I'm surprised!  That sucks.
> 
> Let's leave the discussion for how best to deal with Android in a separate CL.

Punted. For now, Android returns kMaxTouchPointsUnknown (as does everything
except default (0) and Windows (actual value)).

>
https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_oz...
> File ui/base/touch/touch_device_ozone.cc (right):
> 
>
https://codereview.chromium.org/26764002/diff/1/ui/base/touch/touch_device_oz...
> ui/base/touch/touch_device_ozone.cc:15: return false;
> Hmm, I forgot about Ozone.  I'll ask Sadrul if he can easily implement this
too
> (or perhaps if we're going to lie above, we can just lie here too).

Returning kMaxTouchPointsUnknown for now.

[Rick Byers, 2013-10-18 21:12:38]

This LGTM (with a small nit).

You'll need to wait for the blink side to land and roll into the chromium tree
before you put this in the CQ.  Also, once it lands it's a good idea to add a
link to the blink revision in the CL description (since this change won't
compile without that one).

https://codereview.chromium.org/26764002/diff/16001/ui/base/touch/touch_device.h
File ui/base/touch/touch_device.h (right):

https://codereview.chromium.org/26764002/diff/16001/ui/base/touch/touch_devic...
ui/base/touch/touch_device.h:10: const int kMaxTouchPointsUnknown = -1;
This is just temporary until we have each platform implemented, right?  Since -1
isn't specified as valid by the standard, we wouldn't want to actually return
this outside of status=experimental.

Nit: add a TODO to remove this referencing crbug.com/308649.

[Rick Byers, 2013-10-18 21:13:48]

+sadrul for OWNERS in ui/base/touch

[sadrul, 2013-10-21 02:27:21]

Looks good. A couple of nits:

https://codereview.chromium.org/26764002/diff/50001/ui/base/touch/touch_device.h
File ui/base/touch/touch_device.h (right):

https://codereview.chromium.org/26764002/diff/50001/ui/base/touch/touch_devic...
ui/base/touch/touch_device.h:13: const int kMaxTouchPointsUnknown = -1;
This should be inside the ui namespace.

https://codereview.chromium.org/26764002/diff/50001/ui/base/touch/touch_devic...
ui/base/touch/touch_device.h:20: UI_EXPORT int MaxTouchPoints();
Add a doc comment.

There can be multiple touch devices in use at the same time (e.g. an external
touch-screen attached to a pixel device). Does this function add up the
max-touch points for each touch-device? Please include this in the doc comment.

[Scott Blomquist (MS OPEN TECH), 2013-10-22 18:27:41]

Thanks for the feedback. I've incorporated all of it unless "doc comment" means
something more specific than just a comment that describes what the function
should do.

Let me know if we're good. Thanks.

On 2013/10/21 02:27:21, sadrul wrote:
>
https://codereview.chromium.org/26764002/diff/50001/ui/base/touch/touch_devic...
> ui/base/touch/touch_device.h:13: const int kMaxTouchPointsUnknown = -1;
> This should be inside the ui namespace.

Fixed.

>
https://codereview.chromium.org/26764002/diff/50001/ui/base/touch/touch_devic...
> ui/base/touch/touch_device.h:20: UI_EXPORT int MaxTouchPoints();
> Add a doc comment.

Done.

[sadrul, 2013-10-23 02:46:21]

ui/base/touch/ LGTM

darin@ could an owner-reviewer for the webkit/ content/ changes.

[darin (slow to review), 2013-10-25 20:00:55]

LGTM

https://codereview.chromium.org/26764002/diff/100001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/26764002/diff/100001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:610:
prefs.touch_adjustment_enabled =
could you by chance also fix the indentation of this neighboring code?

[commit-bot: I haz the power, 2013-10-25 21:46:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sblom@microsoft.com/26764002/210001

[commit-bot: I haz the power, 2013-10-26 04:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sblom@microsoft.com/26764002/210001

[commit-bot: I haz the power, 2013-10-26 09:41:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sblom@microsoft.com/26764002/210001

[commit-bot: I haz the power, 2013-10-26 12:53:43]

Change committed as 231203