[webkit-reviews] review denied: [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 : [Attachment 77302] Chnage ChangeLog and some function name change according to Ilya's suggestion

[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]Issue 60427: Fix popup menu re-positioning when the menu
is opened upward, above the corresponding form field.  http://crbug.com/60427
https://bugs.webkit.org/show_bug.cgi?id=51382

Attachment 77302: Chnage ChangeLog and some function name change according to
Ilya's suggestion
https://bugs.webkit.org/attachment.cgi?id=77302&action=review

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

> WebCore/ChangeLog:21
> +010-12-09  Dan Bernstein  <mitz at apple.com>

Do not modify an existing ChangeLog entry.
Do not use TAB characters in ChangeLog.

>> 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.

I agree with Ilya.  Side effect in calculateWidgetRect() sounds slightly
curious.
Please try removing layout(), and rename it to layoutAndCalculateWidgetRect().

>> WebCore/platform/chromium/PopupMenuChromium.cpp:368
>> +		    IntRect windowRect(targetFrameRect.location(), size());
> 
> This line seems unnecessary given that windowRect is computed above.

This "windowRect" hides another "windowRect".  It's bad.