Bug 51382 – [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

[Chromium]Issue 6024008: Consider the popup window position when the window shows upward

[Naoki Takano]

Created attachment 77086 [details]
Patch

[David Levin]

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

[Naoki Takano]

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?

[Adam Barth]

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

[Naoki Takano]

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.

[Naoki Takano]

Eric,

Thank you for your comment.

But I cannot find "Tools" folder in WebKit.

Where is this?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

WebKitTools recently moved to Tools.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Naoki Takano]

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

[Eric Seidel (no email)]

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.

[Naoki Takano]

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,

[Naoki Takano]

Is there anybody who can review my patch?

[David Levin]

(In reply to comment #15)
> Is there anybody who can review my patch?

You have nothing marked for review. (No patches marked with r?)

[Naoki Takano]

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

Could you review ?

[Ilya Sherman]

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

[Naoki Takano]

Ilya,

Thank you for your suggestion.

I'll fix it and upload the patch again.

[Naoki Takano]

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

[Naoki Takano]

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

Could you review again?

[WebKit Review Bot]

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.

[Ilya Sherman]

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.

[Naoki Takano]

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?

[Naoki Takano]

Could you review my change?

Is there anybody who are familiar with this part?

[Kent Tamura]

I think fishd and dglazkov are appropriate reviewers.

[Kent Tamura]

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  <[e-mail]>

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.

[Kent Tamura]

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

and -> or

[Kent Tamura]

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.

[Naoki Takano]

Thanks, Kent,

I'll update patch again later.

[Naoki Takano]

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

[Naoki Takano]

I eliminated layout() call form calculateWidgetRect().

Could you review it?

[Ilya Sherman]

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.

[Ilya Sherman]

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.

[Ilya Sherman]

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" ;)

[Naoki Takano]

(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;-)

[Naoki Takano]

Created attachment 78303 [details]
Remove m_popupRect

 Remove m_popupRect.
    Change name of layoutOrCalculateWidgetRect().

[WebKit Review Bot]

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.

[Naoki Takano]

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?

[Kent Tamura]

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.

[Naoki Takano]

Created attachment 78498 [details]
Revise ChangeLog and format errors.

Add more description for ChangeLogs.
Also fix some format errors.

Could you review again?

Thanks,

[Naoki Takano]

Kent,

Could you review my change?

[Naoki Takano]

Could you review, Kent?

[Kent Tamura]

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
> [phone]  Naoki Takano  <[e-mail]>

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

[Naoki Takano]

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.

[Kent Tamura]

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.

[Naoki Takano]

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.

[Kent Tamura]

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.

[Naoki Takano]

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,

[Naoki Takano]

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,

[Naoki Takano]

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,

[WebKit Review Bot]

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.

[Kent Tamura]

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

[Naoki Takano]

Created attachment 79135 [details]
Change names and indentation.

Changed some param names according to your suggestion.

Please review it.

Thanks,

[Kent Tamura]

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

Looks good!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79135 [details]:

http/tests/appcache/xhr-foreign-resource.html bug 52562 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Kent Tamura]

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
> [phone]  Naoki Takano  <[e-mail]>
> +
> +        [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  <[e-mail]>
 
         Reviewed by NOBODY (OOPS!).
 
         [Chromium] Fix popup menu ...

> WebKit/chromium/ChangeLog:3
> [phone]  Naoki Takano  <[e-mail]>
> +
> +        [Chromium] Fix popup menu re-positioning when the menu is opened upward, above the corresponding form field.

ditto.

[Naoki Takano]

Created attachment 79189 [details]
Chnage ChangeLog.

Inserted one line into each ChangeLog.

Please review it again.

Thanks,

[Naoki Takano]

Created attachment 79190 [details]
Delete one white space.

Oops, I deleted one white space.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79190 [details]:

inspector/debugger-pause-on-breakpoint.html bug 51320 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 79190 [details]
Delete one white space.

Clearing flags on attachment: 79190

Committed r75982: <http://trac.webkit.org/changeset/75982>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.