[Webkit-unassigned] [Bug 51382] [Chromium]Issue 60427: Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field. http://crbug.com/60427

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from Ilya Sherman <isherman at chromium.org>  2010-12-23 02:30:24 PST ---
(From update of attachment 77302)
View in context: https://bugs.webkit.org/attachment.cgi?id=77302&action=review

> WebCore/ChangeLog:19
> +          * platform/chromium/PopupMenuChromium.cpp:
> +          (WebCore::PopupContainer::calculateWidgetRect): New Function to
> +					calculate popup widget rect.
> +          (WebCore::PopupContainer::showPopup): Move widgetRect calculation to
> +					calculateWidgetRect().
> +          (WebCore::PopupContainer::moveAndLayout): New function to calculate both
> +					location and size and set the rect.
> +          * platform/chromium/PopupMenuChromium.h: Append new functions.

nit: There's no 80-col restriction for WebKit, so it's fine to keep each description on a single line.

> WebCore/platform/chromium/PopupMenuChromium.cpp:343
>      layout();

It seems weird to me that layout() -- which seems to have side effects -- is being called from a method named "calculateWidgetRect", which I would expect not to have side effects.

> WebCore/platform/chromium/PopupMenuChromium.cpp:368
> +                IntRect windowRect(targetFrameRect.location(), size());

This line seems unnecessary given that windowRect is computed above.

> WebCore/platform/chromium/PopupMenuChromium.cpp:443
> +void PopupContainer::moveAndLayout()

nit: Please add this in the same order as in the header file, below layout().

> WebCore/platform/chromium/PopupMenuChromium.h:199
> +    // Calculate popup widget size and location and returns it as IntRect.

nit: "Calculate popup widget size and location." (the rest seems redundant to me)

> WebCore/platform/chromium/PopupMenuChromium.h:214
> +    IntRect m_popupRect;

Does the value of this variable ever differ from the value returned by calling frameRect()?  Looking at the code, it seems to me like they are tracking the same thing.

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