[webkit-reviews] review granted: [Bug 106815] [Chromium] Tests and fixes for spell checker behavior : [Attachment 185547] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Rouslan Solomakhin
<rouslan+webkit at chromium.org>'s request for review:
Bug 106815: [Chromium] Tests and fixes for spell checker behavior
https://bugs.webkit.org/show_bug.cgi?id=106815

Attachment 185547: Patch
https://bugs.webkit.org/attachment.cgi?id=185547&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=185547&action=review


Some minor style nits, but this looks OK overall.  Note that the Qt, Gtk and
Efl bots don't run tests, so these tests may fail on those ports too.  You can
either suppress the errors in TestExpectations or land them and watch the bots
on build.webkit.org.

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:296
> +		       // If there is no selection, then select the
misspelling.

WebKit style is to avoid comments that only describe the code.	Most comments
are for answering 'why' the code is there. I would remove this comment.

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:76
> +	   int maxWordLength = stringText.length() - wordOffset;
> +	   int wordLength;

Don't you need to static_cast length() to an int?  I think some compilers will
complain about this signed/unsigned conversion.  Should maxWordLength and
wordLength be unsigned?

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:77
> +	   WTF::String word;

Nit: No WTF:: needed.

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:85
> +	       wordLength = m_misspelledWords.at(i).length() > maxWordLength ?
maxWordLength : m_misspelledWords.at(i).length();

I'm surprised this doesn't complain about signed/unsigned either.