Bug 50137 – Support multiple correction candidates panel for misspelled word on Mac OS X.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

This patch is to bring WebKit editing into parity with that in AppKit. When caret is moved to the end of a word that is marked as misspelling, we will show a panel containing one or more suggestions. User can use left and right arrow key to choose the desired candidate. He can also dismiss the panel by pressing ESC key.

[Jia Pu]

<rdar://problem/8568059>

[Jia Pu]

Created attachment 74986 [details]
Proposed patch (v1)

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Build Bot]

Attachment 74986 [details] did not build on win:
Build output: http://queues.webkit.org/results/6322095

[Jia Pu]

Created attachment 75037 [details]
Proposed patch (v2)

Updated the patch to make the compiler on non-Mac platforms happy.

[Early Warning System Bot]

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

[Darin Adler]

Comment on attachment 75037 [details]
Proposed patch (v2)

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

Looks good.

I’m going to say review- because there are a lot of small things worth improving, but it’s close to a review+.

> WebCore/editing/Editor.cpp:95
> +// This is for stopping compilers on non-Mac platform from complaing about unused functions, since these functions are used on Mac.

This comment is confusing. It’s true that these autocorrection-panel-only functions need to be inside an if, but no comment is needed.

Typo: "complaing".

> WebCore/editing/Editor.cpp:97
> +static const Vector<DocumentMarker::MarkerType>& getMarkerTypesForAutoCorrection()

In WebKit we don’t name this kind of function “get”. Also, autocorrection is a single word. The name should be markerTypesForAutocorrection().

> WebCore/editing/Editor.cpp:100
> +    DEFINE_STATIC_LOCAL(Vector<DocumentMarker::MarkerType>, markerTypesForAutoCorrection, ());
> +    if (!markerTypesForAutoCorrection.size()) {

Better to use isEmpty() instead of size() for this. There’s also an even cleaner way to do it without an if statement by using a separate function that returns the vector, but I think it’s OK to leave it this way since other call sites don’t seem to use that way yet either.

> WebCore/editing/Editor.cpp:114
> +    Vector<FloatQuad>::const_iterator end = textQuads.end();
> +    FloatRect totalBoundingBox;
> +    for (Vector<FloatQuad>::const_iterator it = textQuads.begin(); it < end; ++it)
> +        totalBoundingBox.unite(it->boundingBox());

Generally we don’t iterate vectors using iterators. Instead we just use indexing. The iterators are provided for use when the algorithm is generic. Code should be more like this:

     size_t size = textQuads.size();
     for (size_t i = 0; i < size; ++i)
        totalBoundingBox.unite(textQuads[i].boundingBox());

> WebCore/editing/Editor.cpp:119
> +static const Vector<DocumentMarker::MarkerType>& getMarkerTypesForReplacement()

Same comment about function name.

> WebCore/editing/Editor.cpp:122
> +    if (!markerTypesForReplacement.size())

isEmpty rather than size.

> WebCore/editing/Editor.cpp:535
> +        if (((marker.type == DocumentMarker::CorrectionIndicator && marker.description.length() > 0) || marker.type == DocumentMarker::Spelling) && static_cast<int>(marker.endOffset) == endOffset) {

In WebKit code we would normally say just length() rather than length() > 0.

> WebCore/editing/Editor.cpp:538
> -            if (currentWord.length() > 0 && marker.description.length() > 0) {
> +            if (currentWord.length() > 0) {

In WebKit code we would normally say just length() rather than length() > 0.

> WebCore/editing/Editor.cpp:546
> +                if (marker.type == DocumentMarker::CorrectionIndicator) {
> +                    m_correctionPanelInfo.m_replacementString = marker.description;
> +                    startCorrectionPanelTimer(CorrectionPanelInfo::PanelTypeReversion);
> +                } else {
> +                    startCorrectionPanelTimer(CorrectionPanelInfo::PanelTypeSpellingSuggestions);
> +                }

We never put braces around single line bodies. Some people would reverse the if expression so the non-braced half would come first, but the braces violate the style rule.

> WebCore/editing/Editor.cpp:2480
> +        FloatRect boundingBox = boundingBoxForRange(m_correctionPanelInfo.m_rangeToBeReplaced.get());

It would be better to compute the bounding box inside the if statement so it’s computed only if needed.

> WebCore/editing/Editor.cpp:2482
> +        TextCheckingParagraph paragraph(m_correctionPanelInfo.m_rangeToBeReplaced);
> +        String paragraphText = plainText(paragraph.paragraphRange().get());

It might be easier to read this if these two lines were collapsed into a single line. The local variable doesn’t help make things clearer.

> WebCore/editing/Editor.cpp:2485
> +        if (suggestions.size() > 0) {

This should probably be !suggestions.isEmpty().

Also, you could use break to avoid having to nest the rest of the code.

    if (suggestions.isEmpty())
        break;

> WebCore/editing/Editor.cpp:2486
> +            String topSuggestion = suggestions[0];

Could use suggestions.first() here instead if you like. Either way is probably OK.

> WebCore/editing/Editor.cpp:2490
> +            client()->showCorrectionPanel(m_correctionPanelInfo.m_panelType, boundingBox, m_correctionPanelInfo.m_replacedString,
> +                                          topSuggestion, suggestions, this);

We don’t indent subsequent lines like this. Putting the entire statement on one line is one way to handle it. Another is to indent just one level (4 spaces).

> WebCore/editing/Editor.cpp:2495
> +    default:
> +        break;

We normally omit a default case so the compiler can warn us if we forgot any of the cases in the switch.

> WebCore/editing/Editor.cpp:2729
> +        Vector<DocumentMarker::MarkerType>::const_iterator it = markerTypesToAdd.begin();
> +        Vector<DocumentMarker::MarkerType>::const_iterator end = markerTypesToAdd.end();
> +        for (; it != end; ++it)
> +            replacementRange->startContainer()->document()->markers()->addMarker(replacementRange.get(), *it, m_correctionPanelInfo.m_replacementString);

Again, this sort of thing normally reads better with indexing for vectors. I think it makes sense to put the markers() pointer into a local variable to hoist it out of the loop:

    DocumentMarkerController* markers = replacementRange->startContainer()->document()->markers();
    size_t size = markerTypesToAdd.size();
    for (size_t i = 0; i < size; ++i)
        markers->addMarker(replacementRange.get(), markerTypesToAdd[i], m_correctionPanelInfo.m_replacementString);

> WebCore/editing/Editor.h:316
> +    // If user confirmed a correction in the correction panel, correction has non-zero length; other wise it means that user has dismissed the panel.

I believe it is incorrect to use a semicolon here. Instead you should use a comma.

Typo: “other wise” instead of “otherwise”.

> WebCore/page/EditorClient.h:204
> -    virtual void getGuessesForWord(const String&, Vector<String>& guesses) = 0;
> +    virtual void getGuessesForWord(const String& word, Vector<String>& guesses) = 0;
> +    // Some spellchecker may be able to take advantage of paragraph context to more accurately identify the language.
> +    virtual void getGuessesForWord(const String& word, const String& context, Vector<String>& guesses) = 0;

Should probably be “spellcheckers” or spelling checkers”.

If both functions are going to be pure virtual, it doesn’t make sense to keep both of them. Since they are pure virtual all implementers of EditorClient have to be revised anyway. Callers can simply ignore the context argument.

I suggest adding the context argument and removing the comment. I don’t think the string “context” makes it clear enough exactly what the argument is. The comment could be used to explain what exactly “context” consists of.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:885
> +        default:
> +            // Should never be here.
> +            ASSERT(false);
> +            break;

I’m surprised we are not getting uninitialized variable warnings from the compiler for this code path. Normally we would try to do this in a way that gets us both compile time warnings if we leave out a type and a runtime exception. This can be done by putting the conversion into a function, using return rather than break, using no default case, using ASSERT_NOT_REACHED outside the switch statement, and returning some default value in that case. I can find an example, but you probably can too.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:888
> +    NSMutableArray* alternativeStrings = nil;

Incorrect placement of the * here.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:894
> +        alternativeStrings = [NSMutableArray arrayWithCapacity:alternativeReplacementStrings.size()];
> +        Vector<String>::const_iterator it = alternativeReplacementStrings.begin();
> +        Vector<String>::const_iterator end = alternativeReplacementStrings.end();
> +        for (; it != end; ++it)
> +            [alternativeStrings addObject:(NSString*)*it];

Again, an index-based loop is better for Vector.

Again, incorrect placement of *.

[WebKit Review Bot]

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

[Jia Pu]

Created attachment 75064 [details]
Proposed patch (v3)

Revised according to comment #8. Also fixed more compilation errors on non-Mac platforms.

[Jia Pu]

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

canceling review request. Spotted a couple of more issues that may cause compiler error. Will upload a new patch soon.

[Jia Pu]

Created attachment 75065 [details]
Proposed patch (v4)

[Darin Adler]

Comment on attachment 75065 [details]
Proposed patch (v4)

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

r=me assuming you get it building on all the platforms

> WebCore/editing/Editor.cpp:2517
> +    default:
> +        break;

This should be omitted, sorry I missed it last time.

> WebCore/editing/Editor.cpp:2543
> +    m_correctionPanelInfo.m_rangeToBeReplaced.clear();

All the direct uses of fields in m_correctionPanelInfo make me suspect that it’s not really a class, but more like a struct, and so we should take the m_ prefixes off the data members at some point. Not right now, though. It’s also pretty weak to have “info” in the name; that’s a sort of programmer pause word, like “uh” in public speaking, that doesn’t add much value.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:80
> +static NSCorrectionBubbleType correctionBubbleTypeForCorrectionPanelType(CorrectionPanelInfo::PanelType panelType)

Could mark this inline since it’s only used in one place.

I think you could just mark name this function correctionBubbleType; C++ has overloading so there is no need to include the type of the argument.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:896
> +        switch (bubbleType) {
> +            case NSCorrectionBubbleTypeCorrection:

WebKit coding style calls for lining up the case with switch rather than indenting.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:915
> +            default:
> +                break;

Again, please omit the default so the compiler can catch it if we forget to handle an enum value.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:962
> +    if (context.length() > 0) {

Normally we’d omit the “> 0”.

> WebKit/wince/WebCoreSupport/EditorClientWinCE.cpp:490
> -void EditorClientWinCE::getGuessesForWord(const String& word, WTF::Vector<String>& guesses)
> +    void EditorClientWinCE::getGuessesForWord(const String& word, const String& context, WTF::Vector<String>& guesses)

Looks like the editor indented this for you. Need to fix it.

[Jia Pu]

Created attachment 75068 [details]
Proposed patch (v5)

Fixed two style problems.

[Jia Pu]

Comment on attachment 75068 [details]
Proposed patch (v5)

Didn't see Darin's new comment. Will submit a new patch to address those issues.

[Jia Pu]

Created attachment 75071 [details]
Proposed patch (v6)

Addressed most issues raised in comment #13. And fixed some style problems.

[Jia Pu]

(In reply to comment #13)
> (From update of attachment 75065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75065&action=review
> 
> > WebCore/editing/Editor.cpp:2543
> > +    m_correctionPanelInfo.m_rangeToBeReplaced.clear();
> 
> All the direct uses of fields in m_correctionPanelInfo make me suspect that it’s not really a class, but more like a struct, and so we should take the m_ prefixes off the data members at some point. Not right now, though. It’s also pretty weak to have “info” in the name; that’s a sort of programmer pause word, like “uh” in public speaking, that doesn’t add much value.
> 

In Proposed patch (v6), I have addressed all issues you raised in comment #13 except this one. This type is indeed declared as a struct. I didn't know the conventional usage of m_ prefix here. Regarding the naming, this struct is really a collection of all information relevant to the handling of autocorrection panel. Do you have any suggestion on the naming? CorrectionPanelContext?

[Darin Adler]

(In reply to comment #17)
> This type is indeed declared as a struct. I didn't know the conventional usage of m_ prefix here.

Normally we use m_ for data members in classes, and don’t put any prefix on the names of data members in structures. This is not covered in the WebKit coding style guidelines document, but should be.

> Regarding the naming, this struct is really a collection of all information relevant to the handling of autocorrection panel. Do you have any suggestion on the naming? CorrectionPanelContext?

I can’t think of a better name yet. I don’t think “context” is much better than “info”, and I’m pondering it.

[WebKit Review Bot]

Comment on attachment 75071 [details]
Proposed patch (v6)

Rejecting patch 75071 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Jia Pu]

Darin, please cq+ the latest patch. Thanks.

[WebKit Commit Bot]

Comment on attachment 75071 [details]
Proposed patch (v6)

Rejecting patch 75071 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
XCODE_VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh

** BUILD FAILED **


The following build commands failed:
WebKit:
	CompileC /Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/x86_64/WebEditorClient.o /Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


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

[Darin Adler]

(In reply to comment #21)
> Full output: http://queues.webkit.org/results/6378128

Looks like we have some code that needs Mac OS X version #ifdef:

/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm:80: error: 'NSCorrectionBubbleType' does not name a type
cc1objplus: warnings being treated as errors
/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm: In member function 'virtual void WebEditorClient::getGuessesForWord(const WTF::String&, const WTF::String&, WTF::Vector<WTF::String, 0ul>&)':
/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm:961: warning: 'NSSpellChecker' may not respond to '-languageForWordRange:inString:orthography:'
/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm:961: warning: (Messages without a matching method signature
/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm:961: warning: will be assumed to return 'id' and accept
/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm:961: warning: '...' as arguments.)
/Projects/CommitQueue/WebKit/mac/WebCoreSupport/WebEditorClient.mm:961: warning: cannot pass objects of non-POD type 'const class WTF::String' through '...'; call will abort at runtime

[Jia Pu]

Created attachment 75186 [details]
Proposed patch (v7)

Fixed build failure on older Mac OS.

[WebKit Commit Bot]

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

animations/combo-transform-translate+scale.html
compositing/reflections/nested-reflection-animated.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[Jia Pu]

This patch has been the queue for a day now. Is this normal? Shall I just wait?

[Darin Adler]

Comment on attachment 75186 [details]
Proposed patch (v7)

Clearing flags on attachment: 75186

Committed r73088: <http://trac.webkit.org/changeset/73088>

[Darin Adler]

All reviewed patches have been landed.  Closing bug.