[webkit-reviews] review denied: [Bug 105034] [Qt][WK2] Add spell checking support : [Attachment 183983] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied José Dapena Paz
<jdapena at igalia.com>'s request for review:
Bug 105034: [Qt][WK2] Add spell checking support
https://bugs.webkit.org/show_bug.cgi?id=105034

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

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=183983&action=review


Sorry for the long delay to check this patch. There has been some changes going
on around WK2 that we're currently trying to deal with as you might have seen
yourself.
Here are some comments about the patch if you're still willing to iterate on
it.

> Source/WebKit/qt/declarative/experimental/plugin.cpp:58
> +	   engine->rootContext()->setContextProperty(QLatin1String("webkit"),
QWebGlobal::instance());

The Qt global object is defined as "Qt", so unless this creates some clash with
the QtWebKit import, I would call the global object "QtWebKit". (we should
probably discuss about this)

> Source/WebKit2/UIProcess/API/qt/qwebglobal.cpp:44
> +QWebGlobal* QWebGlobal::instance()

Preferably, the instance would be owned by the QtWebContext instead.

> Source/WebKit2/UIProcess/API/qt/qwebglobal_p.h:30
> +    Q_PROPERTY(bool textCheckerEnabled READ textCheckerEnabled WRITE
setTextCheckerEnabled NOTIFY textCheckerEnabledChanged)

I think that all the user-visible properties should be "spell" checkers instead
of "text".

> Source/WebKit2/UIProcess/API/qt/qwebglobal_p.h:31
> +    Q_PROPERTY(QList<QString> textCheckerLanguages READ textCheckerLanguages
WRITE setTextCheckerLanguages);

You forgot "NOTIFY textCheckerLanguagesChanged".

> Source/WebKit2/UIProcess/API/qt/qwebglobal_p.h:40
> +    virtual void setTextCheckerLanguages(const QList<QString>& languages);
> +    virtual QList<QString> textCheckerLanguages() const;
> +    virtual QList<QString> textCheckerAvailableLanguages() const;

QList<QString> -> QStringList
No need for virtual.

> Source/WebKit2/UIProcess/API/qt/qwebglobal_p.h:47
> +    explicit QWebGlobal();

Isn't the explicit keyword redundant?

> Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:44
> +QtTextChecker* QtTextChecker::instance()

This should also be owned by QtWebContext.

> Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:84
> +void QtTextChecker::learnWord(const QString& word)

There quite a lot of string conversion going on:
WTF::String -> WebString -> WKString -> QString -> (back again to WebCore)
WTF::String -> (finally to enchant) utf8 char*

It feels like a layer violation to get out of the WK2 C API to go back to
WebCore to use a convenience class there.
I wonder if the use of the 200-lines TextCheckerEnchant is really worth it,
what do you think?

> Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:147
> +   
toTextChecker(clientInfo)->checkSpellingOfString(WebKit::toImpl(text)->string()
, *misspellingLocation, *misspellingLength);

We are now doing directly from the static callback instead of delegating to an
instance method, so it would be nice if you could apply this pattern directly.
See the use of WKPageLoaderClient in qquickwebview.cpp for an example.

> Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:159
> +	   WKRetainPtr<WKStringRef> wkSuggestion(AdoptWK,
WKStringCreateWithQString(guess));
> +	   WKArrayAppendItem(wkSuggestions, wkSuggestion.get());

WKRetainPtr foo = adoptWK(WKCreate...()) should be used instead.
Or you could add it directly to the array:
WKArrayAppendItem(wkSuggestions,
adoptWK(WKStringCreateWithQString(guess)).get());

> Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:190
> +    WKTextCheckerSetClient(&textCheckerClient);

I would do this directly in the constructor.

> Source/WebKit2/UIProcess/qt/QtTextChecker.h:29
> +    virtual void checkSpellingOfString(const QString&, int&
misspellingLocation, int& misspellingLength);
> +    virtual QVector<QString> getGuessesForWord(const QString& word);
> +    virtual void learnWord(const QString& word);
> +    virtual void ignoreWord(const QString& word);
> +
> +    virtual bool enabled() const;
> +    virtual void setEnabled(bool);
> +    virtual void setLanguages(const QList<QString>&);
> +    virtual QList<QString> languages() const;
> +    virtual QList<QString> availableLanguages() const;

Those shouldn't be virtual.

> Source/WebKit2/UIProcess/qt/QtTextChecker.h:36
> +    OwnPtr<WebCore::TextCheckerEnchant> m_textChecker;

This might go above the C API eventually, so please use QScopedPointer instead.


> Source/WebKit2/UIProcess/qt/TextCheckerQt.cpp:30
> -#include <WebCore/NotImplemented.h>
> +#include "WebTextChecker.h"

TextCheckerQt.cpp looks almost identical to TextCheckerGTK.cpp.
Any way we could avoid the duplication (something like having TextChecker.cpp
with a big #if !PLATFORM(MAC) in it)?

> Tools/qmake/mkspecs/features/features.pri:100
> +    ENABLE_SPELLCHECK=0 \

If we have API for it, I think that it should be enabled by default.