Bug 47689 - REGRESSION (r69548): Autocorrections are applied even after typing further characters in the word
Summary: REGRESSION (r69548): Autocorrections are applied even after typing further ch...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-10-14 13:44 PDT by Jia Pu
Modified: 2010-10-14 23:53 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch (v1) (1.40 KB, patch)
2010-10-14 14:49 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v2) (15.47 KB, patch)
2010-10-14 15:11 PDT, Jia Pu
mrowe: review-
Details | Formatted Diff | Diff
Proposed patch (v3) (15.79 KB, patch)
2010-10-14 16:11 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v4) (71.13 KB, patch)
2010-10-14 16:55 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v5) (71.12 KB, patch)
2010-10-14 21:07 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v6) (3.98 KB, patch)
2010-10-14 21:09 PDT, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2010-10-14 13:44:17 PDT
<rdar://problem/8552250>
Comment 1 Jia Pu 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.
Comment 2 Jia Pu 2010-10-14 15:11:55 PDT
Created attachment 70785 [details]
Proposed patch (v2)

Added regression test.
Comment 3 Mark Rowe (bdash) 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.
Comment 4 Jia Pu 2010-10-14 16:11:58 PDT
Created attachment 70794 [details]
Proposed patch (v3)

Revised according to comment #3.
Comment 5 Darin Adler 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?
Comment 6 Jia Pu 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.
Comment 7 Jia Pu 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.
Comment 8 Jia Pu 2010-10-14 16:55:48 PDT
Created attachment 70800 [details]
Proposed patch (v4)

Revised according to comment #5.
Comment 9 Darin Adler 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.
Comment 10 Jia Pu 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.
Comment 11 Jia Pu 2010-10-14 21:07:24 PDT
Created attachment 70828 [details]
Proposed patch (v5)

Putting test in manual-tests.
Comment 12 Jia Pu 2010-10-14 21:09:52 PDT
Created attachment 70829 [details]
Proposed patch (v6)

Put test in manual-tests.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2010-10-14 23:53:10 PDT
All reviewed patches have been landed.  Closing bug.