Fix autofill popup height computation
Created attachment 73467 [details] Patch
Comment on attachment 73467 [details] Patch looks ok.
Comment on attachment 73467 [details] Patch Clearing r+ and cq+ . Please wait a sec... I'm guessing this patch needs a bit more works.
Comment on attachment 73467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73467&action=review It's nice if we can add a test (maybe manual test, unfortunately) for this change. > WebCore/platform/chromium/PopupMenuChromium.cpp:1301 > + int y = 0; I think we can just reuse windowHeight and we don't need this variable? > WebCore/platform/chromium/PopupMenuChromium.cpp:1307 > + y += rowHeight; Could you check if this test http://trac.webkit.org/browser/trunk/WebCore/manual-tests/display-none-option.html?rev=71727 still works? I guess we need if (!m_popupClient->itemStyle(i).isDisplayNone()) here.
Created attachment 73475 [details] Patch
Comment on attachment 73475 [details] Patch Sorry, my last concern was a false alarm.
(In reply to comment #4) > (From update of attachment 73467 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73467&action=review > > It's nice if we can add a test (maybe manual test, unfortunately) for this change. AFAIK this code is used only be Chromium autofill, and there is no good way to add a test for this from within WebKit -- even a manual one. Eventually I'd like to add a way to test this popup more generally, but that's outside the scope of this patch. > > > WebCore/platform/chromium/PopupMenuChromium.cpp:1301 > > + int y = 0; > > I think we can just reuse windowHeight and we don't need this variable? Good call =) > > WebCore/platform/chromium/PopupMenuChromium.cpp:1307 > > + y += rowHeight; > > Could you check if this test http://trac.webkit.org/browser/trunk/WebCore/manual-tests/display-none-option.html?rev=71727 still works? Seems to work -- pretty sure the <select> popup is independent of this code. > I guess we need > > if (!m_popupClient->itemStyle(i).isDisplayNone()) > > here. This is handled internally to getRowHeight().
Comment on attachment 73475 [details] Patch Rejecting patch 73475 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 73475]" exit_code: 1 Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=73475&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=49306&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Updating working directory Processing patch 73475 from bug 49306. ERROR: /Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/5528079
Created attachment 73476 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 73476 [details]: animations/suspend-resume-animation.html media/video-seek-past-end-paused.html Please file bugs against the tests. These tests were authored by cmarrin@apple.com, eric.carlson@apple.com, hclam@google.com, and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
Comment on attachment 73476 [details] Patch Clearing flags on attachment: 73476 Committed r71734: <http://trac.webkit.org/changeset/71734>
All reviewed patches have been landed. Closing bug.
Created attachment 73809 [details] Patch
Comment on attachment 73809 [details] Patch (In reply to comment #7) > (In reply to comment #4) > > (From update of attachment 73467 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=73467&action=review > > > > It's nice if we can add a test (maybe manual test, unfortunately) for this change. > > AFAIK this code is used only be Chromium autofill, and there is no good way to add a test for this from within WebKit -- even a manual one. Eventually I'd like to add a way to test this popup more generally, but that's outside the scope of this patch. Looks like I was dead wrong here -- this code is indeed used by both <select> elements and autofill popups. The previous patch broken <select> elements that needed scroll bars; this patch should fix that.
Marking as "unconfirmed" because there was a regression; and I don't have privileges to mark as "reopened".
Comment on attachment 73809 [details] Patch Looks good.
Comment on attachment 73809 [details] Patch Clearing flags on attachment: 73809 Committed r71963: <http://trac.webkit.org/changeset/71963>