[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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #142263|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #15 from James Robinson <jamesr at chromium.org>  2012-05-22 12:40:05 PST ---
(From update of attachment 142263)
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

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