Summary: | REGRESSION (r69548): Autocorrections are applied even after typing further characters in the word | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jia Pu <jiapu.mail> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.6 | ||||||||||||||||
Attachments: |
|
Description
Jia Pu
2010-10-14 13:44:17 PDT
Created attachment 70781 [details]
Proposed patch (v1)
Due to the reliance on wall clock delay, it's rather difficult to create a regression test with current testing facility.
Created attachment 70785 [details]
Proposed patch (v2)
Added regression test.
Comment on attachment 70785 [details] Proposed patch (v2) View in context: https://bugs.webkit.org/attachment.cgi?id=70785&action=review > WebCore/editing/Editor.cpp:2901 > + m_rangeToBeReplacedByCorrection.release(); release() on a RefPtr returns a PassRefPtr and clears out the RefPtr. This code isn’t interested in the return value of the release() method so it would be more obvious to call clear() instead. Created attachment 70794 [details] Proposed patch (v3) Revised according to comment #3. Comment on attachment 70794 [details] Proposed patch (v3) View in context: https://bugs.webkit.org/attachment.cgi?id=70794&action=review It’s not great to name a test after the Apple Radar bug number for the problem. Since people outside Apple have limited or no access to Radar we try to keep the amount of Radar references in the bugs and tests to a minimum. But I suppose we can live with it for a Mac-specific test. > LayoutTests/platform/mac-tiger/Skipped:213 > +platform/mac/editing/spelling/radar8552250.html > \ No newline at end of file Should include a newline. > LayoutTests/platform/mac/editing/spelling/radar8552250.html:38 > + setTimeout("keepTyping()", 1000); It’s really bad to have a 1 second delay in a regression test. We want the layout tests to run really quickly. There is a debugging machinery for making time move forward quickly: the eventSender.leapForward function. However, if the timing check is in code outside WebKit perhaps that won’t work. You need to at least give that a try. > LayoutTests/platform/mac/editing/spelling/radar8552250.html:46 > +<div>You should see phrase 'the collapsing' without underline.</div> How does the test output indicate the lack of the underline? Is there something in the render tree dump that would be there if the underline was present? (In reply to comment #5) > (From update of attachment 70794 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70794&action=review > I will generate a new patch to correct some of following issues. > It’s not great to name a test after the Apple Radar bug number for the problem. Since people outside Apple have limited or no access to Radar we try to keep the amount of Radar references in the bugs and tests to a minimum. But I suppose we can live with it for a Mac-specific test. I will use webkit bug number or more descriptive name in the future. > > > LayoutTests/platform/mac-tiger/Skipped:213 > > +platform/mac/editing/spelling/radar8552250.html > > \ No newline at end of file > > Should include a newline. > > > LayoutTests/platform/mac/editing/spelling/radar8552250.html:38 > > + setTimeout("keepTyping()", 1000); > > It’s really bad to have a 1 second delay in a regression test. We want the layout tests to run really quickly. There is a debugging machinery for making time move forward quickly: the eventSender.leapForward function. However, if the timing check is in code outside WebKit perhaps that won’t work. You need to at least give that a try. leapForward() didn't work in this case, because the timer is inside WebCore. > > > LayoutTests/platform/mac/editing/spelling/radar8552250.html:46 > > +<div>You should see phrase 'the collapsing' without underline.</div> > > How does the test output indicate the lack of the underline? Is there something in the render tree dump that would be there if the underline was present? The underline is drawn based on marker value "CorrectionIndicator", which seems to be contained in the dump.
> > How does the test output indicate the lack of the underline? Is there something in the render tree dump that would be there if the underline was present?
>
> The underline is drawn based on marker value "CorrectionIndicator", which seems to be contained in the dump.
I take it back. It seems a pixel comparison is necessary. It would be helpful to generalize hasSpellingMarker() function to allow query on any marker value.
Created attachment 70800 [details] Proposed patch (v4) Revised according to comment #5. Comment on attachment 70800 [details]
Proposed patch (v4)
I don’t think we should land a regression test that takes more than a second. We have two choices:
1) Make the leapForward trick work with WebCore timers.
2) Put this test into the manual tests directory until we can figure out a way to automate it without a 1 second delay.
(In reply to comment #9) > (From update of attachment 70800 [details]) > I don’t think we should land a regression test that takes more than a second. We have two choices: > > 1) Make the leapForward trick work with WebCore timers. > 2) Put this test into the manual tests directory until we can figure out a way to automate it without a 1 second delay. What's the proper location for manual tests? Would 0.35 second be acceptable? We need at least 0.3 second. Created attachment 70828 [details]
Proposed patch (v5)
Putting test in manual-tests.
Created attachment 70829 [details]
Proposed patch (v6)
Put test in manual-tests.
Comment on attachment 70829 [details] Proposed patch (v6) Clearing flags on attachment: 70829 Committed r69841: <http://trac.webkit.org/changeset/69841> All reviewed patches have been landed. Closing bug. |