[Webkit-unassigned] [Bug 85921] [chromium] Only increase size of Combo Box Options when displayed on touch screen

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85921





--- Comment #17 from Robert Flack <flackr at chromium.org>  2012-05-23 11:31:31 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.