[webkit-reviews] review denied: [Bug 44114] [Qt] Missing spell check support : [Attachment 103842] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Lindsay Mathieson
<lindsay.mathieson at gmail.com>'s request for review:
Bug 44114: [Qt] Missing spell check support
https://bugs.webkit.org/show_bug.cgi?id=44114

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103842&action=review


Quick review.

Still no Changelog. :(

Conversion Vector<String> -> QStringList is done twice. It would be nice to
have a static inline function for doing it.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:997
> +	   painter->setPen(originalPen);

You did not change the pen, no need to reset it.

> Source/WebKit/qt/Api/qwebkitplatformplugin.h:145
> +class QWEBKIT_EXPORT QWebSpellCheckerGrammarDetail {
> +public:
> +    int location;
> +    int length;
> +    QStringList guesses;
> +    QString userDescription;
> +};

This could be defined in QWebSpellChecker

> Source/WebKit/qt/QtWebKit.pro:198
> +    $$PWD/WebCoreSupport/WebPlatformStrategies.cpp \
> +    $$PWD/WebCoreSupport/TextCheckerClientQt.cpp

Filenames are usually sorted alphabetically in pro/pri file.

> Source/WebKit/qt/QtWebKit.pro:215
> -    $$PWD/WebCoreSupport/WebPlatformStrategies.h
> +    $$PWD/WebCoreSupport/WebPlatformStrategies.h \
> +    $$PWD/WebCoreSupport/TextCheckerClientQt.h

Ditto

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:82
> +    
> +    

One empty line too many.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:83
> +    // Do Grammer Check

Comments must be sentences in WebKit, starting by a upper case character and
ending by a point.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:84
> +    QVector<QWebSpellCheckerGrammarDetail>  qWebDetails;

Two spaces between the type and the variable.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:87
> +    // copy qWebDetails to details

Uppercase C for copy and a period at the end of the sentence.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:90
> +    while (it < qWebDetails.end())

A for() loop is preferred to a while() loop if you go over each element of the
collection.
It is also preferred to keep a variable with the end iterator so it is not
created at each iteration.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:92
> +	   GrammarDetail gd;

Not a fan of gd for the variable name. The name "grammarDetail" is fine.