[webkit-reviews] review denied: [Bug 51382] [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field. : [Attachment 78303] Remove m_popupRect

[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 78303: Remove m_popupRect
https://bugs.webkit.org/attachment.cgi?id=78303&action=review

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

The code change looks ok to me.
r- because of out-of-sync ChangeLog.


> WebCore/ChangeLog:15
> +	   * platform/chromium/PopupMenuChromium.cpp:
> +	   (WebCore::PopupContainer::layoutOrCalculateWidgetRect): 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.

Please update the ChangeLog entry.

I'd like to know why the boundsRect() -> frameRect() changes in WebViewImpl.cpp
are needed.

> WebCore/platform/chromium/PopupMenuChromium.cpp:564
> +void PopupContainer::refresh(const IntRect& r)

Please use more descriptive parameter name.

> WebCore/platform/chromium/PopupMenuChromium.cpp:568
> +	// Move it below the select widget.
> +    location.move(0, r.height());

Indentation error.

> WebCore/platform/chromium/PopupMenuChromium.h:166
> +	// Compute size and location of widget and children.
> +    void moveAndLayout();
> +

This block seems to be unnecessary.