Bug 28254 – [Chromium] Remove legacy code in "WebCore/platform/chromium/PopupMenuChromium.cpp"

• Component: WebKit Misc.
• Status: RESOLVED DUPLICATE

[Hironori Bono]

(Copied from <http://crbug.com/19107>)

Chrome Version       : 3.0.197.11
OS + version : Ubuntu linux, 8.04
CPU architecture (32-bit / 64-bit): 64
window manager : metacity w/ GNOME
URLs (if applicable) :
Behavior in Firefox 3.x (if applicable): words typed fast enough refine the 
selection
Behavior in Chrome for Windows (optional): same as Firefox

What steps will reproduce the problem?
1. Go to any page which contains a dropdown with more than one entry 
starting with the same letter
2. Select dropdown and begin typing a word

What is the expected result?
Given the following options:
+
|-apple
|-anchovy
|-antelope
|-artichoke
|-navel orange
|-rutabaga


If you type "ar", you should be jumped to "artichoke"

What happens instead?

You jump to rutabaga.

Please use labels and text to provide additional information.

Linux and Mac Chromium have not used KeyDown messages since we made our keyboard-input code platform-independent.
On the other hand, PopupMenuChromium.cpp still uses the KeyDown messages and this caused the above problem.
To solve this problem, we need to remove the platform-dependent code in isCharacterTypeEvent().

[Hironori Bono]

I noticed this was a duplicate of <https://bugs.webkit.org/show_bug.cgi?id=28205>.
Sorry for committing a duplicate.

*** This bug has been marked as a duplicate of bug 28205 ***

[Xiaomei Ji]

(In reply to comment #1)
> I noticed this was a duplicate of
> <https://bugs.webkit.org/show_bug.cgi?id=28205>.
> Sorry for committing a duplicate.
> 
> *** This bug has been marked as a duplicate of bug 28205 ***

This is not a duplicate bug of 28205.
fix of 28205 does not remove the platform dependent code in isCharacterTypeEvent().

But I tested the test case in Windows and Mac, they both works fine.
Could you reproduce the bug in Linux?

[Hironori Bono]

(In reply to comment #2)
> (In reply to comment #1)
> > I noticed this was a duplicate of
> > <https://bugs.webkit.org/show_bug.cgi?id=28205>.
> > Sorry for committing a duplicate.
> > 
> > *** This bug has been marked as a duplicate of bug 28205 ***
> 
> This is not a duplicate bug of 28205.
> fix of 28205 does not remove the platform dependent code in
> isCharacterTypeEvent().

I think the solution for this issue is technically the same issue as your Bug 28205. Technically, this issue is caused by this typeAheadFind() that does not update m_lastCharTime at all on Linux Chrome. So, updating m_lastCharTime every time when typeAheadFind() is called is another solution for this issue. 

I also confirmed this issue does not happen on Linux Chrome now.

[Hironori Bono]

Created attachment 34825 [details]
A layout test that prevent regressions

Even though this issue is fixed by a change for Bug 28205, I would like to add a layout test for this bug to prevent regressions.
Is it possible to add only a layout test?

[Eric Seidel (no email)]

Comment on attachment 34825 [details]
A layout test that prevent regressions

This bug is resolved.  No need for this patch to be marked for review.

[Hironori Bono]

(In reply to comment #5)
> (From update of attachment 34825 [details])
> This bug is resolved.  No need for this patch to be marked for review.

Sorry for my stupid question in advance.
I would like to clarify whether or not it is good to send a change that only includes layout tests since I'm writing some layout tests to replace some part of manual tests with them.
As I written in my previous comments, this issue is fixed by a change for Bug 28205. But it uses a manual test because it is not easy to write a layout test which verifies Bug 28205. On the other hand, this issue can be verified with a simple layout test. So, I would like to add this layout test to lessen the dependency to manual tests even though this doesn't include any code changes.
Personally, either is fine for me whether or not to add the attached layout test. Nevertheless, I would like to clarify it to prevent annoying reviewers with my stupid changes that only include layout tests.

Regards,