Bug 46986 - Should commit pending autocorrection before next round of text checking.
Summary: Should commit pending autocorrection before next round of text checking.
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-01 09:07 PDT by Jia Pu
Modified: 2010-10-11 18:21 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (v1) (8.93 KB, patch)
2010-10-01 10:18 PDT, Jia Pu
adele: review-
Details | Formatted Diff | Diff
Proposed patch (v1) (9.76 KB, patch)
2010-10-07 14:07 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v3) (7.88 KB, patch)
2010-10-07 15:10 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-01 09:07:44 PDT
<rdar://problem/8424535>
Comment 1 Jia Pu 2010-10-01 10:18:15 PDT
Created attachment 69481 [details]
proposed patch (v1)
Comment 2 Enrica Casucci 2010-10-06 14:06:58 PDT
Comment on attachment 69481 [details]
proposed patch (v1)

View in context: https://bugs.webkit.org/attachment.cgi?id=69481&action=review

> WebCore/editing/Editor.cpp:2410
> +    if (m_rangeToBeReplacedByCorrection.get()) {

No need to call get(). Your test could be
if (m_rangeToBeReplacedByCorrection)

> WebCore/editing/Editor.cpp:2412
> +        RefPtr<Range> paragraphRange = m_rangeToBeReplacedByCorrection->cloneRange(ec);

It would be great if you could come up with a more descriptive name than paragraphRange.

> WebCore/editing/Editor.cpp:2416
> +            RefPtr<Range> offsetAsRange = Range::create(paragraphRange->startContainer(ec)->document(), paragraphRange->startPosition(), paragraphRange->startPosition());

Same for offsetAsRange.

> WebCore/editing/Editor.cpp:2421
> +                    // Take note of the location of autocorrection so that we can add marker after the replacement took place.

This code would benefit from a more detailed comment.

> WebCore/editing/Editor.cpp:2439
> +        m_rangeToBeReplacedByCorrection = RefPtr<Range>();

if what you want to do here is release, then call m_rangeToBeReplacedByCorrection.release();

> WebCore/editing/Editor.cpp:2717
> +            for (size_t i = 0; i < markerCount; ++i) {

You don't need markerCount, you can use directly markers.size() in your for loop.

> WebCore/editing/Editor.cpp:2724
> +            if (doMarkMisspelling)

You can use node->document()->markers()->addMarker, since you have node initialized.
Comment 3 Adele Peterson 2010-10-06 15:18:38 PDT
Comment on attachment 69481 [details]
proposed patch (v1)

If you address Enrica's comments, I'll do the official review.
Comment 4 Jia Pu 2010-10-07 14:07:52 PDT
Created attachment 70148 [details]
Proposed patch (v1)

Updated patch per comment 2.
Comment 5 Jia Pu 2010-10-07 14:17:20 PDT
Comment on attachment 70148 [details]
Proposed patch (v1)

Just noticed that this seems have broken the autocorrection-delete test I added earlier. Good thing to have the tests.
Comment 6 Jia Pu 2010-10-07 15:10:25 PDT
Created attachment 70158 [details]
Proposed patch (v3)

Updated patch per comment 2.
Comment 7 WebKit Commit Bot 2010-10-11 18:21:23 PDT
Comment on attachment 70158 [details]
Proposed patch (v3)

Clearing flags on attachment: 70158

Committed r69548: <http://trac.webkit.org/changeset/69548>
Comment 8 WebKit Commit Bot 2010-10-11 18:21:28 PDT
All reviewed patches have been landed.  Closing bug.