Bug 66812 – [Qt] Spell check platform plugin API enhancement

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Dawit A.]

Add several missing functions to the newly create spelling and grammar check platform plugin.

[Dawit A.]

Created attachment 104917 [details]
proposed patch

[Benjamin Poulain]

Comment on attachment 104917 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104917&action=review

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:55
> -    m_spellChecker->ignoreWordInSpellDocument(word);
> +    m_spellChecker->ignoreWord(word);

I think a mapping 1 to 1 between the WebKit APIs and the platform plugin is a good thing. Platform plugins are a low level tool for us, it does not needs a beautiful API, it needs a totally unambiguous API.

[Dawit A.]

(In reply to comment #2)
> (From update of attachment 104917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104917&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:55
> > -    m_spellChecker->ignoreWordInSpellDocument(word);
> > +    m_spellChecker->ignoreWord(word);
> 
> I think a mapping 1 to 1 between the WebKit APIs and the platform plugin is a good thing. Platform plugins are a low level tool for us, it does not needs a beautiful API, it needs a totally unambiguous API.

Ok. Will revert.

[Dawit A.]

Created attachment 105663 [details]
proposed patch II

Reverted back "ignoreWord" to "ignoreWordInSpellDocument" in QWebSpellChecker.

[Benjamin Poulain]

Comment on attachment 105663 [details]
proposed patch II

I am not even convinced that should be part of the same plugin.
This is a lot more intrusive than the text checker patch, not sure how do deal with that really.

[Allan Sandfeld Jensen]

The patch looks good to me, but will need a rebase for a new review.

[Simon Hausmann]

(In reply to comment #6)
> The patch looks good to me, but will need a rebase for a new review.

You're right, the patch is rather clean. I think it's missing a best a version update of the platform plugin interface version string because of the API changes.

But there's one thing I'm not sure if I understand correctly: How does the UI for this look like? Does the platform plugin pop up a modal dialog? Is there any way for the application / browser to affect the appearance of this? How does it look like in other browsers? Is this still relevant? :)

[Lindsay Mathieson]

Its a platform plugin framework, it doesn't have a UI or even spellcheck capability of its own. The Applications using it supply the plugin implementation and UI.

[Simon Hausmann]

(In reply to comment #8)
> Its a platform plugin framework, it doesn't have a UI or even spellcheck capability of its own. The Applications using it supply the plugin implementation and UI.

The interface added with this patch appears to delegate the responsibility of showing a spell-checking UI (dialog) to the platform plugin.

The platform plugin on the other hand is designed to be supplied by the platform (KDE, or for example the Nokia N9 using MeeGo is shipping one such plugin), not the application. In fact there is no application side interface to supply the platform plugin.

[Lindsay Mathieson]

(In reply to comment #9)
> (In reply to comment #8)
> 
> The interface added with this patch appears to delegate the responsibility of showing a spell-checking UI (dialog) to the platform plugin.

Yes.

> 
> The platform plugin on the other hand is designed to be supplied by the platform (KDE, or for example the Nokia N9 using MeeGo is shipping one such plugin), not the application. In fact there is no application side interface to supply the platform plugin.

I wasn't under that impression, I thought Platform Plugin meant plugins that were platform dependent (as in KDE, Pure Qt, Meego etc), but implemented by the application either dynamically or statically. Why else have explicit support for static plugins which can only be implemented by applications.

[Simon Hausmann]

(In reply to comment #10)
> > The platform plugin on the other hand is designed to be supplied by the platform (KDE, or for example the Nokia N9 using MeeGo is shipping one such plugin), not the application. In fact there is no application side interface to supply the platform plugin.
> 
> I wasn't under that impression, I thought Platform Plugin meant plugins that were platform dependent (as in KDE, Pure Qt, Meego etc), but implemented by the application either dynamically or statically. Why else have explicit support for static plugins which can only be implemented by applications.

That's unfortunately not the case. The mechanism for loading the platform plugin expects to find it in the system wide library directory, to be loaded dynamically at run-time. It does not support statically linked platform plugins and applications are not supposed to install libraries into those system locations (because they would affect other unrelated applications).

The platform plugin - like the name says - is supposed to be provided by the platform, the entity distribution Qt usually.

This is why I'm concerned that this may not be the right approach to implementing / delegating the UI of spell checking.

[Lindsay Mathieson]

(In reply to comment #11)
> 
> That's unfortunately not the case. The mechanism for loading the platform plugin expects to find it in the system wide library directory, to be loaded dynamically at run-time. It does not support statically linked platform plugins and applications are not supposed to install libraries into those system locations (because they would affect other unrelated applications).


It most definitely does load static plugins - for a start off I'm testing with them. Also see "bool QtPlatformPlugin::loadStaticallyLinkedPlugin()" in QPlatformPlugin.cpp. Also it loads dynamic plugins from QCoreApplication::libraryPaths() + "/webkit/" which as per the Qt Docs:

"This list will include the installation directory for plugins if it exists (the default installation directory for plugins is INSTALL/plugins, where INSTALL is the directory where Qt was installed). The directory of the application executable (NOT the working directory) is always added, as well as the colon separated entries of the QT_PLUGIN_PATH environment variable."



> 
> The platform plugin - like the name says - is supposed to be provided by the platform, the entity distribution Qt usually.

I disagree - to me platform plugins are specific to the platform but not neccessarily provided by it. The inline spell plugin is a good example, in KDE its far more useful for it to be customised per app then to be a single global item.

[Anders Carlsson]

Comment on attachment 105663 [details]
proposed patch II

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.