[webkit-reviews] review denied: [Bug 51382] [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field. : [Attachment 79057] Restore layout() call

[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 79057: Restore layout() call
https://bugs.webkit.org/attachment.cgi?id=79057&action=review

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

I think the code is good and has no wrong side effects.
r- because of a style error.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:585
> +    IntRect widgetRect =
layoutAndCalculateWidgetRect(targetFormRect.height(), location);
> +   if (originalHeight != widgetRect.height())
> +	   setFrameRect(widgetRect);

Wrong indentation.

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

nit: the target is not a form, it's a form control.  So I prefer
"targetControlRect" for the parmeter name.

> Source/WebCore/platform/chromium/PopupMenuChromium.h:197
> +    IntRect layoutAndCalculateWidgetRect(int heightOfTargetFormControl,
const IntPoint& initialCoordinateOfPopup);

nit: Using "of" for variable names are not so common. I prefer:
 - targetControlHeight and
 - popupInitialCoordinate