Bug 105034

Created attachment 179990 [details] Patch Implementation of Qt WebKit2 spell checking support

Comment on attachment 179990 [details] Patch Attachment 179990 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15402311

build log: In file included from /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebshared.cpp:26: /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h:63: error: a class-key must be used when declaring a friend /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h:63: error: friend declaration does not name a class or function /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h: In member function 'void QWebShared::setTextChecker(QWebTextChecker*)': /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h:59: error: 'void QWebTextChecker::setWK()' is private /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebshared.cpp:51: error: within this context ICECC[7224] 23:12:32: Compiled on 10.109.115.1 make[3]: *** [.obj/release-shared/UIProcess/API/qt/qwebshared.o] Error 1 make[3]: *** Waiting for unfinished jobs.... In file included from /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker.cpp:24: /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h:63: error: a class-key must be used when declaring a friend /home/oszi/WebKit/Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h:63: error: friend declaration does not name a class or function ICECC[7227] 23:12:33: Compiled on 10.109.205.1 make[3]: *** [.obj/release-shared/UIProcess/API/qt/qwebtextchecker.o] Error 1 make[3]: Leaving directory `/home/oszi/WebKit/WebKitBuild/Release/Source/WebKit2' make[2]: *** [sub-Target-pri-make_first-ordered] Error 2 make[2]: Leaving directory `/home/oszi/WebKit/WebKitBuild/Release/Source/WebKit2' make[1]: *** [sub-Source-WebKit2-WebKit2-pro-make_first-ordered] Error 2 make[1]: Leaving directory `/home/oszi/WebKit/WebKitBuild/Release' make: *** [incremental] Error 2

Created attachment 180127 [details] Patch Implementation of Qt WebKit2 spell checking support

Comment on attachment 180127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180127&action=review Good stuff, but some things are a bit confusing. > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:323 > + QWebShared* shared() const; It seems counterintuitive to me that shared settings/objects is a property of a view object. Is there somewhere more global it could be exported? > Source/WebKit2/UIProcess/API/qt/qwebtextchecker.cpp:50 > +bool QWebTextChecker::isSpellCheckingEnabled() const > +{ > + return m_spellCheckingEnabled; > +} > + > +bool QWebTextChecker::isSpellCheckingAllowed() const > +{ > + return false; > +} I think this API naming is confusing. It appears isSpellCheckingEnabled, tells whether spell-checking is user-enabled, while isSpellCheckingAllowed tells if it system available. Perhaps isSpellCheckingAllowed would be better named as isSpellCheckingPossible/Available or something that better represent what it is?

(In reply to comment #5) > > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:323 > > + QWebShared* shared() const; > > It seems counterintuitive to me that shared settings/objects is a property of a view object. Is there somewhere more global it could be exported? Well, the other way I found to do this is adding the shared object as a context property: engine->rootContext()->setContextProperty( QLatin1String("experimentalWebShared"), QWebShared::instance()); Though the way to use it wouldn't be specially clean AFAIK (I still don't know a lot about QML). We could bind some javascript code to completed signal, so that it sets the text checker and enables it. Less strange than setting it as a property of the view object, but also less convenient API. Any better idea for this? > > +bool QWebTextChecker::isSpellCheckingAllowed() const > > +{ > > + return false; > > +} > > I think this API naming is confusing. It appears isSpellCheckingEnabled, tells whether spell-checking is user-enabled, while isSpellCheckingAllowed tells if it system available. Perhaps isSpellCheckingAllowed would be better named as isSpellCheckingPossible/Available or something that better represent what it is? Yes, it is somehow confusing. This is more or less a mapping of the names used internally in WebKit. But we'd likely better replace them with something more simple: Q_PROPERTY(bool available READ isAvailable) Q_PROPERTY(bool enabled READ isEnabled WRITE setEnabled) Same applies to updateSpellCheckingLanguages, that could be: Q_PROPERTY(QList<QString> languages WRITE setLanguages) The idea of these changes is, of course, being less confusing, but also remove "spellchecking" in the wording.

Comment on attachment 180127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180127&action=review >> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:323 >> + QWebShared* shared() const; > > It seems counterintuitive to me that shared settings/objects is a property of a view object. Is there somewhere more global it could be exported? We have a Qt global object, how about having a QtWebKit global object that would be added when the QtWebKit QML module is imported? It would be nice too if this global object could be accessed through a static instance if the user wants to use C++ instead of QML. > Source/WebKit2/UIProcess/API/qt/qwebtextchecker_p.h:41 > + Q_PROPERTY(bool enabled READ isSpellCheckingEnabled WRITE setSpellCheckingEnabled) What is the use case for anything more than an "enabled" API? The current language and the spell checker engine isn't something that applications should be worrying about, can't we fetch this from under through the platform? I would also be in favor of a simpler API for the first version, please keep in mind that once we released this we have to support it until Qt6 which won't happen anytime soon.

(In reply to comment #7) > We have a Qt global object, how about having a QtWebKit global object that would be added when the QtWebKit QML module is imported? > It would be nice too if this global object could be accessed through a static instance if the user wants to use C++ instead of QML. See http://doc.qt.digia.com/qt/qdeclarativeglobalobject.html

(In reply to comment #7) > We have a Qt global object, how about having a QtWebKit global object that would be added when the QtWebKit QML module is imported? > It would be nice too if this global object could be accessed through a static instance if the user wants to use C++ instead of QML. Sounds good. I'll create a webkit global object for accessing text checker. Another point I'll change is the "feature" that allows to change the selected spell checker from QML. So my idea is not exposing this. At C++, we'll still be able to replace the text checker with a different implementation (a requirement in my environment). > I would also be in favor of a simpler API for the first version, please keep in mind that once we released this we have to support it until Qt6 which won't happen anytime soon. Yes, you are right. I was exposing almost everthing that is available in webkit API. Actually, from a UI we can expect people enabling/disabling spell checker, and also setting languages (i.e. to implement a UI for choosing the languages to check). It is also important to be able to retrieve currently available spell checking languages for such a UI. My new proposal for API would be: * New QML global object webkit (that would expose QWebShared). * Rename C++ QWebShared to QWebGlobal (more consistent with other API's). * New QML global object webkit.textChecker (that would expose current QWebTextChecker object). This object can be replaced in C++ by a different one with same API that exists now: QWebGlobal::setTextChecker * API for enabling/disabling text checker: QWebTextChecker::setEnabled. In QML it would be webkit.textChecker.enabled * API for setting the current text checker languages: QWebTextChecker::setLanguages. In QML it would be webkit.textChecker.languages * NEW! API for getting the current text checker _available_ languages: QWebTextChecker::availableLanguages. In QML it would be webkit.textChecker.availableLanguages. * By default, languages would default to the dictionary that matches user environment. And enabled would default to false.

(In reply to comment #9) > (In reply to comment #7) > > We have a Qt global object, how about having a QtWebKit global object that would be added when the QtWebKit QML module is imported? > > It would be nice too if this global object could be accessed through a static instance if the user wants to use C++ instead of QML. > > Sounds good. I'll create a webkit global object for accessing text checker. Another point I'll change is the "feature" that allows to change the selected spell checker from QML. So my idea is not exposing this. At C++, we'll still be able to replace the text checker with a different implementation (a requirement in my environment). > > > I would also be in favor of a simpler API for the first version, please keep in mind that once we released this we have to support it until Qt6 which won't happen anytime soon. > > Yes, you are right. I was exposing almost everthing that is available in webkit API. Actually, from a UI we can expect people enabling/disabling spell checker, and also setting languages (i.e. to implement a UI for choosing the languages to check). It is also important to be able to retrieve currently available spell checking languages for such a UI. > > My new proposal for API would be: > * New QML global object webkit (that would expose QWebShared). > * Rename C++ QWebShared to QWebGlobal (more consistent with other API's). > * New QML global object webkit.textChecker (that would expose current QWebTextChecker object). This object can be replaced in C++ by a different one with same API that exists now: QWebGlobal::setTextChecker Why exactly do we need a new global object when functionality wise as a developer using this API I just want to enable spell checking. Why do I have to instantiate a new type or re-use some global object to say "yes please" ? > * API for enabling/disabling text checker: QWebTextChecker::setEnabled. In QML it would be webkit.textChecker.enabled > * API for setting the current text checker languages: QWebTextChecker::setLanguages. In QML it would be webkit.textChecker.languages > * NEW! API for getting the current text checker _available_ languages: QWebTextChecker::availableLanguages. In QML it would be webkit.textChecker.availableLanguages. > * By default, languages would default to the dictionary that matches user environment. And enabled would default to false. Perhaps this is going into the area of settings, something we do need to tackle for the next release :)

(In reply to comment #10) > > * New QML global object webkit.textChecker (that would expose current QWebTextChecker object). This object can be replaced in C++ by a different one with same API that exists now: QWebGlobal::setTextChecker > > Why exactly do we need a new global object when functionality wise as a developer using this API I just want to enable spell checking. Why do I have to instantiate a new type or re-use some global object to say "yes please" ? Typically by default this is disabled, not enabled. Examples: Chrome and Firefox. It is enabled after user sets it. The most bad part of all this is that this is also a global setting, not a webview setting nor context setting. > > * By default, languages would default to the dictionary that matches user environment. And enabled would default to false. > > Perhaps this is going into the area of settings, something we do need to tackle for the next release :) Just checking the Qt4 API, actually this makes a lot of sense: QWebSettings is associated to each context, but it also includes some global variables accessed through static methods. I wouldn't mind moving this "QWebGlobal" and name it QWebSettings, changing the behavior to this. The only remaining question is how to expose this in QML (there are not static methods). So, we could just have QWebSettings, the same way it worked in qt4, associated to web contexts. But we could still have a global object for accessing the static preferences in QML: webkit.textCheckerEnabled, webkit.textCheckerLanguages, webkit.textCheckerAvailableLanguages, that would just wrap the access to the static methods of QWebSettings. This way we would minimize the access to the spellcheck objects in QML and expose only the API we need in UI.

Comment on attachment 180127 [details] Patch Simon looks unconvinced by the API. r-ing so that it does not linger around in the review queue. Please reset to r? if I misunderstood the state of this.

Created attachment 182561 [details] Patch Expose text checker through a global object, and add a way to get available languages

Quick comment on the way: you may want to namespace your internal classes(?). I let Simon do a real review :)

Comment on attachment 182561 [details] Patch Removing from review after discussion on mailing list. I'll send a new version of the patch today.

Created attachment 183938 [details] Patch Removed all the C++ API for changing or accessing the text checker. Now QtTextChecker directly uses TextCheckerEnchant implementation. Also, now QWebGlobal - webkit var offers settings for changing text checker instead of offering direct access to text checker.

Comment on attachment 183938 [details] Patch Attachment 183938 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16038451

Created attachment 183950 [details] Patch Disabled spellcheck by default, and put QtTextChecker in WebKit namespace

Comment on attachment 183950 [details] Patch Attachment 183950 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16041525

Created attachment 183983 [details] Patch Fix build without spellcheck

Comment on attachment 183983 [details] Patch 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.

Comment on attachment 183983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183983&action=review > Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:51 > + : m_enabled(true) > + , m_textChecker(WebCore::TextCheckerEnchant::create()) > +{ > + m_textChecker->updateSpellCheckingLanguages(Vector<String>()); > + setWK(); A few more comments: - How expensive is the text checker? Most people won't need it in their application, so I think it would make sense to have it disabled by default and the text checker should only be instanciated if it gets enabled. - Could you try feeding QLocale::system().name() to the default list of languages if it's available?

(In reply to comment #21) > Here are some comments about the patch if you're still willing to iterate on it. Sure! > > 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) Yes, it makes sense. > > > Source/WebKit2/UIProcess/API/qt/qwebglobal.cpp:44 > > +QWebGlobal* QWebGlobal::instance() > > Preferably, the instance would be owned by the QtWebContext instead. So we would have a QWebGlobal per context? I implemented this as a singleton as the properties are global to all QtWebKit views and contexts. > > 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". Yeah, I used text as it is the wording in WebCore. But the usual concept here (and in HTML) is spellchecking. > > Source/WebKit2/UIProcess/API/qt/qwebglobal_p.h:47 > > + explicit QWebGlobal(); > > Isn't the explicit keyword redundant? Yes. > > > Source/WebKit2/UIProcess/qt/QtTextChecker.cpp:44 > > +QtTextChecker* QtTextChecker::instance() > > This should also be owned by QtWebContext. Same thing here. We only have one text checker application wide. If we have one text checker per context, we would be replacing text checkers on creating a text checker in a new context. > > 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? My opinion now is that it's not specially useful. As we are moving to expose the WK2 C API, likely the best solution would be just doing the minimal to make QtWebKit use the spellchecker enabled through C API, and let the API user implement the text checker they want. > > 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)? This would make a lot of sense, yes. We just want the wk to use the C API enabled text checker.

(In reply to comment #23) > > > Source/WebKit2/UIProcess/API/qt/qwebglobal.cpp:44 > > > +QWebGlobal* QWebGlobal::instance() > > > > Preferably, the instance would be owned by the QtWebContext instead. > > So we would have a QWebGlobal per context? I implemented this as a singleton as the properties are global to all QtWebKit views and contexts. Currently QML will only be in contact with QtWebContext::defaultContext, so you could use this one. There is no plan to allow more than one QtWebContext, for non-testing uses at least.

(In reply to comment #24) > (In reply to comment #23) > > > > Source/WebKit2/UIProcess/API/qt/qwebglobal.cpp:44 > > > > +QWebGlobal* QWebGlobal::instance() > > > > > > Preferably, the instance would be owned by the QtWebContext instead. > > > > So we would have a QWebGlobal per context? I implemented this as a singleton as the properties are global to all QtWebKit views and contexts. > > Currently QML will only be in contact with QtWebContext::defaultContext, so you could use this one. > There is no plan to allow more than one QtWebContext, for non-testing uses at least. I've been thinking about it a bit, and maybe ultimately QtWebContext and QWebGlobal should be the same class. The have the same lifetime and will already be bound together. The name of the final class should be QWebContext (no 't' after 'Q'). So it would be great if you can use this name, and we can later on merge QtWebContext into QWebContext.

(In reply to comment #25) > I've been thinking about it a bit, and maybe ultimately QtWebContext and QWebGlobal should be the same class. The have the same lifetime and will already be bound together. > The name of the final class should be QWebContext (no 't' after 'Q'). > So it would be great if you can use this name, and we can later on merge QtWebContext into QWebContext. After discussing with Tor Arne, having properties/functions directly in "QtWebKit" might not be the cleanest approach if we want to see "QtWebKit" as some kind of namespace in QML. Instead, we could have "QtWebKit.globalPreferences", "QtWebKit.downloads", etc. So please ignore my comment about QtWebContext. Moreover, maybe you could name your new class QWebGlobalPreferences, and make it available through QtWebKit.globalPreferences using something like: qtWebKit = QQmlEngine::newObject(); qtWebKit.setProperty("globalPreferences", new QWebGlobalPreferences); engine.setProperty("QtWebKit", qtWebKit); Tell me if you see any issue with it, sorry for going back and forth.

(In reply to comment #26) > After discussing with Tor Arne, having properties/functions directly in "QtWebKit" might not be the cleanest approach if we want to see "QtWebKit" as some kind of namespace in QML. > Instead, we could have "QtWebKit.globalPreferences", "QtWebKit.downloads", etc. > > So please ignore my comment about QtWebContext. Moreover, maybe you could name your new class QWebGlobalPreferences, and make it available through QtWebKit.globalPreferences using something like: Uhm, makes sense this way of exposing it. It is very explicit about these properties being global, and that's good to avoid confusion. > > qtWebKit = QQmlEngine::newObject(); > qtWebKit.setProperty("globalPreferences", new QWebGlobalPreferences); > engine.setProperty("QtWebKit", qtWebKit); > > Tell me if you see any issue with it, sorry for going back and forth. I'll go back to this in next weeks and prepare a proper implementation taking all this into account.

Qt port is not in WebKit anymore. Won't fix.