Media queries: excluded non-pointing devices from pointer/hover

Media queries: excluded non-pointing devices from pointer/hover

The pointer/hover media query spec is ambiguous about the inclusion of
non-pointing input device like keyboards:
http://dev.w3.org/csswg/mediaqueries-4/#mf-interaction

This CL removes the such devices from consideration in Chrome---this
seems more logical since pointer/hover queries make sense only for
pointing devices.

BUG=442795
Committed: https://crrev.com/7a42795e4c7c3449129d4077df0bd8c269c7ed4b
Cr-Commit-Position: refs/heads/master@{#309203}

[mustaq, 2014-12-16 17:34:50]

mustaq@chromium.org changed reviewers:
+ rbyers@chromium.org

[jdduke (slow), 2014-12-16 19:44:50]

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

[jdduke (slow), 2014-12-16 19:44:51]

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

https://codereview.chromium.org/806693008/diff/1/ui/base/touch/touch_device_a...
ui/base/touch/touch_device_android.cc:37: DCHECK(available_pointer_types ==
POINTER_TYPE_NONE);
DCHECK_EQ?

[mustaq, 2014-12-16 20:34:02]

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

https://codereview.chromium.org/806693008/diff/1/ui/base/touch/touch_device_a...
ui/base/touch/touch_device_android.cc:37: DCHECK(available_pointer_types ==
POINTER_TYPE_NONE);
On 2014/12/16 19:44:51, jdduke wrote:
> DCHECK_EQ?

Done.

[Rick Byers, 2014-12-16 21:23:09]

lgtm

[Rick Byers, 2014-12-16 21:24:32]

On 2014/12/16 21:23:09, Rick Byers wrote:
> lgtm

It's worth noting that we'll get this clarified in the spec - won't consider the
bug 'fixed' until our implementation clearly matches the spec here.

[mustaq, 2014-12-16 21:28:02]

mustaq@chromium.org changed reviewers:
+ sadrul@chromium.org

[mustaq, 2014-12-16 21:28:03]

sadrul@chromium.org: Need approval for ui/base/touch/touch_device*

[sadrul, 2014-12-16 22:23:37]

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_aurax11.cc (right):

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:28: available_pointer_types |=
POINTER_TYPE_FINE;
We can change these to assignments now I think?

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:52: available_hover_types |=
HOVER_TYPE_HOVER;
ditto

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_ozone.cc (right):

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_ozone.cc:21: return POINTER_TYPE_COARSE;
Is there a reason any of the ozone implementations should be different from the
aurax11 implementation? (apart from IsTouchDevicePresent(), which could probably
be implemented to use DeviceDataManager::touchscreen_devices() for both, instead
of looking at TouchFactory for x11)

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_win.cc (right):

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_win.cc:34: available_pointer_types =
POINTER_TYPE_NONE;
Should POINTER_TYPE_NONE be 0, in which case, we don't need this block of code.

[mustaq, 2014-12-17 15:08:38]

Patchset #3 (id:40001) has been deleted

[mustaq, 2014-12-17 15:16:05]

ptal

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_aurax11.cc (right):

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:28: available_pointer_types |=
POINTER_TYPE_FINE;
On 2014/12/16 22:23:37, sadrul wrote:
> We can change these to assignments now I think?

Done.

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:52: available_hover_types |=
HOVER_TYPE_HOVER;
On 2014/12/16 22:23:37, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_ozone.cc (right):

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_ozone.cc:21: return POINTER_TYPE_COARSE;
On 2014/12/16 22:23:37, sadrul wrote:
> Is there a reason any of the ozone implementations should be different from
the
> aurax11 implementation? (apart from IsTouchDevicePresent(), which could
probably
> be implemented to use DeviceDataManager::touchscreen_devices() for both,
instead
> of looking at TouchFactory for x11)

No strong reason, I did minimal here because this file was a stub.

Done doing the exact thing as aurax11. Also updated IsTouchDevicePresent in both
places since you seemed to suggest DeviceDataManager is a better way.

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_win.cc (right):

https://codereview.chromium.org/806693008/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_win.cc:34: available_pointer_types =
POINTER_TYPE_NONE;
On 2014/12/16 22:23:37, sadrul wrote:
> Should POINTER_TYPE_NONE be 0, in which case, we don't need this block of
code.

That's the end goal, but not something we will attempt before M41 branch date.
We will do it after the spec change (see the bug). We will have to refactor
pointer/hover type enums everywhere, such enums are defined in three (or four?)
places in chromium+blink, needs careful commit sequence across the repo
boundary.

[Rick Byers, 2014-12-17 15:28:46]

Since we're talking about making Ozone match AuraX11 defaults, I figure it's
also worth debating the correct AuraX11 defaults here...

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_aurax11.cc (right):

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:23: int available_pointer_types = 0;
Our default for AuraX11 should always include POINTER_TYPE_FINE (until we have
the ability to explicitly detect a mouse).  Aura is used almost exclusively on
laptops.

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:39: if (available_pointer_types &
POINTER_TYPE_FINE)
Again, since Aura is used primarily on laptops, I think the default primary
pointer type should be 'fine'.

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:47: int available_hover_types = 0;
ditto

[mustaq, 2014-12-17 17:47:49]

ptal

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_aurax11.cc (right):

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:23: int available_pointer_types = 0;
On 2014/12/17 15:28:46, Rick Byers wrote:
> Our default for AuraX11 should always include POINTER_TYPE_FINE (until we have
> the ability to explicitly detect a mouse).  Aura is used almost exclusively on
> laptops.

Done.

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:39: if (available_pointer_types &
POINTER_TYPE_FINE)
On 2014/12/17 15:28:46, Rick Byers wrote:
> Again, since Aura is used primarily on laptops, I think the default primary
> pointer type should be 'fine'.

Done.

https://codereview.chromium.org/806693008/diff/60001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_aurax11.cc:47: int available_hover_types = 0;
On 2014/12/17 15:28:46, Rick Byers wrote:
> ditto

Done.

[sadrul, 2014-12-17 20:08:42]

Should the code for ozone and aurax11 be moved into a single file (e.g.
touch_device_linux.cc) so we don't have duplicate code?

https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_ozone.cc (right):

https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_ozone.cc:22: // FIXME: Use mouse detection logic.
crbug.com/440503
TODO

[mustaq, 2014-12-18 15:07:32]

PTAL

We are rushing to push the MQ-related changes by this week because I will
unavailable until an 12 (incl) which is after M41 branch date.

I believe merging touch_device_aurax11 and touch_device_ozone is beyond the
scope of this CL because of our time constraint as well as being unrelated to
MQs.

https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_ozone.cc (right):

https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_ozone.cc:22: // FIXME: Use mouse detection logic.
crbug.com/440503
On 2014/12/17 20:08:42, sadrul wrote:
> TODO

Chromium coding style seems silent about FIXME vs TODO, so I am following the
Blink style here (FIXME because there's no owner). Moreover, code-search failed
to convince me that there is a standard style. Please let me know if I am
missing something.

[Rick Byers, 2014-12-18 17:04:39]

On 2014/12/18 15:07:32, mustaq wrote:
> PTAL
> 
> We are rushing to push the MQ-related changes by this week because I will
> unavailable until an 12 (incl) which is after M41 branch date.
> 
> I believe merging touch_device_aurax11 and touch_device_ozone is beyond the
> scope of this CL because of our time constraint as well as being unrelated to
> MQs.
> 
>
https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
> File ui/base/touch/touch_device_ozone.cc (right):
> 
>
https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
> ui/base/touch/touch_device_ozone.cc:22: // FIXME: Use mouse detection logic.
> crbug.com/440503
> On 2014/12/17 20:08:42, sadrul wrote:
> > TODO
> 
> Chromium coding style seems silent about FIXME vs TODO, so I am following the
> Blink style here (FIXME because there's no owner). Moreover, code-search
failed
> to convince me that there is a standard style. Please let me know if I am
> missing something.

Chromium is a superset of Google coding style, so TODO comments are covered
here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments. 
The convention is indeed FIXME in blink and TODO in chromium - sigh.  Hopefully
we can slowly move blink to match the chromium style (I'm forever making style
mistakes in my chromium CLs due to this).

[mustaq, 2014-12-18 18:45:42]

On 2014/12/18 17:04:39, Rick Byers wrote:
> On 2014/12/18 15:07:32, mustaq wrote:
> > PTAL
> > 
> > We are rushing to push the MQ-related changes by this week because I will
> > unavailable until an 12 (incl) which is after M41 branch date.
> > 
> > I believe merging touch_device_aurax11 and touch_device_ozone is beyond the
> > scope of this CL because of our time constraint as well as being unrelated
to
> > MQs.
> > 
> >
>
https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
> > File ui/base/touch/touch_device_ozone.cc (right):
> > 
> >
>
https://codereview.chromium.org/806693008/diff/80001/ui/base/touch/touch_devi...
> > ui/base/touch/touch_device_ozone.cc:22: // FIXME: Use mouse detection logic.
> > crbug.com/440503
> > On 2014/12/17 20:08:42, sadrul wrote:
> > > TODO
> > 
> > Chromium coding style seems silent about FIXME vs TODO, so I am following
the
> > Blink style here (FIXME because there's no owner). Moreover, code-search
> failed
> > to convince me that there is a standard style. Please let me know if I am
> > missing something.
> 
> Chromium is a superset of Google coding style, so TODO comments are covered
> here:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments.

> The convention is indeed FIXME in blink and TODO in chromium - sigh. 
Hopefully
> we can slowly move blink to match the chromium style (I'm forever making style
> mistakes in my chromium CLs due to this).

Sorry for missing this! Done. PTAL.

[Rick Byers, 2014-12-18 19:30:41]

Thanks, this still lgtm

[sadrul, 2014-12-18 22:10:39]

LGTM

With ozone being shipped in some chromeos devices in 41, we should unify the
aura-x11 and ozone implementations soon to make sure the two impls don't end up
diverging.

[mustaq, 2014-12-19 14:33:43]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2014-12-19 14:34:23]

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

[commit-bot: I haz the power, 2014-12-19 15:35:55]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-12-19 15:36:47]

Patchset 5 (id:??) landed as
https://crrev.com/7a42795e4c7c3449129d4077df0bd8c269c7ed4b
Cr-Commit-Position: refs/heads/master@{#309203}