Bug 85921

Created attachment 140797 [details] Patch

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 140797 [details] Patch Attachment 140797 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12648414

Comment on attachment 140797 [details] Patch Attachment 140797 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12651433

Comment on attachment 140797 [details] Patch Attachment 140797 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12648417

Comment on attachment 140797 [details] Patch Attachment 140797 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12653398

Created attachment 140806 [details] Patch

Comment on attachment 140806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140806&action=review > Source/WebCore/page/Settings.h:736 > + bool m_deviceTouchScreen : 1; it doesn't make any sense for this to be a bitfield here - it'll pack out to a full word anyway. move it up a bit if you want to be in the bitfield above. does it make sense for this to exist when ENABLE(TOUCH_EVENTS) is not set?

Created attachment 140933 [details] Patch

Comment on attachment 140806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140806&action=review >> Source/WebCore/page/Settings.h:736 >> + bool m_deviceTouchScreen : 1; > > it doesn't make any sense for this to be a bitfield here - it'll pack out to a full word anyway. move it up a bit if you want to be in the bitfield above. > > does it make sense for this to exist when ENABLE(TOUCH_EVENTS) is not set? Moved up to be part of the bitfield. I think conceptually this is orthogonal to whether touch events are enabled. Developers may want to set this flag to test how their site will lay out on a touch screen (especially if/when we have a media query for this), and users can use a touch screen to drive mouse events even if touch events are not enabled and some browsers may want to set this. That being said I'm not strongly opposed to putting this behind ENABLE(TOUCH_EVENTS).

Comment on attachment 140933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140933&action=review > Source/WebKit/chromium/tests/PopupMenuTest.cpp:188 > + m_webView->page()->settings()->setDeviceTouchScreen(false); should we also have tests for the inflated version? the default is false, so this call doesn't seem to actually do anything

Created attachment 142263 [details] Patch

(In reply to comment #11) > (From update of attachment 140933 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140933&action=review > > > Source/WebKit/chromium/tests/PopupMenuTest.cpp:188 > > + m_webView->page()->settings()->setDeviceTouchScreen(false); > > should we also have tests for the inflated version? > > the default is false, so this call doesn't seem to actually do anything You're right. I think these tests should actually not be dependent on a particular row height as most of them already aren't as it is. I've fixed the remaining few places.

Comment on attachment 142263 [details] Patch ping, can you take another look?

Comment on attachment 142263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142263&action=review This looks pretty good, I just have a number of smaller nits. With one more round I think this is good to go. > Source/WebCore/platform/chromium/PopupListBox.cpp:-635 > - if (RuntimeEnabledFeatures::touchEnabled()) It looks like you're deleting all callers of RuntimeEnabledFeatures::touchEnabled() / RuntimeEnabledFeatures::setTouchEnabled(). If that's true can you also remove the code for that? Do you expect it to come back? > Source/WebCore/platform/chromium/PopupListBox.h:84 > + bool isDeviceTouchScreen; this (and defaultDeviceScaleFactor) should be documented. the name is pretty clear, but it's not clear why this is relevant to a PopupContainer so documenting what effect this has (i.e. increasing padding) would be helpful > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:78 > + v->frame()->page()->settings()->isDeviceTouchScreen(); nit: I think it'd make sense to stash v->frame()->page()->settings() in a temporary. It's also a bit non-idiomatic to use single-letter variable names in WebKit, more typical names would be frameView instead of v and rect instead of r. > Source/WebKit/chromium/src/WebViewImpl.cpp:2829 > + m_page->settings()->isDeviceTouchScreen(); why does this reach through the Page instead of just grabbing settingsImpl() directly? > Source/WebKit/chromium/tests/PopupMenuTest.cpp:358 > + // menuHeight * 1.5 means the Y position on the item at index 1. is menuHeight the height of the whole menu or a single row? I think it must be the latter for this to make sense, but the name had me confused for a bit

Created attachment 143602 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142263&action=review Thanks for the review! Changes have been uploaded. >> Source/WebCore/platform/chromium/PopupListBox.cpp:-635 >> - if (RuntimeEnabledFeatures::touchEnabled()) > > It looks like you're deleting all callers of RuntimeEnabledFeatures::touchEnabled() / RuntimeEnabledFeatures::setTouchEnabled(). If that's true can you also remove the code for that? Do you expect it to come back? (set)TouchEnabled is still used by chromium to tell WebKit whether it should generate touch events. >> Source/WebCore/platform/chromium/PopupListBox.h:84 >> + bool isDeviceTouchScreen; > > this (and defaultDeviceScaleFactor) should be documented. the name is pretty clear, but it's not clear why this is relevant to a PopupContainer so documenting what effect this has (i.e. increasing padding) would be helpful Done. >> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:78 >> + v->frame()->page()->settings()->isDeviceTouchScreen(); > > nit: I think it'd make sense to stash v->frame()->page()->settings() in a temporary. It's also a bit non-idiomatic to use single-letter variable names in WebKit, more typical names would be frameView instead of v and rect instead of r. Done and done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2829 >> + m_page->settings()->isDeviceTouchScreen(); > > why does this reach through the Page instead of just grabbing settingsImpl() directly? I have now exposed these settings through WebSettingsImpl and use settingsImpl() directly. This was required since it does not grant direct access to the WebCore::Settings object directly. >> Source/WebKit/chromium/tests/PopupMenuTest.cpp:358 >> + // menuHeight * 1.5 means the Y position on the item at index 1. > > is menuHeight the height of the whole menu or a single row? I think it must be the latter for this to make sense, but the name had me confused for a bit Renamed to menuItemHeight.

Comment on attachment 143602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143602&action=review > Source/WebCore/page/Settings.h:571 > + void setDeviceTouchScreen(bool enabled) { m_deviceTouchScreen = enabled; } > + bool isDeviceTouchScreen() const { return m_deviceTouchScreen; } Do you still need this to exist on WebCore::Settings? If this only is ever set or queried on chromium's WebSettings, you can save the value in a bool on WebSettingsImpl and use that instead

(In reply to comment #18) > (From update of attachment 143602 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143602&action=review > > > Source/WebCore/page/Settings.h:571 > > + void setDeviceTouchScreen(bool enabled) { m_deviceTouchScreen = enabled; } > > + bool isDeviceTouchScreen() const { return m_deviceTouchScreen; } > > Do you still need this to exist on WebCore::Settings? If this only is ever set or queried on chromium's WebSettings, you can save the value in a bool on WebSettingsImpl and use that instead I was about to start consuming this from WebCore to support the new pointer:coarse and hover:0 CSS media features: https://bugs.webkit.org/show_bug.cgi?id=87403. Except I'm probably going to eventually want this to be tri-state: has touch screen, doesn't have touch screen, unknown (for ports that don't support it). If this CL is good as is, I can convert it to tri-state as part of my bug.

Add the setting to WebCore::Settings in the patch where you need it.

(In reply to comment #20) > Add the setting to WebCore::Settings in the patch where you need it. That's fine with me. Note that initially I expect to just add it back as a bool exactly how Rob has done. Making it tri-state will hopefully something I won't need until a future CL.

Comment on attachment 143602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143602&action=review >>> Source/WebCore/page/Settings.h:571 >>> + bool isDeviceTouchScreen() const { return m_deviceTouchScreen; } >> >> Do you still need this to exist on WebCore::Settings? If this only is ever set or queried on chromium's WebSettings, you can save the value in a bool on WebSettingsImpl and use that instead > > I was about to start consuming this from WebCore to support the new pointer:coarse and hover:0 CSS media features: https://bugs.webkit.org/show_bug.cgi?id=87403. Except I'm probably going to eventually want this to be tri-state: has touch screen, doesn't have touch screen, unknown (for ports that don't support it). If this CL is good as is, I can convert it to tri-state as part of my bug. PopupMenuChromium (in WebCore/platform/chromium) needs to query isDeviceTouchScreen and as far as I can tell only has access to WebCore::Settings.

Ping, James?

Comment on attachment 143602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143602&action=review This looks good. You seem to have addressed all of James' comments. I've got a handful of nits below and then I think we're ready to land this patch. > Source/WebCore/page/Settings.h:742 > + bool m_deviceTouchScreen : 1; The name of the member should match the name of the accessor. I might also have picked a name like "device supports touch" so that the if statements read more like English: "if device supports touch". > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:75 > popupSettings.defaultDeviceScaleFactor = > - v->frame()->page()->settings()->defaultDeviceScaleFactor(); > + settings->defaultDeviceScaleFactor(); This can be on one line. There's no line limit in WebKit. > Source/WebKit/chromium/src/WebViewImpl.cpp:2954 > popupSettings.defaultDeviceScaleFactor = > - m_page->settings()->defaultDeviceScaleFactor(); > + settingsImpl()->defaultDeviceScaleFactor(); One line. > Source/WebKit/chromium/src/WebViewImpl.cpp:2958 > + popupSettings.isDeviceTouchScreen = > + settingsImpl()->isDeviceTouchScreen(); One line.

Created attachment 144389 [details] Patch

Comment on attachment 143602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143602&action=review >> Source/WebCore/page/Settings.h:742 >> + bool m_deviceTouchScreen : 1; > > The name of the member should match the name of the accessor. I might also have picked a name like "device supports touch" so that the if statements read more like English: "if device supports touch". Done. >> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:75 >> + settings->defaultDeviceScaleFactor(); > > This can be on one line. There's no line limit in WebKit. Done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2954 >> + settingsImpl()->defaultDeviceScaleFactor(); > > One line. Done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:2958 >> + settingsImpl()->isDeviceTouchScreen(); > > One line. Done.

Attachment 144389 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 144390 [details] Patch

Comment on attachment 144390 [details] Patch Thanks.

Comment on attachment 144390 [details] Patch Clearing flags on attachment: 144390 Committed r118707: <http://trac.webkit.org/changeset/118707>

All reviewed patches have been landed. Closing bug.