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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 143602: Patch
https://bugs.webkit.org/attachment.cgi?id=143602&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.