Bug 51382

Created attachment 77086 [details] Patch

Added two people who have done a lot of recent changes in this file so they can give their opinion on the change.

This patch is related to [Chromium]Issue 6024008. But I noticed the patch is not enough. I tried to upload it again, but I cannot upload because of utf8 decode error as following, WebKit/wx/ChangeLog" took 0.01s Traceback (most recent call last): File "WebKitTools/Scripts/webkit-patch", line 70, in <module> main() File "WebKitTools/Scripts/webkit-patch", line 65, in main WebKitPatch(__file__).main() File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/tool/multicommandtool.py", line 308, in main result = command.check_arguments_and_execute(options, args, self) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/tool/multicommandtool.py", line 117, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 43, in execute self._sequence.run_and_handle_errors(tool, options, self._prepare_state(options, args, tool)) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors self._run(tool, options, state) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/tool/steps/suggestreviewers.py", line 45, in run reviewers = self._tool.checkout().suggested_reviewers(self._options.git_commit, self._changed_files(state)) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 127, in suggested_reviewers commit_infos = self.recent_commit_infos_for_files(changed_files) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 123, in recent_commit_infos_for_files return set(map(self.commit_info_for_revision, revisions)) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/common/memoized.py", line 45, in __call__ result = self._function(*args) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 66, in commit_info_for_revision changelog_entries = self.changelog_entries_for_revision(revision) File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 61, in changelog_entries_for_revision return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self._is_path_to_changelog(path)] File "/mnt/chrome/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 56, in _latest_entry_for_changelog_at_revision changelog_file = StringIO.StringIO(changelog_contents.decode("utf-8")) File "/usr/lib/python2.6/encodings/utf_8.py", line 16, in decode return codecs.utf_8_decode(input, errors, True) UnicodeDecodeError: 'utf8' codec can't decode bytes in position 819-821: invalid data I don't use any special mutibyte character. Do you have any suggestion?

+ Eric (our webkit-patch Unicode expert) > Do you have any suggestion? Sorry webkit-patch didn't work for you. You can use Tools/Scripts/prepare-ChangeLogs and Tools/Scripts/svn-create-patch to create the ChangeLogs and generate the patch manually.

Created attachment 77089 [details] Uploaded again because the previous patch has bug. I cannot revise my patch due to very strange error. So, for now, I uploaded patch itself again.

Eric, Thank you for your comment. But I cannot find "Tools" folder in WebKit. Where is this?

Yes, don't pass --suggested-reviewers and your upload shoudl be fine. WE need to file that unicode error for --suggested-reviewers and get it fixed.

WebKitTools recently moved to Tools.

I suspect that WebKit/wx/ChangeLog has non-utf8 in it. :)

Yup. WebKit/wx/ChangeLog needs to be fixed.

I fixed the ChangeLog in r74405. However since --suggest-reviewers will pull the ChangeLog from the original commit, that wont' fix your issue. I suggest just not passing --suggest-reviewers next time you use webkit-patch upload for this change.

Eric, As I said, I cannot find any "Tools" folder. The all files and folder in WebKit root folder are following, Android.mk CMakeLists.txt Makefile WebKit2 ANGLE common.pri Makefile.shared WebKitExamplePlugins autogen.sh configure.ac PageLoadTests WebKitLibraries autotools DerivedSources.pro PlanetWebKit WebKit.pri b GNUmakefile.am SunSpider WebKit.pro BugsSite Issue55874 test2.patch WebKitSite ChangeLog JavaScriptCore test.patch WebKitTools cmake JavaScriptGlue WebCore wscript cmakeconfig.h.cmake LayoutTests WebKit Or is there in any other root folder? But I cannot find it even if I "find . -name Tools".

Yes, your checkout is out of date. Just use "WebKitTools" in your checkout. It was renamed to Tools yesterday. When you update your checkout, you'll find that WebKitTools is moved to Tools.

I see, But, right now, I have to sync the rev. with chrome project. So that's why the source is a little bit old. Anyway, if raw patch is fine, I'll revise with it. Thanks,

Is there anybody who can review my patch?

(In reply to comment #15) > Is there anybody who can review my patch? You have nothing marked for review. (No patches marked with r?)

Comment on attachment 77089 [details] Uploaded again because the previous patch has bug. Could you review ?

View in context: https://bugs.webkit.org/attachment.cgi?id=77089&action=review I'm having some trouble following these changes -- it looks like the patch you've uploaded is actually a combination of two diffs: (a) a diff of the second patch to the first, and (b) a diff of the first patch to the trunk version. Please try again to upload the patch, and double-check that the diff looks right. If you run into any trouble, probably the quickest way to get help is to hop onto #webkit on IRC, though of course posting questions here or via email also works. > WebCore/ChangeLog:5 > + [Chromium]Issue 6024008: Consider the popup window position when the window shows upward nit: Please refer to the issue number in the Chromium bug tracker, and include a link: http://crbug.com/60427 For the description, I think something like this would be clearer: "Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field." > WebCore/ChangeLog:14 > + * platform/chromium/PopupMenuChromium.cpp: > + (WebCore::PopupContainer::calcWidgetRect): > + (WebCore::PopupContainer::showPopup): > + (WebCore::PopupContainer::moveAndLayout): > + * platform/chromium/PopupMenuChromium.h: nit: Please add brief descriptions of the changes for each of these. > WebCore/platform/chromium/PopupMenuChromium.h:199 > + IntRect calcWidgetRect(const IntRect& targetFrameRect); nit: Please include a comment describing what this function does. Also, prefer the full "calculate" to the abbreviated "calc".

Ilya, Thank you for your suggestion. I'll fix it and upload the patch again.

Created attachment 77302 [details] Chnage ChangeLog and some function name change according to Ilya's suggestion

Comment on attachment 77302 [details] Chnage ChangeLog and some function name change according to Ilya's suggestion Could you review again?

Attachment 77302 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/chromium/PopupMenuChromium.cpp', u'WebCore/platform/chromium/PopupMenuChromium.h', u'WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1 WebCore/ChangeLog:7410: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7411: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7418: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7420: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7422: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 77302 [details] Chnage ChangeLog and some function name change according to Ilya's suggestion 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.

Comment on attachment 77302 [details] Chnage ChangeLog and some function name change according to Ilya's suggestion View in context: https://bugs.webkit.org/attachment.cgi?id=77302&action=review > WebCore/ChangeLog:20 > + Ok, > WebCore/platform/chromium/PopupMenuChromium.cpp:344 > Yes, "calculateWidgetRect()" has side effect. Because the function uses "layout()". So do you imply I should make "calculateWidgetRect()" have no side effect? > WebCore/platform/chromium/PopupMenuChromium.cpp:369 > + widgetRect = chromeClient->windowToScreen(windowRect); I need more investigation. Because "layout()" has side effect and widgetRect might be change according to "layout()" call. The original code also recalculate here, so that's why I remained this line. > WebCore/platform/chromium/PopupMenuChromium.cpp:444 > +{ Ok, > WebCore/platform/chromium/PopupMenuChromium.h:200 > + IntRect calculateWidgetRect(const IntRect& targetFrameRect); Ok, > WebCore/platform/chromium/PopupMenuChromium.h:215 > }; m_popupRect is different from frameRect(). Because frameRect() is variable according to setFrameRect(). But this m_popupRect is saved as the first original position and rect of input text position. Or do you mean other problem?

Could you review my change? Is there anybody who are familiar with this part?

I think fishd and dglazkov are appropriate reviewers.

Comment on attachment 77302 [details] Chnage ChangeLog and some function name change according to Ilya's suggestion View in context: https://bugs.webkit.org/attachment.cgi?id=77302&action=review > WebCore/ChangeLog:21 > +010-12-09 Dan Bernstein <mitz@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.

> Please try removing layout(), and rename it to layoutAndCalculateWidgetRect(). and -> or

Comment on attachment 77302 [details] Chnage ChangeLog and some function name change according to Ilya's suggestion View in context: https://bugs.webkit.org/attachment.cgi?id=77302&action=review > WebCore/ChangeLog:10 > + [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 > + > + No new tests. (OOPS!) The summary has too much information. I like: [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 http://crbug.com/60427 <Add how you fix this bug> Please remove "(OOPS!)", and add a reason why no new tests.

Thanks, Kent, I'll update patch again later.

Created attachment 78213 [details] Eliminate layout() call from calculateWidgetRect() and change the name to layoutOrCalculateWidgetRect()

I eliminated layout() call form calculateWidgetRect(). Could you review it?

Apologies for the delay in responding. (In reply to comment #24) > (From update of attachment 77302 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77302&action=review > > Yes, "calculateWidgetRect()" has side effect. Because the function uses "layout()". So do you imply I should make "calculateWidgetRect()" have no side effect? I think either it should have no side effect, or it should have a name that makes it clearer that there will be a side effect. > > WebCore/platform/chromium/PopupMenuChromium.cpp:369 > > + widgetRect = chromeClient->windowToScreen(windowRect); > > I need more investigation. Because "layout()" has side effect and widgetRect might be change according to "layout()" call. The original code also recalculate here, so that's why I remained this line. I also didn't investigate closely -- it just looked rather surprising when reading the code. If you find that you need to leave this in place, please add a brief comment explaining why it's needed =) > > WebCore/platform/chromium/PopupMenuChromium.h:215 > > }; > > m_popupRect is different from frameRect(). > Because frameRect() is variable according to setFrameRect(). > But this m_popupRect is saved as the first original position and rect of input text position. > > Or do you mean other problem? Again, I didn't try making this change, but it strikes me as odd that we need to cache the input text field's position. It seems like we should be able to query the position directly from the input text field when we need it, though it might be that there's no (clean/easy) way to do that.

Erm, some of the comments in comment 33 are obsolete. I should have read through the rest of the comments before posting -- sorry about that.

Comment on attachment 78213 [details] Eliminate layout() call from calculateWidgetRect() and change the name to layoutOrCalculateWidgetRect() View in context: https://bugs.webkit.org/attachment.cgi?id=78213&action=review You should set "r: ?" and "cq: ?" for patches that you'd like reviewed -- setting "r: -" means that the review has been denied. > WebCore/platform/chromium/PopupMenuChromium.cpp:332 > +IntRect PopupContainer::layoutOrCalculateWidgetRect() nit: Since you don't call layout() within this function, the name calculateWidgetRect() is more appropriate. I think you read Kent's "and -> or" comment as applying to the wrong "and" ;)

(In reply to comment #33) > Apologies for the delay in responding. No problem, > > > WebCore/platform/chromium/PopupMenuChromium.h:215 > > > }; > > > > m_popupRect is different from frameRect(). > > Because frameRect() is variable according to setFrameRect(). > > But this m_popupRect is saved as the first original position and rect of input text position. > > > > Or do you mean other problem? > > Again, I didn't try making this change, but it strikes me as odd that we need to cache the input text field's position. It seems like we should be able to query the position directly from the input text field when we need it, though it might be that there's no (clean/easy) way to do that. I see. Actually, I didn't find the way to query the input text field position, so I cached m_popupRect. I try to find the way, but do you have any suggestion or referred part? Also I roll back calculateWidgetRect() function name. I totally misunderstood Kent's comment. Sorry;-)

Created attachment 78303 [details] Remove m_popupRect Remove m_popupRect. Change name of layoutOrCalculateWidgetRect().

Attachment 78303 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/chromium/PopupMenuChromium.cpp', u'WebCore/platform/chromium/PopupMenuChromium.h', u'WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1 WebCore/platform/chromium/PopupMenuChromium.h:174: The parameter name "r" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

I'll fix style bug later, but I want to make sure if it's correct to move away m_popupRect. Could you review my patch?

Comment on attachment 78303 [details] Remove m_popupRect 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.

Created attachment 78498 [details] Revise ChangeLog and format errors. Add more description for ChangeLogs. Also fix some format errors. Could you review again? Thanks,

Kent, Could you review my change?

Could you review, Kent?

Comment on attachment 78498 [details] Revise ChangeLog and format errors. 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@gmail.com> You edited a wrong ChangeLog. You should update WebKit/chromium/ChangeLog.

Comment on attachment 78498 [details] Revise ChangeLog and format errors. View in context: https://bugs.webkit.org/attachment.cgi?id=78498&action=review > WebCore/platform/chromium/PopupMenuChromium.cpp:333 > { Yes, So should I split it to two params, or just to change the name is enough? > WebCore/platform/chromium/PopupMenuChromium.cpp:357 > // And move upwards if necessary. Yes, You are right. So I should look into more. But it need to call setMaxHeight() part, I'll pull out this part. > WebCore/platform/chromium/PopupMenuChromium.h:172 > Ok, So is it enough to add more comment here? > WebKit/ChangeLog:2 > + Thank you for your suggestion. I didn't know that.

Comment on attachment 78498 [details] Revise ChangeLog and format errors. View in context: https://bugs.webkit.org/attachment.cgi?id=78498&action=review >> WebCore/platform/chromium/PopupMenuChromium.cpp:333 >> { > > Yes, > So should I split it to two params, or just to change the name is enough? Should split it into two. >> WebCore/platform/chromium/PopupMenuChromium.h:172 >> > > Ok, > So is it enough to add more comment here? Either of adding a comment or add a meaningful parameter name. >> WebKit/ChangeLog:2 >> + > > Thank you for your suggestion. > I didn't know that. You should use Tools/Scripts/prepare-ChangeLog.

Comment on attachment 78498 [details] Revise ChangeLog and format errors. View in context: https://bugs.webkit.org/attachment.cgi?id=78498&action=review > WebCore/platform/chromium/PopupMenuChromium.h:173 > // The menu per-item data. WebKit format don't look like to take parameter names in header file declaration. Is it Ok to add a meaningful parameter name in WebKit format? > WebKit/ChangeLog:3 > + [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field. I wanted to use it. But it failed due to UTF8 coding errors. I'll try it again, but if I can't I'll edit manually.

Comment on attachment 78498 [details] Revise ChangeLog and format errors. View in context: https://bugs.webkit.org/attachment.cgi?id=78498&action=review Please use the reply feature when you add a reply comment. You can write a reply by clicking a comment which you'd like to reply to. >> WebCore/platform/chromium/PopupMenuChromium.h:173 >> // The menu per-item data. > > WebKit format don't look like to take parameter names in header file declaration. > Is it Ok to add a meaningful parameter name in WebKit format? http://webkit.org/coding/coding-style.html > 8. Leave meaningless variable names out of function declarations. So, we should keep meaningful variable names in declarations. There are many examples in WebKit headers.

Kent, Thank you for your review and comment every time. (In reply to comment #48) > Please use the reply feature when you add a reply comment. > You can write a reply by clicking a comment which you'd like to reply to. Ok, > So, we should keep meaningful variable names in declarations. There are many examples in WebKit headers. I see. I just mimic the declaration in the same header file. I should read WebKit coding style guide again. Thanks,

Created attachment 78899 [details] Fix comments and pull out setMaxHeight() call. Kent, Could you review again? At this time, I pull out setMaxHeight() call, so I cannot delete some RectInt local object to call the function. Also according to the latest source code location change, I changed the patch. Thanks,

Created attachment 79057 [details] Restore layout() call Sorry for bothering you again. But I restore layout() call to layoutAndCalculateWidgetRect(). Because my previous patch does not work well about layout() and setMaxHeight() call. I tried to rewrite it as the same call without layout() call, but it became confusing. Please review it. Thanks,

Attachment 79057 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/chromium/PopupMenuChromium.cpp:584: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 79057 [details] Restore layout() call 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

Created attachment 79135 [details] Change names and indentation. Changed some param names according to your suggestion. Please review it. Thanks,

Comment on attachment 79135 [details] Change names and indentation. Looks good!

The commit-queue encountered the following flaky tests while processing attachment 79135 [details]: http/tests/appcache/xhr-foreign-resource.html bug 52562 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 79135 [details] Change names and indentation. Rejecting attachment 79135 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1 Last 500 characters of output: ce-expected.txt M LayoutTests/ChangeLog r75930 = cda0ab5a05703c4a6769fee4d5a613ed7523d38f (refs/remotes/trunk) M Source/WebKit/chromium/tests/WebFrameTest.cpp M Source/WebKit/chromium/ChangeLog r75931 = 5e0e02629103bd87b049b6be215b2160cb3c58b8 (refs/remotes/trunk) Auto packing the repository for optimum performance. You may also run "git gc" manually. See "git help gc" for more information. First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/7563151

Comment on attachment 79135 [details] Change names and indentation. View in context: https://bugs.webkit.org/attachment.cgi?id=79135&action=review > Source/WebCore/ChangeLog:3 > +2011-01-13 Naoki Takano <takano.naoki@gmail.com> > + > + [Chromium] Fix popup menu re-positioning when the menu is opened The ChangeLog format is wrong. Please insert " Reviewed by NOBODY (OOPS!)." and 1 blank line between date&name and the summary line like the following. 2011-01-13 Naoki Takano <takano.naoki@gmail.com> Reviewed by NOBODY (OOPS!). [Chromium] Fix popup menu ... > WebKit/chromium/ChangeLog:3 > +2011-01-10 Naoki Takano <takano.naoki@gmail.com> > + > + [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field. ditto.

Created attachment 79189 [details] Chnage ChangeLog. Inserted one line into each ChangeLog. Please review it again. Thanks,

Created attachment 79190 [details] Delete one white space. Oops, I deleted one white space.

The commit-queue encountered the following flaky tests while processing attachment 79190 [details]: inspector/debugger-pause-on-breakpoint.html bug 51320 (author: podivilov@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 79190 [details] Delete one white space. Clearing flags on attachment: 79190 Committed r75982: <http://trac.webkit.org/changeset/75982>

All reviewed patches have been landed. Closing bug.