Summary: | [chromium] Increase size of Combo Box Options for touch and high DPI devices | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Dresser <tdresser> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | fishd, fsamuel, rjkroege, tkent, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 80799 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Tim Dresser
2012-03-01 07:30:50 PST
Created attachment 129710 [details]
Patch
Comment on attachment 129710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129710&action=review > Source/WebCore/platform/chromium/PopupListBox.cpp:49 > +#include "Settings.h" do you really need this include? > Source/WebCore/platform/chromium/PopupListBox.cpp:357 > + const int& scale = m_settings.defaultDeviceScaleFactor; why make this a reference? it seems like making a copy of the int should be sufficient unless you really need the indirection. > Source/WebCore/platform/chromium/PopupListBox.cpp:397 > + const int& scale = m_settings.defaultDeviceScaleFactor; ditto > Source/WebCore/platform/chromium/PopupListBox.cpp:401 > + // Height and y will both be evenly divisible by scale. should you perhaps assert this? it seems like rounding errors if possible would be annoying. > Source/WebCore/platform/chromium/PopupListBox.cpp:632 > + const int& scale = m_settings.defaultDeviceScaleFactor; ditto > Source/WebCore/platform/chromium/PopupListBox.cpp:634 > + if (WebCore::RuntimeEnabledFeatures::touchEnabled()) you should not need the "WebCore::" prefix > Source/WebCore/rendering/RenderMenuList.cpp:312 > + const int& scale = document()->page()->settings()->defaultDeviceScaleFactor(); no reference > Source/WebKit/chromium/src/WebViewImpl.cpp:637 > + // That tap triggered a select popup which is the same as the one that so we will see a flickering of the select popup? is there some way to mitigate / avoid that? Created attachment 130614 [details]
Patch
(In reply to comment #2) > (From update of attachment 129710 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129710&action=review > > > Source/WebCore/platform/chromium/PopupListBox.cpp:49 > > +#include "Settings.h" > > do you really need this include? > Nope, removed. > > Source/WebCore/platform/chromium/PopupListBox.cpp:357 > > + const int& scale = m_settings.defaultDeviceScaleFactor; > > why make this a reference? it seems like making a copy of the int should be sufficient unless you really need the indirection. > Done. > > Source/WebCore/platform/chromium/PopupListBox.cpp:397 > > + const int& scale = m_settings.defaultDeviceScaleFactor; > > ditto > > > Source/WebCore/platform/chromium/PopupListBox.cpp:401 > > + // Height and y will both be evenly divisible by scale. > > should you perhaps assert this? it seems like rounding errors if possible would be annoying. > Done. > > Source/WebCore/platform/chromium/PopupListBox.cpp:632 > > + const int& scale = m_settings.defaultDeviceScaleFactor; > > ditto > > > Source/WebCore/platform/chromium/PopupListBox.cpp:634 > > + if (WebCore::RuntimeEnabledFeatures::touchEnabled()) > > you should not need the "WebCore::" prefix > Done. > > Source/WebCore/rendering/RenderMenuList.cpp:312 > > + const int& scale = document()->page()->settings()->defaultDeviceScaleFactor(); > > no reference > > > Source/WebKit/chromium/src/WebViewImpl.cpp:637 > > + // That tap triggered a select popup which is the same as the one that > > so we will see a flickering of the select popup? is there some way to mitigate / avoid that? This code is parallel to Source/WebCore/rendering/RenderMenuList.cpp:516. The popup is never shown, as it is hidden before a render occurs. Comment on attachment 130614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130614&action=review Switch the 'const int scale' to just 'int scale', and then R=me > Source/WebCore/platform/chromium/PopupListBox.cpp:356 > + const int scale = m_settings.defaultDeviceScaleFactor; nit: no need for 'const' in front of int here. it is unnecessary verbosity. we usually only write 'const int' when giving a name to an int literal. Created attachment 131093 [details]
Patch
Created attachment 131103 [details]
Patch
Created attachment 131104 [details]
Patch
Comment on attachment 131104 [details] Patch Clearing flags on attachment: 131104 Committed r110359: <http://trac.webkit.org/changeset/110359> Created attachment 131882 [details]
Patch
(In reply to comment #10) > Created an attachment (id=131882) [details] > Patch The most recent patch ensures that RuntimeEnabledFeatures::touch_enabled() is false for the duration of SelectPopupMenuTest. This fixes the unit test failure in SelectPopupMenuTest.ClickItem. Comment on attachment 131882 [details] Patch Rejecting attachment 131882 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: g file Source/WebCore/rendering/RenderMenuList.cpp patching file Source/WebKit/chromium/src/WebViewImpl.cpp Hunk #1 FAILED at 602. Hunk #2 succeeded at 2753 (offset 62 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej patching file Source/WebKit/chromium/tests/PopupMenuTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Darin Fish..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12000453 Created attachment 133034 [details]
Patch
Comment on attachment 133034 [details] Patch Clearing flags on attachment: 133034 Committed r111539: <http://trac.webkit.org/changeset/111539> All reviewed patches have been landed. Closing bug. |