Summary: | [Chromium] Remove legacy code in "WebCore/platform/chromium/PopupMenuChromium.cpp" | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hironori Bono <hbono> | ||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED DUPLICATE | ||||||
Severity: | Normal | CC: | estade, xji | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Attachments: |
|
Description
Hironori Bono
2009-08-13 04:21:27 PDT
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 *** (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? (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. 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? 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.
(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, |