Bug 46839 - Autocorrection shouldn't prompt the same correction after user has edited previous correction.
Summary: Autocorrection shouldn't prompt the same correction after user has edited pre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-09-29 14:16 PDT by Jia Pu
Modified: 2010-10-01 11:11 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (v1) (1.82 KB, patch)
2010-09-29 15:26 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-09-29 14:16:34 PDT
<rdar://problem/8476963>
Comment 1 Jia Pu 2010-09-29 15:26:00 PDT
Created attachment 69260 [details]
Proposed patch (v1)

Prior to bug 45709. We use "Replacement" marker to indicate the word that has been autocorrected and need to be marked with underline. Whenever user edits an autocorrected word, in order to remove the underline, we remove the "Replacement" marker. However, doing this also prevents us from keeping track of on which word we have already applied autocorrection. So if the user corrects an autocorrection, we would keep prompting the same autocorrection suggestion.

In bug 45709, we introduced a new marker "CorrectionIndicator" to indicate whether we should draw underline. So now we can remove underline by removing "CorrectionIndicator" marker. This allows us to keep "Replacement" marker around, so that we won't suggest autocorrection on word that already bears "Replacement" marker.
Comment 2 Darin Adler 2010-09-29 16:06:40 PDT
Comment on attachment 69260 [details]
Proposed patch (v1)

Change looks fine. We should come up with a way to do regression tests. It’s not safe to have this code in here but not tested in the WebKit regression test suite.
Comment 3 Jia Pu 2010-09-29 16:12:10 PDT
(In reply to comment #2)
> (From update of attachment 69260 [details])
> Change looks fine. We should come up with a way to do regression tests. It’s not safe to have this code in here but not tested in the WebKit regression test suite.

Agree. I will look into writing test for autocorrection. One of the trick parts is that autocorrection result depends on user's past usage. In other word, we adapt to user feedback. So unless we always run the tests on a freshly installed system, there's no guarantee to get the expected correction result back.
Comment 4 WebKit Commit Bot 2010-09-30 01:53:41 PDT
Comment on attachment 69260 [details]
Proposed patch (v1)

Rejecting patch 69260 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
 tests successful.
Files=14, Tests=304,  1 wallclock secs ( 0.63 cusr +  0.13 csys =  0.76 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21408 test cases.
websocket/tests/simple.html -> failed

Exiting early after 1 failures. 21390 tests run.
498.10s total testing time

21389 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
33 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4229026
Comment 5 Darin Adler 2010-10-01 10:28:43 PDT
Comment on attachment 69260 [details]
Proposed patch (v1)

Trying again because the last failure seemed to be due to a flaky websocket test.
Comment 6 WebKit Commit Bot 2010-10-01 11:11:34 PDT
Comment on attachment 69260 [details]
Proposed patch (v1)

Clearing flags on attachment: 69260

Committed r68902: <http://trac.webkit.org/changeset/68902>
Comment 7 WebKit Commit Bot 2010-10-01 11:11:40 PDT
All reviewed patches have been landed.  Closing bug.