Initial working patch. This includes the entire contents of the patch in bug 85921 - I'll wait until that lands and resolve before requesting review of this path.
With this patch, if we're on platform that sets the TouchScreenDevice setting (currently Chromium for Windows and ChromeOS), then we can return true for the (pointer) (pointer:coarse) and (hover:0) queries. Otherwise we behave exactly as if the media feature isn't supported at all. Eg., I don't want to report anything for pointer:none or pointer:fine when we don't know whether or not there's a mouse attached. I'll handle the mouse case for chromium in a follow-up CL.

By the way, you can just upload the diff after some other patch if you like. It's fine to upload patches that don't apply to trunk or that don't compile. That's better than having a bunch of unrelated changes in your patch, which make it hard to review

Comment on attachment 143944[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=143944&action=review>> Source/WebCore/css/MediaQueryEvaluator.cpp:542
>> +static PointerDeviceType getLeastCapablePrimaryPointerDeviceType(Frame* frame)
>
> We generally dislike using "get" as a verb because it's such a weak verb. Perhaps "leastCapablePrimaryPointerDeviceType" ?
Done
>> Source/WebCore/css/MediaQueryEvaluator.cpp:547
>> + // FIXME: Should also try to determine if we know we have a mouse.
>
> Please use complete sentences in comments. "We should ..."
Done
>> Source/WebCore/css/MediaQueryEvaluator.cpp:557
>> +static bool hoverMediaFeatureEval(CSSValue *value, RenderStyle*, Frame* frame, MediaFeaturePrefix)
>
> CSSValue *value => CSSValue* value
Done
>> Source/WebCore/css/MediaQueryEvaluator.cpp:559
>> + PointerDeviceType p = getLeastCapablePrimaryPointerDeviceType(frame);
>
> p => type (Please use words for variable names, not letters.)
Done
>> Source/WebCore/css/MediaQueryEvaluator.cpp:581
>> + return p != NoPointer;
>
> Four-space indent.
Done.
>> Source/WebCore/css/MediaQueryEvaluator.cpp:590
>> + }
>
> Bad indent.
Done
>> LayoutTests/fast/media/mq-pointer-touchscreen.html:7
>> + }
>
> No need for { }
Done
>> LayoutTests/fast/media/mq-pointer-touchscreen.html:41
>> + <p id="g">This text should be green if the least capable primary pointer supports hover.</p>
>
> Can we use getComputedStyle or something so we can use a text test rather than a render tree dump? Render tree dumps are annoying because they're different on every platform.
Sure, I'll start working on that now. I debated between the two approaches. I'd like to avoid having one test file per hardware configuration (since there will be several more), and the resulting duplication across the files. So once I make this a JS test, I'll also refactor to have one test for "pointer" and one test for "hover" - which each test checking all hardware configurations.
I thought it was nicer for CSS tests to be more declarative like this (at least a lot of the existing media query tests use this style), but I didn't realize the output would be different on different platforms (presumably the size of text changes?). In general wouldn't it be better to have some modes where DumpRenderTree omits details that are known to vary across platforms and considered irrelevant to the test (in this case all sizes)? The general approach of declarative layout testing seems nice to me.

(In reply to comment #4)
> By the way, you can just upload the diff after some other patch if you like. It's fine to upload patches that don't apply to trunk or that don't compile. That's better than having a bunch of unrelated changes in your patch, which make it hard to review
Thanks. I looked for a baseline option to "webkit-patch upload", but of course I can just create a separate non-compiling git branch. Done.

> I thought it was nicer for CSS tests to be more declarative like this (at least a lot of the existing media query tests use this style), but I didn't realize the output would be different on different platforms (presumably the size of text changes?).
Yeah.
> In general wouldn't it be better to have some modes where DumpRenderTree omits details that are known to vary across platforms and considered irrelevant to the test (in this case all sizes)? The general approach of declarative layout testing seems nice to me.
Yes, we'd like to do that, but it's somewhat tricky and no one has done it yet. We might even be able to get pixel perfect in most cases, at least for the Chromium port, by using mock scroll bars and using some repeatable Skia text rendering.(In reply to comment #8)
> Thanks. I looked for a baseline option to "webkit-patch upload", but of course I can just create a separate non-compiling git branch. Done.
I usually use this option:
abarth@quadzen:~/svn/webkit$ ./Tools/Scripts/webkit-patch help upload
[...]
-g GIT_COMMIT, --git-commit=GIT_COMMIT
Operate on a local commit. If a range, the commits are
squashed into one. <ref>.... includes the working copy
changes. UPSTREAM can be used for the
upstream/tracking branch.

I wonder if we should have an ENABLE(TOUCH_MEDIA_QUERIES) macro for this patch. It seems like other ports won't want to ship this code if they haven't wired up the settings property.
Also, we probably should email webkit-dev to let folks know we're implementing this feature: http://www.webkit.org/coding/adding-features.html
I'm also slightly worried that we're doing the wrong thing in the common case. In the common case, we have a mouse pointer, but we're treating that case as "unknown" currently. If folks write content using this feature, that means that "unknown" will de facto mean "mouse". It seems better to wire in a setting for whether the device supports mice.

(In reply to comment #12)
> I wonder if we should have an ENABLE(TOUCH_MEDIA_QUERIES) macro for this patch. It seems like other ports won't want to ship this code if they haven't wired up the settings property.
I was hoping this wouldn't be necessary since I've been careful to make the PointerUnknown case behave identically to the case where the media query isn't supported at all. Let me know if you think it's required. Do I then need to request a bot be setup that runs with this feature enabled?
> Also, we probably should email webkit-dev to let folks know we're implementing this feature: http://www.webkit.org/coding/adding-features.html
Sure, I'm happy to do that. Let's answer these other two questions firs though.
> I'm also slightly worried that we're doing the wrong thing in the common case. In the common case, we have a mouse pointer, but we're treating that case as "unknown" currently. If folks write content using this feature, that means that "unknown" will de facto mean "mouse". It seems better to wire in a setting for whether the device supports mice.
Note that the "unknown" case is identical to the "feature not supported" case. Having to handle the not supported case is something sites will need to do anyway to support browsers that don't yet have this feature. Since sites will (for some time anyway) always need some fallback decision in their app, it seems better to me to leave that decision to the app then to try to apply some heuristic behavior ourselves (like encouraging ports to assume there is a mouse when we're not sure).
I agree though that I'm not 100% positive this is the right approach (this is what I was describing in my original e-mail to you). I can't think of a concrete scenario where it might be a problem though, any suggestions?

(In reply to comment #13)
> (In reply to comment #12)
> > I'm also slightly worried that we're doing the wrong thing in the common case. In the common case, we have a mouse pointer, but we're treating that case as "unknown" currently. If folks write content using this feature, that means that "unknown" will de facto mean "mouse". It seems better to wire in a setting for whether the device supports mice.
>
> Note that the "unknown" case is identical to the "feature not supported" case. Having to handle the not supported case is something sites will need to do anyway to support browsers that don't yet have this feature. Since sites will (for some time anyway) always need some fallback decision in their app, it seems better to me to leave that decision to the app then to try to apply some heuristic behavior ourselves (like encouraging ports to assume there is a mouse when we're not sure).
>
> I agree though that I'm not 100% positive this is the right approach (this is what I was describing in my original e-mail to you). I can't think of a concrete scenario where it might be a problem though, any suggestions?
Also, just to be clear, I intend to implement the mouse case for chromium. It's just not as urgent as touch for me right now (I really need pointer:coarse in Chrome 21). I can split the mouse piece out to a separate bug if you like.
Note that "unknown" (i.e. both (pointer)" and (pointer:none) being false) already de facto means "mouse" - that's the web as it exists today. So I don't think I'm changing anything in that respect. First step is for (pointer:coarse) to mean "definitely a touch screen", then later I'll add (pointer:fine) to mean definitely a mouse but no touch screen, and (pointer:none) to mean definitely no pointer. Sound reasonable?
If you think it's important then I'm willing to wait and do the whole thing in one big CL instead. Let me know. Thanks!
Rick

If you add an ENABLE macro, then the order of implementation is less important because each port can decide whether or not to enable the feature. Currently, the patch doesn't have an ENABLE macro, which means that it will report "unknown" for non-Chromium ports, even if they are using touch devices.

(In reply to comment #15)
> If you add an ENABLE macro, then the order of implementation is less important because each port can decide whether or not to enable the feature. Currently, the patch doesn't have an ENABLE macro, which means that it will report "unknown" for non-Chromium ports, even if they are using touch devices.
What exactly do you mean "report unknown"?
If I add an ENABLE macro, then ports that haven't opted into this will return 'false' for all relevant queries. Without the ENABLE macro, ports that haven't opted into supplying pointer device data return 'false' for all relevant queries - i.e. exactly the same behavior as with the macro. So the two choices we're debating have identical behavior.
That said, if there's other reasons to use an ENABLE macro I'm fine doing that. I just want to make sure I'm clear on the benefits it provides, and importantly what the compat implications of enabling the feature are.

(In reply to comment #19)
> (From update of attachment 144600[details])
> Thanks for clarifying! I see that now in the spreadsheet you gave me. My apologies, I was confused before.
Cool, thanks! I'll make that post to webkit-dev before putting this in the commit queue.

(In reply to comment #11)
> @tabatkins: Should we be using a vendor prefix in this patch?
I spoke with Tab over IM and he said "Honestly, I'd recommend no prefix. This is not going to be controversial."