Bug 47689

Summary: REGRESSION (r69548): Autocorrections are applied even after typing further characters in the word
Product: WebKit Reporter: Jia Pu <jiapu.mail>
Component: New BugsAssignee: 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 Flags
Proposed patch (v1)
none
Proposed patch (v2)
mrowe: review-
Proposed patch (v3)
none
Proposed patch (v4)
none
Proposed patch (v5)
none
Proposed patch (v6) none

Jia Pu
Reported 2010-10-14 13:44:17 PDT
Attachments
Proposed patch (v1) (1.40 KB, patch)
2010-10-14 14:49 PDT, Jia Pu
no flags
Proposed patch (v2) (15.47 KB, patch)
2010-10-14 15:11 PDT, Jia Pu
mrowe: review-
Proposed patch (v3) (15.79 KB, patch)
2010-10-14 16:11 PDT, Jia Pu
no flags
Proposed patch (v4) (71.13 KB, patch)
2010-10-14 16:55 PDT, Jia Pu
no flags
Proposed patch (v5) (71.12 KB, patch)
2010-10-14 21:07 PDT, Jia Pu
no flags
Proposed patch (v6) (3.98 KB, patch)
2010-10-14 21:09 PDT, Jia Pu
no flags
Jia Pu
Comment 1 2010-10-14 14:49:18 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.
Jia Pu
Comment 2 2010-10-14 15:11:55 PDT
Created attachment 70785 [details] Proposed patch (v2) Added regression test.
Mark Rowe (bdash)
Comment 3 2010-10-14 15:42:28 PDT
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.
Jia Pu
Comment 4 2010-10-14 16:11:58 PDT
Created attachment 70794 [details] Proposed patch (v3) Revised according to comment #3.
Darin Adler
Comment 5 2010-10-14 16:18:25 PDT
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?
Jia Pu
Comment 6 2010-10-14 16:23:41 PDT
(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.
Jia Pu
Comment 7 2010-10-14 16:38:39 PDT
> > 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.
Jia Pu
Comment 8 2010-10-14 16:55:48 PDT
Created attachment 70800 [details] Proposed patch (v4) Revised according to comment #5.
Darin Adler
Comment 9 2010-10-14 17:07:51 PDT
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.
Jia Pu
Comment 10 2010-10-14 17:12:44 PDT
(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.
Jia Pu
Comment 11 2010-10-14 21:07:24 PDT
Created attachment 70828 [details] Proposed patch (v5) Putting test in manual-tests.
Jia Pu
Comment 12 2010-10-14 21:09:52 PDT
Created attachment 70829 [details] Proposed patch (v6) Put test in manual-tests.
WebKit Commit Bot
Comment 13 2010-10-14 23:53:04 PDT
Comment on attachment 70829 [details] Proposed patch (v6) Clearing flags on attachment: 70829 Committed r69841: <http://trac.webkit.org/changeset/69841>
WebKit Commit Bot
Comment 14 2010-10-14 23:53:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.