Bug 46986 – Should commit pending autocorrection before next round of text checking.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

<rdar://problem/8424535>

[Jia Pu]

Created attachment 69481 [details]
proposed patch (v1)

[Enrica Casucci]

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.

[Adele Peterson]

Comment on attachment 69481 [details]
proposed patch (v1)

If you address Enrica's comments, I'll do the official review.

[Jia Pu]

Created attachment 70148 [details]
Proposed patch (v1)

Updated patch per comment 2.

[Jia Pu]

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.

[Jia Pu]

Created attachment 70158 [details]
Proposed patch (v3)

Updated patch per comment 2.

[WebKit Commit Bot]

Comment on attachment 70158 [details]
Proposed patch (v3)

Clearing flags on attachment: 70158

Committed r69548: <http://trac.webkit.org/changeset/69548>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.