[webkit-reviews] review denied: [Bug 51382] [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field. : [Attachment 78498] Revise ChangeLog and format errors.

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Naoki Takano
<takano.naoki at gmail.com>'s request for review:
Bug 51382: [Chromium] Fix popup menu re-positioning when the menu is opened
upward, above the corresponding form field.
https://bugs.webkit.org/show_bug.cgi?id=51382

Attachment 78498: Revise ChangeLog and format errors.
https://bugs.webkit.org/attachment.cgi?id=78498&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78498&action=review

I'm not familiar with this area.  I'm trying to understand...

r- because of wrong ChangeLog.

> WebCore/platform/chromium/PopupMenuChromium.cpp:332
> +IntRect PopupContainer::calculateWidgetRect(const IntRect& popupRect)

"popupRect" parameter seems not to represent a rectangle of something.
What we need is
 - initial coordinate of the popup, and
 - the height of the target form control.
Right?

> WebCore/platform/chromium/PopupMenuChromium.cpp:347
> +	   IntRect windowRect(popupRect.location(), targetSize);
> +	   widgetRect = chromeClient->windowToScreen(windowRect);

We can omit the local variable "windowRect".

> WebCore/platform/chromium/PopupMenuChromium.cpp:-368
> -		   if (spaceAbove > spaceBelow)
> -		       m_listBox->setMaxHeight(spaceAbove);
> -		   else
> -		       m_listBox->setMaxHeight(spaceBelow);
> -		   layout();
> -		   // Our size has changed, recompute the widgetRect.
> -		   widgetRect = chromeClient->windowToScreen(frameRect());

This removal looks to change the existing behavior.  I wonder what's the
behavior change.

> WebCore/platform/chromium/PopupMenuChromium.cpp:375
> +	   const IntRect widgetRect = calculateWidgetRect(popupRect);
>	   chromeClient->popupOpened(this, widgetRect, false);

We can merge these two lines.  The local variable "widgetRect" is not needed.

> WebCore/platform/chromium/PopupMenuChromium.cpp:575
> +    IntRect popupRect = IntRect(location, focusRect.size());
> +
>      listBox()->updateFromElement();
> +    // Store the original height to check if we need to request the
location.
> +    int originalHeight = height();
>      layout();
> +    IntRect widgetRect = calculateWidgetRect(popupRect);

The local variable "popupRect" is referred just once. We can omit it.

> WebCore/platform/chromium/PopupMenuChromium.h:171
> +    void refresh(const IntRect&);

It's unclear what rectangle should be passed.

> WebKit/ChangeLog:1
> +2011-01-10  Naoki Takano  <takano.naoki at gmail.com>

You edited a wrong ChangeLog.  You should update WebKit/chromium/ChangeLog.