Updated hover media feature to comply updated media queries spec

Updated hover media feature to comply updated media queries spec

The spec (http://dev.w3.org/csswg/mediaqueries-4/#hover)  for the hover
media query has added keywords for the feature's value. This CL updates
the media query keywords to propertly parse and evaluate the feature;
however, it remains only sparsely implemented. The hover (and pointer)
media queries currently only produce non-false output for touchscreen
devices. The plumbing work to actually produce the correct results in
all cases will be done in a future CL.

BUG=136119
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180006

[bokan, 2014-08-08 20:42:16]

Hi Adam, PTAL when you have a chance.

This is a web facing change but this feature went out before the Blink
feature process was developed and it's been half-baked since. The spec
has been updated to change the 0/1 values to keywords. In implementing
the any-pointer, any-hover features
(http://www.chromestatus.com/feature/6460705494532096) I think it makes
sense to land this change first.

I checked the CSS use counter but it looks like we don't track media
queries so I have no way to get approximate usage (correct me if I'm
wrong). I would expect few/no users since it's only half implemented
in Chrome (doesn't report non-touchscreen case) and not implemented
at all in other browsers (I tested Safari OSX and Firefox). Since this
is a (small) change rather than a new feature I figure putting
this behind a flag would be awkward, however, I submit to your guidance here.

Thanks.

[abarth-chromium, 2014-08-08 21:41:20]

I'm probably not the best reviewer for this CL...  Maybe yoav would be able to
review?

[Yoav Weiss, 2014-08-08 22:01:16]

On 2014/08/08 20:42:16, bokan wrote:
> Hi Adam, PTAL when you have a chance.
> 
> This is a web facing change but this feature went out before the Blink
> feature process was developed and it's been half-baked since. The spec
> has been updated to change the 0/1 values to keywords. In implementing
> the any-pointer, any-hover features
> (http://www.chromestatus.com/feature/6460705494532096) I think it makes
> sense to land this change first.
> 
> I checked the CSS use counter but it looks like we don't track media
> queries so I have no way to get approximate usage (correct me if I'm
> wrong). I would expect few/no users since it's only half implemented
> in Chrome (doesn't report non-touchscreen case) and not implemented
> at all in other browsers (I tested Safari OSX

Are you sure it's not working in Safari iOS? The code seems to be in WebKit,
with the same brokenness Blink has, so both pointer & hover MQs return false for
devices with no touch support, but may respond with relevant values to devices
with touch support.
It's possible that they never merged it to the Safari branch, but it's worth
making sure.

> and Firefox). Since this
> is a (small) change rather than a new feature I figure putting
> this behind a flag would be awkward, however, I submit to your guidance here.
> 
> Thanks.

As far as the implementation goes, LGTM.
You probably want to fix MediaQueryEvaluatorTest.cpp though, to get rid of the
old hover tests, and add new ones.

Regarding the decision whether we can remove hover 1/0 MQs support, I defer that
to the API owners, so please do not commit this before getting their approval.
Maybe send out an "intent to remove/change" to blink-dev? Personally, I'd be
wary to modify a Web facing feature without any usage data.

[bokan, 2014-08-08 22:30:23]

On 2014/08/08 22:01:16, Yoav Weiss wrote:
> Are you sure it's not working in Safari iOS? The code seems to be in WebKit,
> with the same brokenness Blink has, so both pointer & hover MQs return false
for
> devices with no touch support, but may respond with relevant values to devices
> with touch support.
> It's possible that they never merged it to the Safari branch, but it's worth
> making sure.

Good point, I tested on OSX so there's no touchscreen. I just tried it on Safari
mobile and it's not implemented either. The way it's currently hooked up is the
embedder needs to set the "deviceSupportsTouch" appropriately, seems they do
not.

> As far as the implementation goes, LGTM.
> You probably want to fix MediaQueryEvaluatorTest.cpp though, to get rid of the
> old hover tests, and add new ones.

Done

> Regarding the decision whether we can remove hover 1/0 MQs support, I defer
that
> to the API owners, so please do not commit this before getting their approval.
> Maybe send out an "intent to remove/change" to blink-dev? Personally, I'd be
> wary to modify a Web facing feature without any usage data.

Will do

[bokan, 2014-08-11 18:05:59]

Yoav, see discussion at
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/gVAJjaNzDNM

I've put the change behind a RuntimeEnabledFeature so it can be easily rolled
back if needed. Still LGTM?

[Yoav Weiss, 2014-08-11 18:40:45]

On 2014/08/11 18:05:59, bokan wrote:
> Yoav, see discussion at
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/gVAJjaNzDNM
> 
> I've put the change behind a RuntimeEnabledFeature so it can be easily rolled
> back if needed. Still LGTM?

Yup. CQ away :)

[bokan, 2014-08-11 18:58:33]

On 2014/08/11 18:40:45, Yoav Weiss wrote:
> On 2014/08/11 18:05:59, bokan wrote:
> > Yoav, see discussion at
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/gVAJjaNzDNM
> > 
> > I've put the change behind a RuntimeEnabledFeature so it can be easily
rolled
> > back if needed. Still LGTM?
> 
> Yup. CQ away :)

abarth@, could you please stamp for Source/platform?

[abarth-chromium, 2014-08-11 22:25:48]

Source/platform/RuntimeEnabledFeatures.in LGTM

[bokan, 2014-08-11 22:26:24]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-08-11 22:27:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bokan@chromium.org/451113002/40001

[commit-bot: I haz the power, 2014-08-11 23:27:15]

Change committed as 180006