Bug 47689 – REGRESSION (r69548): Autocorrections are applied even after typing further characters in the word

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jia Pu]

<rdar://problem/8552250>

[Jia Pu]

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]

Created attachment 70785 [details]
Proposed patch (v2)

Added regression test.

[Mark Rowe (bdash)]

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]

Created attachment 70794 [details]
Proposed patch (v3)

Revised according to comment #3.

[Darin Adler]

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]

(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]

> > 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]

Created attachment 70800 [details]
Proposed patch (v4)

Revised according to comment #5.

[Darin Adler]

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]

(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]

Created attachment 70828 [details]
Proposed patch (v5)

Putting test in manual-tests.

[Jia Pu]

Created attachment 70829 [details]
Proposed patch (v6)

Put test in manual-tests.

[WebKit Commit Bot]

Comment on attachment 70829 [details]
Proposed patch (v6)

Clearing flags on attachment: 70829

Committed r69841: <http://trac.webkit.org/changeset/69841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.