Created attachment 143944 [details] Patch

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.

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" ? > 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 ..." > Source/WebCore/css/MediaQueryEvaluator.cpp:557 > +static bool hoverMediaFeatureEval(CSSValue *value, RenderStyle*, Frame* frame, MediaFeaturePrefix) CSSValue *value => CSSValue* value > Source/WebCore/css/MediaQueryEvaluator.cpp:559 > + PointerDeviceType p = getLeastCapablePrimaryPointerDeviceType(frame); p => type (Please use words for variable names, not letters.) > Source/WebCore/css/MediaQueryEvaluator.cpp:581 > + return p != NoPointer; Four-space indent. > Source/WebCore/css/MediaQueryEvaluator.cpp:590 > + } Bad indent. > LayoutTests/fast/media/mq-pointer-touchscreen.html:7 > + if (window.internals) { > + internals.settings.setDeviceTouchScreen(true); > + } No need for { } > LayoutTests/fast/media/mq-pointer-touchscreen.html:41 > + <p id="a">This text should be green if some pointer is known to be available.</p> > + <p id="b">This text should be green if it's known that no pointers are available.</p> > + <p id="c">This text should be green if a touch screen is known to be available.</p> > + <p id="d">This text should be green if a mouse (but not a coarse pointer) is known to be available.</p> > + <p id="e">This text should be green if the least capable primary pointer supports hover.</p> > + <p id="f">This text should be green if the least capable primary pointer doesn't support hover.</p> > + <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.

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

This patch is not Chromium specific.

Created attachment 144388 [details] Patch

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.

Comment on attachment 144388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144388&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:589 > + return (pointer == NoPointer && str == "none") > + || (pointer == TouchPointer && str == "coarse") > + || (pointer == MousePointer && str == "fine"); To confirm, this is supposed to be case sensitive? > LayoutTests/fast/media/mq-pointer-touchscreen.html:15 > +@media screen and (pointer:coarse) { Can you add some tests for mixed case? > LayoutTests/fast/media/mq-pointer-touchscreen.html:24 > +@media screen and (hover:0) { You can add some tests for non-numeric values?

@tabatkins: Should we be using a vendor prefix in this patch?

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.

Created attachment 144600 [details] Patch

(In reply to comment #10) > (From update of attachment 144388 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144388&action=review > > > Source/WebCore/css/MediaQueryEvaluator.cpp:589 > > + return (pointer == NoPointer && str == "none") > > + || (pointer == TouchPointer && str == "coarse") > > + || (pointer == MousePointer && str == "fine"); > > To confirm, this is supposed to be case sensitive? It's case insensitive but it's normalized to lowercase by the parsing code. I've added tests that verify this. > > LayoutTests/fast/media/mq-pointer-touchscreen.html:15 > > +@media screen and (pointer:coarse) { > > Can you add some tests for mixed case? Done > > LayoutTests/fast/media/mq-pointer-touchscreen.html:24 > > +@media screen and (hover:0) { > > You can add some tests for non-numeric values? Done

(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.

Comment on attachment 144600 [details] Patch Thanks for clarifying! I see that now in the spreadsheet you gave me. My apologies, I was confused before.

(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."

I've filed https://bugs.webkit.org/show_bug.cgi?id=87806 to track adding the mouse support piece, since my patch here is just about touch screens.

Comment on attachment 144600 [details] Patch Rejecting attachment 144600 [details] from commit-queue. rbyers@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 144600 [details] Patch Rejecting attachment 144600 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: of 2 hunks FAILED -- saving rejects to file Source/WebCore/testing/InternalSettings.h.rej patching file Source/WebCore/testing/InternalSettings.idl patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/media/mq-pointer-expected.txt patching file LayoutTests/fast/media/mq-pointer.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12849950

Created attachment 144940 [details] Merge with trunk, resolve conflicts

Comment on attachment 144940 [details] Merge with trunk, resolve conflicts Clearing flags on attachment: 144940 Committed r119045: <http://trac.webkit.org/changeset/119045>

All reviewed patches have been landed. Closing bug.