[webkit-reviews] review denied: [Bug 103520] On Linux, should be able to get spelling suggestions without selecting the misspelled word : [Attachment 179744] update WebCore.exp.in

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Grzegorz Czajkowski
<g.czajkowski at samsung.com>'s request for review:
Bug 103520: On Linux, should be able to get spelling suggestions without
selecting the misspelled word
https://bugs.webkit.org/show_bug.cgi?id=103520

Attachment 179744: update WebCore.exp.in
https://bugs.webkit.org/attachment.cgi?id=179744&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179744&action=review


> Source/WebCore/editing/EditingBehavior.h:67
> +#if PLATFORM(EFL) || PLATFORM(GTK)

It's probably better to say !PLATFORM(CHROMIUM) in accordance with the comment.

Or does Qt port not want this behavior?

> Source/WebCore/editing/Editor.cpp:1684
> +// Helper function to determine whether text is a single word.

We prefer having only "why" comments. This comment repeats the code. Please
remove it.

> Source/WebCore/editing/Editor.cpp:1688
> +    return it && textBreakNext(it) == static_cast<int>(text.length());

I don't think you can call wordBreakIterator like this on a fragment of text.
There are cases where wordBreakIterator needs more context than the word itself
to determine whether something is a word or not (e.g. CJK).

> Source/WebCore/editing/Editor.cpp:1693
> +    // Don't do anything if the spelling for the node is forbidden.

Again, this comment says what the code below already tells us. Please remove
it.

> Source/WebCore/editing/Editor.cpp:1708
> +    // Get selected text to check for multiple word selection.
> +    String selectedString = selectedText().stripWhiteSpace();
> +
> +    if (!selectedString.isEmpty()) {
> +	   // Don't provide suggestions for multiple words.
> +	   if (!isASingleWord(selectedString))
> +	       return String();
> +    }

You should use nextWordPosition and so forth to check whether there are more
than one word included in the selection.
r- because of this.

> Source/WebCore/editing/Editor.cpp:1710
> +    // Get the word arround the caret.

Instead of adding a comment like this, please extract the code that extracts
the word around the caret as a function.