[webkit-reviews] review denied: [Bug 85921] [chromium] Only increase size of Combo Box Options when displayed on touch screen : [Attachment 142263] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Robert Flack
<flackr at chromium.org>'s request for review:
Bug 85921: [chromium] Only increase size of Combo Box Options when displayed on
touch screen
https://bugs.webkit.org/show_bug.cgi?id=85921

Attachment 142263: Patch
https://bugs.webkit.org/attachment.cgi?id=142263&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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