Bug 49306 - Fix autofill popup height computation
Summary: Fix autofill popup height computation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 00:42 PST by Ilya Sherman
Modified: 2010-11-13 01:47 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.89 KB, patch)
2010-11-10 00:45 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (1.95 KB, patch)
2010-11-10 01:54 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2010-11-10 02:39 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (3.05 KB, patch)
2010-11-12 22:26 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2010-11-10 00:42:57 PST
Fix autofill popup height computation
Comment 1 Ilya Sherman 2010-11-10 00:45:50 PST
Created attachment 73467 [details]
Patch
Comment 2 Kent Tamura 2010-11-10 01:00:52 PST
Comment on attachment 73467 [details]
Patch

looks ok.
Comment 3 Shinichiro Hamaji 2010-11-10 01:02:19 PST
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 4 Shinichiro Hamaji 2010-11-10 01:16:11 PST
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.
Comment 5 Ilya Sherman 2010-11-10 01:54:53 PST
Created attachment 73475 [details]
Patch
Comment 6 Shinichiro Hamaji 2010-11-10 01:58:39 PST
Comment on attachment 73475 [details]
Patch

Sorry, my last concern was a false alarm.
Comment 7 Ilya Sherman 2010-11-10 02:00:18 PST
(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 8 WebKit Commit Bot 2010-11-10 02:21:44 PST
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
Comment 9 Ilya Sherman 2010-11-10 02:39:38 PST
Created attachment 73476 [details]
Patch
Comment 10 WebKit Commit Bot 2010-11-10 05:38:12 PST
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 11 WebKit Commit Bot 2010-11-10 05:54:28 PST
Comment on attachment 73476 [details]
Patch

Clearing flags on attachment: 73476

Committed r71734: <http://trac.webkit.org/changeset/71734>
Comment 12 WebKit Commit Bot 2010-11-10 05:54:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Ilya Sherman 2010-11-12 22:26:16 PST
Created attachment 73809 [details]
Patch
Comment 14 Ilya Sherman 2010-11-12 22:30:41 PST
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.
Comment 15 Ilya Sherman 2010-11-12 22:32:33 PST
Marking as "unconfirmed" because there was a regression; and I don't have privileges to mark as "reopened".
Comment 16 Shinichiro Hamaji 2010-11-13 01:23:29 PST
Comment on attachment 73809 [details]
Patch

Looks good.
Comment 17 WebKit Commit Bot 2010-11-13 01:47:47 PST
Comment on attachment 73809 [details]
Patch

Clearing flags on attachment: 73809

Committed r71963: <http://trac.webkit.org/changeset/71963>
Comment 18 WebKit Commit Bot 2010-11-13 01:47:53 PST
All reviewed patches have been landed.  Closing bug.