[webkit-reviews] review denied: [Bug 50137] Support multiple correction candidates panel for misspelled word on Mac OS X. : [Attachment 75037] Proposed patch (v2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied jpu at apple.com's request for review:
Bug 50137: Support multiple correction candidates panel for misspelled word on
Mac OS X.
https://bugs.webkit.org/show_bug.cgi?id=50137

Attachment 75037: Proposed patch (v2)
https://bugs.webkit.org/attachment.cgi?id=75037&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(replacemen
tRange.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 *.