Bug 112841 - Extend pointer/hover media features to support mouse and none
Summary: Extend pointer/hover media features to support mouse and none
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Bokan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-20 13:55 PDT by David Bokan
Modified: 2013-03-20 15:03 PDT (History)
15 users (show)

See Also:


Attachments
work in progress, not for review (22.53 KB, patch)
2013-03-20 14:06 PDT, David Bokan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Bokan 2013-03-20 13:55:29 PDT
https://bugs.webkit.org/show_bug.cgi?id=87806

Extension of previous work from above bug. Replacing deviceSupportsMouse/Touch with a primaryPointer setting to support pointer/hover media queries.
Comment 1 Rick Byers 2013-03-20 14:00:33 PDT
Already filed in bug 87806

*** This bug has been marked as a duplicate of bug 87806 ***
Comment 2 David Bokan 2013-03-20 14:06:23 PDT
Reopening to attach new patch.
Comment 3 David Bokan 2013-03-20 14:06:25 PDT
Created attachment 194117 [details]
work in progress, not for review
Comment 4 WebKit Review Bot 2013-03-20 14:10:40 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Rick Byers 2013-03-20 14:23:02 PDT
Comment on attachment 194117 [details]
work in progress, not for review

View in context: https://bugs.webkit.org/attachment.cgi?id=194117&action=review

Looks really good David!  Just a few minor comments.

> Source/WebKit/chromium/ChangeLog:3
> +        Extend pointer/hover media features to support mouse and none

I'd suggest using a different description here.  There's nothing in this part of the tree specific to the media queries.  Instead say that you're adding an API to pass the browser's least-capable primary pointer device.

> Source/WebCore/css/MediaQueryEvaluator.cpp:710
> +    float number = 1.0f;

WebKit style says not to include the .0f. See http://www.webkit.org/coding/coding-style.html

> Source/WebCore/page/Settings.in:197
> +primaryPointerDevice type=PointerDevice, initial=PointerDeviceUnknown

I know it's long, but we should probably call this 'leastCapablePrimaryPointerDevice' to make it clear that Mouse+Touch should report as Touch.

Alternately perhaps it would be cleaner to add one more value to the enum that indicates both Mouse and Touch are considered primary (and treat that the same as Touch everywhere).  I think I like this better - the notion of 'least capable' is somewhat context dependent (eg. touch supports multiple contacts).

> Source/WebKit/chromium/public/WebSettings.h:102
> +    // DEPRECATED

Keeping the common on the same line and eliminating the extra whitespace would be cleaner IMHO.
Also we should probably file a webkit bug to track removing this and assign that to you, then you can change the comment to:
"FIXME: remove this http://wkb.ug/1234"

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:129
> +// DEPRECATED

again (and in Settings.in as well) referencing a bug that tracks removing this is ideal
Comment 6 Terry Anderson 2013-03-20 14:45:09 PDT
Comment on attachment 194117 [details]
work in progress, not for review

View in context: https://bugs.webkit.org/attachment.cgi?id=194117&action=review

Congrats on uploading your first WebKit patch! I just have a few minor comments to add to what Rick said.

> Source/WebCore/css/MediaQueryEvaluator.cpp:717
> +        || (pointer == PointerDeviceTouch && number == 0.0f)

WebKit style says to avoid using an equality comparison when checking for zero, so !number was right.

> Source/WebCore/css/MediaQueryEvaluator.cpp:718
> +        || (pointer == PointerDeviceMouse && number == 1.0f);

Same comment Rick made, this should be just 1 and not 1.0f.

> Source/WebCore/testing/InternalSettings.cpp:529
> +        ec = SYNTAX_ERR;

Just checking: do we really want this else case? Does it make sense to set the primary pointer device to PointerDeviceUnknown when |pointerDevice| is not one of "mouse", "touch", or "none" ?
Comment 7 David Bokan 2013-03-20 14:55:04 PDT
Comment on attachment 194117 [details]
work in progress, not for review

View in context: https://bugs.webkit.org/attachment.cgi?id=194117&action=review

>> Source/WebKit/chromium/ChangeLog:3
>> +        Extend pointer/hover media features to support mouse and none
> 
> I'd suggest using a different description here.  There's nothing in this part of the tree specific to the media queries.  Instead say that you're adding an API to pass the browser's least-capable primary pointer device.

Will-do

>> Source/WebCore/css/MediaQueryEvaluator.cpp:710
>> +    float number = 1.0f;
> 
> WebKit style says not to include the .0f. See http://www.webkit.org/coding/coding-style.html

Fixed

>> Source/WebCore/page/Settings.in:197
>> +primaryPointerDevice type=PointerDevice, initial=PointerDeviceUnknown
> 
> I know it's long, but we should probably call this 'leastCapablePrimaryPointerDevice' to make it clear that Mouse+Touch should report as Touch.
> 
> Alternately perhaps it would be cleaner to add one more value to the enum that indicates both Mouse and Touch are considered primary (and treat that the same as Touch everywhere).  I think I like this better - the notion of 'least capable' is somewhat context dependent (eg. touch supports multiple contacts).

I think calling it "least capable" would be a bit confusing since whatever is set isn't necessarily less capable than the alternative, it's more about what device is actually being used. I'm fine with the alternative, I've added a PointerDeviceMouseAndTouch option as well as a comment to hopefully clarify intention.

>> Source/WebKit/chromium/public/WebSettings.h:102
>> +    // DEPRECATED
> 
> Keeping the common on the same line and eliminating the extra whitespace would be cleaner IMHO.
> Also we should probably file a webkit bug to track removing this and assign that to you, then you can change the comment to:
> "FIXME: remove this http://wkb.ug/1234"

Bug added (http://wkb.ug/112847) and commend updated in all places
Comment 8 Rick Byers 2013-03-20 15:03:15 PDT
Comment on attachment 194117 [details]
work in progress, not for review

View in context: https://bugs.webkit.org/attachment.cgi?id=194117&action=review

>> Source/WebCore/testing/InternalSettings.cpp:529
>> +        ec = SYNTAX_ERR;
> 
> Just checking: do we really want this else case? Does it make sense to set the primary pointer device to PointerDeviceUnknown when |pointerDevice| is not one of "mouse", "touch", or "none" ?

I prefer the else personally.  It will help us catch bugs in the test like setPrimaryPointerDevice('Mouse').  Unknown is an explicit state (port doesn't tell us about it's pointers).