Bug 85921

Summary: [chromium] Only increase size of Combo Box Options when displayed on touch screen
Product: WebKit Reporter: Robert Flack <flackr>
Component: WebCore Misc.Assignee: Robert Flack <flackr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, gns, jamesr, philn, rbyers, tdresser, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87403    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Robert Flack 2012-05-08 15:03:46 PDT
In https://bugs.webkit.org/show_bug.cgi?id=80027 the size of combo box options was increased when touch events were supported. Supporting touch events should not imply the use of a touch friendly layout as we would like to enable touch event support on all devices in case a touch screen is attached. When contents are being displayed on a touch screen we can use the setDeviceTouchScreen pref to enable the increased combo box option size.
Comment 1 Robert Flack 2012-05-08 15:19:23 PDT
Created attachment 140797 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-08 15:21:24 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 3 Early Warning System Bot 2012-05-08 15:28:14 PDT
Comment on attachment 140797 [details]
Patch

Attachment 140797 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12648414
Comment 4 Early Warning System Bot 2012-05-08 15:28:49 PDT
Comment on attachment 140797 [details]
Patch

Attachment 140797 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12651433
Comment 5 Build Bot 2012-05-08 15:42:57 PDT
Comment on attachment 140797 [details]
Patch

Attachment 140797 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12648417
Comment 6 Build Bot 2012-05-08 15:59:41 PDT
Comment on attachment 140797 [details]
Patch

Attachment 140797 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12653398
Comment 7 Robert Flack 2012-05-08 16:04:42 PDT
Created attachment 140806 [details]
Patch
Comment 8 James Robinson 2012-05-08 16:21:05 PDT
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?
Comment 9 Robert Flack 2012-05-09 06:50:37 PDT
Created attachment 140933 [details]
Patch
Comment 10 Robert Flack 2012-05-09 06:54:02 PDT
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 11 James Robinson 2012-05-09 15:37:36 PDT
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
Comment 12 Robert Flack 2012-05-16 07:56:57 PDT
Created attachment 142263 [details]
Patch
Comment 13 Robert Flack 2012-05-16 07:59:48 PDT
(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 14 Robert Flack 2012-05-18 07:52:49 PDT
Comment on attachment 142263 [details]
Patch

ping, can you take another look?
Comment 15 James Robinson 2012-05-22 12:40:05 PDT
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
Comment 16 Robert Flack 2012-05-23 11:28:48 PDT
Created attachment 143602 [details]
Patch
Comment 17 Robert Flack 2012-05-23 11:31:31 PDT
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 18 James Robinson 2012-05-23 15:24:07 PDT
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
Comment 19 Rick Byers 2012-05-24 10:52:05 PDT
(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.
Comment 20 James Robinson 2012-05-24 10:59:48 PDT
Add the setting to WebCore::Settings in the patch where you need it.
Comment 21 Rick Byers 2012-05-24 11:06:43 PDT
(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 22 Robert Flack 2012-05-24 11:24:31 PDT
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.
Comment 23 Rick Byers 2012-05-26 18:30:37 PDT
Ping, James?
Comment 24 Adam Barth 2012-05-28 11:31:57 PDT
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.
Comment 25 Robert Flack 2012-05-28 13:05:16 PDT
Created attachment 144389 [details]
Patch
Comment 26 Robert Flack 2012-05-28 13:06:49 PDT
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.
Comment 27 WebKit Review Bot 2012-05-28 13:07:25 PDT
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.
Comment 28 Robert Flack 2012-05-28 13:18:05 PDT
Created attachment 144390 [details]
Patch
Comment 29 Adam Barth 2012-05-28 14:16:53 PDT
Comment on attachment 144390 [details]
Patch

Thanks.
Comment 30 WebKit Review Bot 2012-05-28 14:55:38 PDT
Comment on attachment 144390 [details]
Patch

Clearing flags on attachment: 144390

Committed r118707: <http://trac.webkit.org/changeset/118707>
Comment 31 WebKit Review Bot 2012-05-28 14:55:45 PDT
All reviewed patches have been landed.  Closing bug.