Bug 112841 – Extend pointer/hover media features to support mouse and none

• Component: CSS
• Status: REOPENED

[David Bokan]

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.

[Rick Byers]

Already filed in bug 87806

*** This bug has been marked as a duplicate of bug 87806 ***

[David Bokan]

Reopening to attach new patch.

[David Bokan]

Created attachment 194117 [details]
work in progress, not for review

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Rick Byers]

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

[Terry Anderson]

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" ?

[David Bokan]

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

[Rick Byers]

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

[Ahmad Saleem]

This patch seems to do changes in Chromium port and those linked files. Do we need to salavage this or we can close this? Thanks!