[Webkit-unassigned] [Bug 61252] [Chromium]Add clipping for listBox in popup window to fix wrong location display when the autofill item is really long.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61252





--- Comment #7 from Naoki Takano <takano.naoki at gmail.com>  2011-05-23 15:14:06 PST ---
Ilya,

Thank you for your review.
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:370
> > +                listBox()->setMaxWidthAndLayout(max(widgetRect.width() - kBorderSize * 2, 0));
> 
> nit: This line can be moved out of the if/else, since it is the same in both places.
I don't think so, because only this is not "if/else" but "if/else if", and only if width is change, we want to call setMaxWidthAndLayout().

(In reply to comment #6)
> (From update of attachment 94354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94354&action=review
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:370
> > +                listBox()->setMaxWidthAndLayout(max(widgetRect.width() - kBorderSize * 2, 0));
> 
> nit: This line can be moved out of the if/else, since it is the same in both places.
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1381
> > +        // windowWidth exceeds m_maxWindowWidth, so we have to clip.
> 
> nit: Please move this comment into the if-stmt
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1385
> > +            baseWidth -= diff;
> 
> nit: I think these three lines would be clearer as
> windowWidth = m_maxWindowWidth;
> baseWidth = windowWidth - scrollbarWidth - paddingWidth;
> 
> If you don't find that clearer, though, feel free to leave this as is.
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1386
> > +            m_baseWidth = baseWidth;
> 
> nit: Is this line not redundant with the else of the if/else below?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.