Bug 112841

Already filed in bug 87806 *** This bug has been marked as a duplicate of bug 87806 ***

Reopening to attach new patch.

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

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

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!