Bug 57665 – [Mac] WebCore need to notify AppKit spell checker after user has modified autocorrected text.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

<rdar://problem/7350477>

[Jia Pu]

Created attachment 87937 [details]
Patch (v1)

[Early Warning System Bot]

Attachment 87937 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8185320

[WebKit Review Bot]

Attachment 87937 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8320314

[Darin Adler]

editing/Editor.cpp:127: error: ‘bool WebCore::markersHaveIdenticalDescription(const WTF::Vector<WebCore::DocumentMarker, 0u>&)’ defined but not used

Sounds like that function has to be moved inside the #if.

[Jia Pu]

Created attachment 88065 [details]
Patch (v2)

Fixed a build failure.

[Darin Adler]

Comment on attachment 88065 [details]
Patch (v2)

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

> Source/WebCore/dom/DocumentMarkerController.cpp:351
> +            if (node == startContainer && node == endContainer) {
> +                // The range spans only one node.
> +                if (it->endOffset > static_cast<unsigned>(range->startOffset()) && it->startOffset < static_cast<unsigned>(range->endOffset()))
> +                    foundMarkers.append(*it);
> +            } else {
> +                if (node == startContainer) {
> +                    if (it->endOffset > static_cast<unsigned>(range->startOffset()))
> +                        foundMarkers.append(*it);
> +                } else if (node == endContainer) {
> +                    if (it->startOffset < static_cast<unsigned>(range->endOffset()))
> +                        foundMarkers.append(*it);
> +                } else
> +                    foundMarkers.append(*it);

I think that with continue you could write this with less repeated code:

    if (node == startContainer && it->endOffset <= static_cast<unsigned>(range->startOffset()))
        continue;
    if (node == endContainer && it->startOffset >= static_cast<unsigned>(range->endOffset()))
        continue;
    foundMarkers.append(*it);

Cuts it down from 13 lines of code to 5 and I think it doesn’t lose clarity.

If this logic was copied from another function, I suggest the same simplification for that other function.

> Source/WebCore/editing/Editor.cpp:2451
> +                        Range* range = replacedRange.get();

I don’t think this local variable does us much good.

> Source/WebCore/editing/Editor.cpp:2784
> +        DocumentMarker::MarkerType markerType = markerTypesToAdd[i];
> +        Range* range = replacementRange.get();
>          if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
> -            markers->addMarker(replacementRange.get(), markerTypesToAdd[i]);
> -        else
> -            markers->addMarker(replacementRange.get(), markerTypesToAdd[i], m_correctionPanelInfo.replacedString);
> +            markers->addMarker(range, markerType);
> +        else {
> +            if (markerType == DocumentMarker::Replacement || markerType == DocumentMarker::Autocorrected)
> +                markers->addMarker(range, markerType, m_correctionPanelInfo.replacedString);
> +            else
> +                markers->addMarker(range, markerType);
> +        }

I think the logic here could be streamlined a bit. The different branches of the if statement look almost the same to me.

[Jia Pu]

Created attachment 88129 [details]
Patch (v3)

[Darin Adler]

Comment on attachment 88129 [details]
Patch (v3)

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

> Source/WebCore/dom/DocumentMarker.h:40
> +        // Text has been modified by spell correction, reversion of spell correction or other types of substitution. 

This would be better worded as "or other type" rather than "or other types".

> Source/WebCore/dom/DocumentMarker.h:41
> +        // On some platforms, this prevents the text to be autocorrected again. On post Snow Leopard Mac OS X, 

Correct grammar would be "prevents the text from being autocorrected".

> Source/WebCore/dom/DocumentMarker.h:54
>          // On some platforms, this prevents the text to be spellchecked again.

Grammar: should be "from being spellchecked" rather than "to be spellchecked".

[WebKit Commit Bot]

Comment on attachment 88129 [details]
Patch (v3)

Rejecting attachment 88129 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build'..." exit_code: 2

Last 500 characters of output:
env XCODE_VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/Editor.o /mnt/git/webkit-commit-queue/Source/WebCore/editing/Editor.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/8329408

[Jia Pu]

Created attachment 88315 [details]
Patch (v4)

Resovled conflict with TOT. Rearranged some code to fix build failure on SnowLeopard.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 88315 [details]:

media/invalid-media-url-crash.html bug 51138 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 88315 [details]
Patch (v4)

Rejecting attachment 88315 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1

Last 500 characters of output:
M	Source/WebKit/chromium/public/WebContextMenuData.h
r83008 = aaf08a4819113308ab54f5e0c3073aa0f941412b (refs/remotes/trunk)
	M	Tools/ChangeLog
	M	Tools/Scripts/webkitpy/layout_tests/port/chromium.py
	M	Tools/Scripts/webkitpy/layout_tests/port/test.py
	M	Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py
r83009 = 8487ee9f890e2780cd7a6371494ec208403cad35 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8341030

[Jia Pu]

Created attachment 88409 [details]
Patch (v5)

Resovled conflict with Added missing "Reviewed by ..." line in previous patch.

[Jia Pu]

(In reply to comment #13)
> Created an attachment (id=88409) [details]
> Patch (v5)
> 
> Resovled conflict with Added missing "Reviewed by ..." line in previous patch.

Correction, it should read: Added missing "Reviewed by ..." line in previous patch.

[WebKit Commit Bot]

Comment on attachment 88409 [details]
Patch (v5)

Clearing flags on attachment: 88409

Committed r83060: <http://trac.webkit.org/changeset/83060>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.