Bug 57665 - [Mac] WebCore need to notify AppKit spell checker after user has modified autocorrected text.
Summary: [Mac] WebCore need to notify AppKit spell checker after user has modified aut...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Jia Pu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-01 14:56 PDT by Jia Pu
Modified: 2011-04-06 09:10 PDT (History)
6 users (show)

See Also:


Attachments
Patch (v1) (20.90 KB, patch)
2011-04-01 16:22 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v2) (20.94 KB, patch)
2011-04-04 09:02 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v3) (21.69 KB, patch)
2011-04-04 14:23 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v4) (23.37 KB, patch)
2011-04-05 15:05 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v5) (23.41 KB, patch)
2011-04-06 06:48 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 2011-04-01 14:56:34 PDT
<rdar://problem/7350477>
Comment 1 Jia Pu 2011-04-01 16:22:21 PDT
Created attachment 87937 [details]
Patch (v1)
Comment 2 Early Warning System Bot 2011-04-01 16:47:09 PDT
Attachment 87937 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8185320
Comment 3 WebKit Review Bot 2011-04-01 16:58:49 PDT
Attachment 87937 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8320314
Comment 4 Darin Adler 2011-04-01 17:37:43 PDT
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.
Comment 5 Jia Pu 2011-04-04 09:02:09 PDT
Created attachment 88065 [details]
Patch (v2)

Fixed a build failure.
Comment 6 Darin Adler 2011-04-04 09:49:44 PDT
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.
Comment 7 Jia Pu 2011-04-04 14:23:49 PDT
Created attachment 88129 [details]
Patch (v3)
Comment 8 Darin Adler 2011-04-04 16:44:16 PDT
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".
Comment 9 WebKit Commit Bot 2011-04-04 19:09:38 PDT
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
Comment 10 Jia Pu 2011-04-05 15:05:12 PDT
Created attachment 88315 [details]
Patch (v4)

Resovled conflict with TOT. Rearranged some code to fix build failure on SnowLeopard.
Comment 11 WebKit Commit Bot 2011-04-05 19:36:50 PDT
The commit-queue encountered the following flaky tests while processing attachment 88315 [details]:

media/invalid-media-url-crash.html bug 51138 (authors: inferno@chromium.org and jamesr@chromium.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2011-04-05 19:38:46 PDT
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
Comment 13 Jia Pu 2011-04-06 06:48:49 PDT
Created attachment 88409 [details]
Patch (v5)

Resovled conflict with Added missing "Reviewed by ..." line in previous patch.
Comment 14 Jia Pu 2011-04-06 06:49:58 PDT
(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.
Comment 15 WebKit Commit Bot 2011-04-06 09:09:53 PDT
Comment on attachment 88409 [details]
Patch (v5)

Clearing flags on attachment: 88409

Committed r83060: <http://trac.webkit.org/changeset/83060>
Comment 16 WebKit Commit Bot 2011-04-06 09:10:01 PDT
All reviewed patches have been landed.  Closing bug.