[webkit-reviews] review denied: [Bug 44114] [Qt] Missing spell check support : [Attachment 103087] proposed patch V

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Dawit A. <adawit at kde.org>'s
request for review:
Bug 44114: [Qt] Missing spell check support
https://bugs.webkit.org/show_bug.cgi?id=44114

Attachment 103087: proposed patch V
https://bugs.webkit.org/attachment.cgi?id=103087&action=review

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


> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:906
> + *	NOTE: This code is completely based upon the one from
> + *	Source/WebCore/platform/graphics/cairo/DrawErrorUnderline.{h|cpp}

Hehe, one thing not to review :)

> Source/WebKit/qt/Api/qwebkitplatformplugin.h:25
> +#include "qwebkitglobal.h"
> +

Why does this plugin require QWEBKIT_EXPORT and not the others?

(My guess was they all need it, but the other plugins works. What's up with
that?)

> Source/WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:141
> +QWebSpellChecker* QtPlatformPlugin::createSpellChecker()
> +{
> +    QWebKitPlatformPlugin* p = plugin();
> +    return p ?
static_cast<QWebSpellChecker*>(p->createExtension(QWebKitPlatformPlugin::SpellC
hecker)) : 0;
> +}

You should return a PassOwnPtr<QWebSpellChecker> like the other functions, that
would be a lot cleaner for the caller.

I would also prefer a real variable name, like "platformPlugin", instead of p.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:47
> +{
> +    m_spellChecker = nullptr;
> +}

No need to do that, the destructor of OwnPtr cleans the memory for you.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:83
> +String TextCheckerClientQt::getAutoCorrectSuggestionForMisspelledWord(const
String& misspelledWord)
> +{
> +    notImplemented();
> +    return String();
> +}
> +
> +void TextCheckerClientQt::checkGrammarOfString(const UChar*, int length,
Vector<GrammarDetail>&, int*, int*)
> +{
> +    notImplemented();
> +}

I think you should add those to the plugin unless you have a good reason.
If those methods were useful for a port, they are likely useful on one of the
platform Qt supports.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:92
> +    for (int i = 0, count = guessesList.count(); i < count; ++i)

I would prefer count = guessesList.count() to have its own variable outside the
for() for readability.
It would also be a good idea to resize the vector "guesses" to the final size
before the loop in order to avoid realloc during the loop.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:120
> +    return (m_spellChecker ? true : false);

return !!m_spellChecker;

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.h:35
> +#include "TextCheckerClient.h"
> +#include "qwebkitplatformplugin.h"
> +
> +#include <wtf/Forward.h>

You should also explicitely include OwnPtr.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.h:38
> +class QWebSpellChecker;
> +

This is not needed. You must #include "qwebkitplatformplugin.h" due to the
OnwPtr (because the destructor of the object is called by OwnPtr).