[Qt] Missing spell check support

[Dawit A.]

Even though webkit already provides the necessary place holder APIs for spell checking and this feature has been implemented in most of the other rendering engine ports, it is still missing from QtWebKit.

https://bugs.webkit.org/show_bug.cgi?id=42100 looks like a duplicate of this (or the other way around :-P)

[Antonio Gomes]

*** Bug 42100 has been marked as a duplicate of this bug. ***

[Andre Pedralho]

Adding myself to the CC list.

[Fabrizio]

Investigating.

[Andreas Kling]

@Fabrizio: Still investigating?

[Lindsay Mathieson]

Working on it

[Dawit A.]

Created attachment 102872 [details]
proposed patch I

Since there has been no movement on this for almost a year, I have taken the liberty of cleaning up Lindsay's patch[1] just a tad bit from and proposing it to get the ball rolling. The main goal here is to finally get spelling checking support for QtWebKit which at the moment lacks any such support unlike most of the other webkit ports.

[1] https://gitorious.org/~blackpaw/webkit/webkit-qt-inline-spell

[WebKit Review Bot]

Attachment 102872 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/EditorClientQt.h:46:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:121:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:655:  Missing spaces around =  [whitespace/operators] [4]
Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:655:  Missing space before ( in for(  [whitespace/parens] [5]
Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:657:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/qt/Api/qwebkitplatformplugin.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/Api/qwebkitplatformplugin.h:142:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 7 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dawit A.]

Created attachment 102875 [details]
proposed patch II

Fixed coding style issues.

[Hajime Morrita]

Why don't you make separate SpellCheckingClientQt class?
We now have WebCore::SpellCheckingClient which is extracted from WebCore::EditorClient.

[Lindsay Mathieson]

(In reply to comment #7)
> Created an attachment (id=102872) [details]
> proposed patch I
> 
> Since there has been no movement on this for almost a year, I have taken the liberty of cleaning up Lindsay's patch[1] just a tad bit from and proposing it to get the ball rolling. The main goal here is to finally get spelling checking support for QtWebKit which at the moment lacks any such support unlike most of the other webkit ports.


Thanks Dawit, I got so far and then got severely distracted :(

I'll apply the patch and your to my branch.

[Benjamin Poulain]

Comment on attachment 102875 [details]
proposed patch II

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

Quick review, first round :)

> Source/WebCore/ChangeLog:1
> [phone]  Dawit Alemayehu  <[e-mail]>

I don't know who did what, but if it is you both worked ~equally on it, you would be fair to add Lindsay as author in the Changelog:
"Dawit Alemayehu  <[e-mail]> and Lindsay Mathieson <[e-mail]>"

> Source/WebCore/ChangeLog:7
> +        [Qt] Missing spell check support
> +        https://bugs.webkit.org/show_bug.cgi?id=44114
> +

You need a description of the changes here.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:903
> +static void drawErrorUnderline(QPainter *painter, double x, double y, double width, double height)

qreal is prefered to double here due to the platforms on which double is a lot slower than float.

This code is not trivial to read and I am too lazy to read it now :)
Is there no way this could be made easier to read by splitting it in small-ish inline functions?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:905
> +    static const double heightSquares = 2.5;

No need for 'static' here.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:921
> +    path.moveTo(x - halfSquare, top + halfSquare); // A

What's those // A, B, C etc comments?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:979
> +    QPainter* painter = platformContext();
> +    painter->save();
> +
> +    switch (style) {
> +    case TextCheckingSpellingLineStyle:
> +        painter->setPen(QColor(255, 0, 0));
> +        break;
> +    case TextCheckingGrammarLineStyle:
> +        painter->setPen(QColor(0, 255, 0));
> +        break;
> +    default:
> +        painter->restore();
> +        return;
> +    }
> +
> +    drawErrorUnderline(painter, origin.x(), origin.y(), width, cMisspellingLineThickness);
> +    painter->restore();

QPainter::save() and QPainter::restore() is not free because the whole state has to be changed.
A good practice is to restore the previous state manually when possible.

e.g.:
const QPen originalPen = painter.pen();
...
painter.setPen(XXX);
...
drawErrorUnderUnderline(....);
painter.setPen(originalPen);

Instead of those QColor for the pen, you should use the constant Qt::red and Qt::green to make the code easier to read.

> Source/WebKit/qt/Api/qwebkitplatformplugin.h:40
>  
> +
> +
>  class QWebSelectData {

Change to revert :)

> Source/WebKit/qt/ChangeLog:7
> +        [Qt] Missing spell check support
> +        https://bugs.webkit.org/show_bug.cgi?id=44114
> +

This needs a description.

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:62
>  
> +
>  #define methodDebug() qDebug("EditorClientQt: %s", __FUNCTION__);

To remove :)

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:332
> +    freeSpellChecker();

This can be removed entirely if you use OwnPtr :)

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:546
> +    : m_page(page), m_spellChecker(0), m_editing(false), m_inUndoRedo(false)

You can split this on 4 lines for clarity.

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:590
> +    m_spellChecker->ignoreWord(word);

This should be learnWord() instead of ignoreWord()

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:593
> +void EditorClientQt::checkSpellingOfString(const UChar *word, int length, int* misspellingLocation, int* misspellingLength)

WebKit coding style:
const UChar* word;

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:598
> +    const QString str(reinterpret_cast<QChar const*>(word), length);

static_cast does not do the job here? I  would expect UChar and QChar to be of compatible types.
For the coding conventions: QChar const* -> const QChar*

I think QString::fromRawData() would be a better choice than the regular constructor here. QString::fromRawData() does not copy the data to create the string.

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:714
> +        qWarning() << Q_FUNC_INFO << "No spell checker found" << endl;

Not sure about the warning...
People do not like warnings when they did nothing wrong with their code. I think you should remove it (or use WebKit logging facilities?).

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:727
> +    if (!m_spellChecker)
> +        return;
> +
> +    delete m_spellChecker;
> +    m_spellChecker = 0;

Just for info: deleting a null ptr is safe, so the if(!m_spellChecker) is overkill.

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.h:123
> +    QWebSpellChecker* m_spellChecker;

You should use a OwnPtr<QWebSpellChecker>, and you can remove freeSpellChecker() and just call  m_spellChecker = nullptr; when needed.

In general, in WebKit, when possible we use smart pointers instead of deleting stuff manually. That makes it a lot harder to leak memory by accident.
WebKit has a very good set of smart pointers in wtf.

[Benjamin Poulain]

(In reply to comment #10)
> Why don't you make separate SpellCheckingClientQt class?
> We now have WebCore::SpellCheckingClient which is extracted from WebCore::EditorClient.

I guess you mean TextCheckerClient. It seems like a good idea to implement that client instead of modifying the editor.

[Dawit A.]

(In reply to comment #13)
> (In reply to comment #10)
> > Why don't you make separate SpellCheckingClientQt class?
> > We now have WebCore::SpellCheckingClient which is extracted from WebCore::EditorClient.
> 
> I guess you mean TextCheckerClient. It seems like a good idea to implement that client instead of modifying the editor.

I guess that can be done. However, EditorClientQt already inherited from both EditorClient and TextCheckerClient and neither myself nor Lindsay who did 99.999% of the work did not change that. Anyhow, I myself thought about doing that already since it makes things a lot more cleaner ; so I too think it would be a good idea to do that.

[Hajime Morrita]

> I guess that can be done. However, EditorClientQt already inherited from both EditorClient and TextCheckerClient and neither myself nor Lindsay who did 99.999% of the work did not change that. Anyhow, I myself thought about doing that already since it makes things a lot more cleaner ; so I too think it would be a good idea to do that.
Well, TextCheckerClient is a interface which was extracted from EditorClient,
thus it works with how current patch is doing.
So as you mentioned, separating class into two is just a nice to have.

[Dawit A.]

(In reply to comment #12)
>> Source/WebCore/ChangeLog:1
>> [phone]  Dawit Alemayehu  <[e-mail]>
>
> I don't know who did what, but if it is you both worked ~equally on it, you
> would be fair to add Lindsay as author in the Changelog:
> "Dawit Alemayehu  <[e-mail]> and Lindsay Mathieson
> <[e-mail]>"

That was not intentional. I always use ./Tools/Scripts/prepare-ChangeLog to generate the changelog so I forgot to actually change the author field. It is Lindsay that did most of the work ; so I will remedy that promptly. Thanks for pointing
it out.

>> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:903
>> +static void drawErrorUnderline(QPainter *painter, double x, double y, double
> width, double height)
>
> qreal is prefered to double here due to the platforms on which double is a lot
> slower than float.
>
> This code is not trivial to read and I am too lazy to read it now :)
> Is there no way this could be made easier to read by splitting it in small-ish
> inline functions?

I am sure it can be simplified. The current version seems to have been copied
from the cairo port, Source/WebCore/platform/graphics/cairo/DrawErrorUnderline.h.

Personally I would love to see and perhaps emulate how QTextDocument
or QTextEdit render such lines when the underline style is set to QTextCharFormat::SpellCheckUnderline. Unofrtunately, I have not yet been able to figure out where that code is located.

>> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:905
>> +    static const double heightSquares = 2.5;
>
> No need for 'static' here.
>
>> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:921
>> +    path.moveTo(x - halfSquare, top + halfSquare); // A
>
> What's those // A, B, C etc comments?

I guess Lindsay did not bother to copy the explanation about those comments. That can be remedied.

>> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:598
>> +    const QString str(reinterpret_cast<QChar const*>(word), length);
>
> static_cast does not do the job here? I  would expect UChar and QChar to be of
> compatible types.

Nope. You get compiler error about illegal case when you do that.

Next round coming up... :)

[Dawit A.]

(In reply to comment #11)
> (In reply to comment #7)
> > Created an attachment (id=102872) [details] [details]
> > proposed patch I
> > 
> > Since there has been no movement on this for almost a year, I have taken the liberty of cleaning up Lindsay's patch[1] just a tad bit from and proposing it to get the ball rolling. The main goal here is to finally get spelling checking support for QtWebKit which at the moment lacks any such support unlike most of the other webkit ports.
> 
> 
> Thanks Dawit, I got so far and then got severely distracted :(

No problems. You did most of the heavy lifting. Let us see if I can help carry this to the finish line.

[Dawit A.]

Created attachment 103074 [details]
proposed patch III

Addessed all the issues pointed out in comment #12. Also moved the implementation of TextCheckerClient into its own separate file.

[Dawit A.]

Created attachment 103075 [details]
proposed patch IV

Previous patch was not complete.

[WebKit Review Bot]

Attachment 103075 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.h:51:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dawit A.]

Created attachment 103087 [details]
proposed patch V

Fixed coding style issues.

[Benjamin Poulain]

Comment on attachment 103087 [details]
proposed patch V

Do you plan to upstream the plugin as well?

Because we want to be able to break the API of the plugin if necessary (adding methods for example).

This is much easier if the plugins are also upstreamed.
I would not mind at all a KDE plugin in WebKit trunk if that can be done in a clean way.

If upstreaming the plugin cannot be done, we might do some stuff differently.

I already cq- because I do not want this to land before deployment is clarified.

[Benjamin Poulain]

Comment on attachment 103087 [details]
proposed patch V

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::SpellChecker)) : 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).

[Benjamin Poulain]

The patch is very close to be landed I think. Probably just one more round.

[Lindsay Mathieson]

(In reply to comment #23)
> (From update of attachment 103087 [details])

> 
> 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?)

Its neccessary for statically linked plugins defined in a hosting app, otherwise you get link errors. And yes, the other plugins need it to, but I didn't want to alter stuff outside the spell checking area.

[Dawit A.]

(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 103087 [details] [details])
> 
> > 
> > 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?)
> 
> Its neccessary for statically linked plugins defined in a hosting app, otherwise you get link errors. And yes, the other plugins need it to, but I didn't want to alter stuff outside the spell checking area.

It is not only statically linked plugins that need it. If the plugin is an external plugin outside of the webkit, the exporting the plugins is required for dynamically linked in plugins as well. At least I get a linking error if the symbols are not exported for the spell checker plugin I attempted to create.

[Dawit A.]

(In reply to comment #22)
> (From update of attachment 103087 [details])
> Do you plan to upstream the plugin as well?
> 
> Because we want to be able to break the API of the plugin if necessary (adding methods for example).
> 
> This is much easier if the plugins are also upstreamed.
> I would not mind at all a KDE plugin in WebKit trunk if that can be done in a clean way.
> 
> If upstreaming the plugin cannot be done, we might do some stuff differently.
> 
> I already cq- because I do not want this to land before deployment is clarified.

Well I am not certain how to handle this either. However, since there is no API guarantee for the platform plugins, then there is no other option but to include the platform specific spell checker plugins into QtWebKit's code base much like the symbian plugins are now.

[Dawit A.]

(In reply to comment #23)
> I would also prefer a real variable name, like "platformPlugin", instead of p.

Does that mean the other functions should be changed as well ? Would it not be consistent to leave this alone especially since the QWebKitPlatformPlugin right before it makes it clear what "p" actually is ?

> > 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.

That can easily be recitfied for getAutoCorrectSuggestionForMisspelledWord. However, there is no easy way to accomplish that for the grammar checking function because it would require a structure that can be manipulated. IOW, how should that structure be passed to the corresponding function in QWebSpellChecker  ?

[Lindsay Mathieson]

(In reply to comment #28)
> (In reply to comment #23)
> > > 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.
> 
> That can easily be recitfied for getAutoCorrectSuggestionForMisspelledWord. However, there is no easy way to accomplish that for the grammar checking function because it would require a structure that can be manipulated. IOW, how should that structure be passed to the corresponding function in QWebSpellChecker  ?



Also, I presumed grammar checking would a separate plugin to the Spell Checker - quite different functionality. PUMA U though of course, I've never worked with grammar checker libraries.

[Benjamin Poulain]

(In reply to comment #28)
> That can easily be recitfied for getAutoCorrectSuggestionForMisspelledWord. However, there is no easy way to accomplish that for the grammar checking function because it would require a structure that can be manipulated. IOW, how should that structure be passed to the corresponding function in QWebSpellChecker  ?

The structure is quite simple: two integers, a Vector of String and one String. You could convert each list in the same way as you did for guessesList (maybe a helper function to convert Vector<String> to QStringList would be useful even outside this patch?).


> Also, I presumed grammar checking would a separate plugin to the Spell Checker - quite different functionality. PUMA U though of course, I've never worked with grammar checker libraries.

The system spell checker of Mac OS X (NSSpellChecker) supports both spelling and grammar.
It would be nice to make a Mac plugin once this patch is integrated, so I would like to have all the APIs ready.

[Dawit A.]

(In reply to comment #27)
> (In reply to comment #22)
> > (From update of attachment 103087 [details] [details])
> > Do you plan to upstream the plugin as well?
> > 
> > Because we want to be able to break the API of the plugin if necessary (adding methods for example).
> > 
> > This is much easier if the plugins are also upstreamed.
> > I would not mind at all a KDE plugin in WebKit trunk if that can be done in a clean way.
> > 
> > If upstreaming the plugin cannot be done, we might do some stuff differently.
> > 
> > I already cq- because I do not want this to land before deployment is clarified.
> 
> Well I am not certain how to handle this either. However, since there is no API guarantee for the platform plugins, then there is no other option but to include the platform specific spell checker plugins into QtWebKit's code base much like the symbian plugins are now.

On second thought this is going to cause circular dependency. kdelibs depends on QtWebKit and if we add a kde platform specific plugin, then QtWebKit would depend on kdelibs.

[Lindsay Mathieson]

(In reply to comment #30)
> (In reply to comment #28)
> 
> The system spell checker of Mac OS X (NSSpellChecker) supports both spelling and grammar.
> It would be nice to make a Mac plugin once this patch is integrated, so I would like to have all the APIs ready.

Ok, I see now, that makes sense. in which case we should probably define & implement:


    virtual bool isGrammarCheckingEnabled();
    virtual void toggleGrammarChecking();

in TextCheckerClientQt.h and use them in EditorClientQt should we not?

[Lindsay Mathieson]

(In reply to comment #32)

ps and QWebSpellChecker to of course

[Benjamin Poulain]

> Ok, I see now, that makes sense. in which case we should probably define & implement:
>
>     virtual bool isGrammarCheckingEnabled();
>     virtual void toggleGrammarChecking();
> 
> in TextCheckerClientQt.h and use them in EditorClientQt should we not?

Yes we should :)

[Lindsay Mathieson]

Created attachment 103765 [details]
proposed patch VI

Adds grammar checking support

[Benjamin Poulain]

(In reply to comment #35)
> Adds grammar checking support

You forgot the changelog :(

[Benjamin Poulain]

(In reply to comment #36)
> You forgot the changelog :(

You can try the script webkit-patch to detect problems early.

For example, to submit the git head to bugzilla you do:
./Tools/Scripts/webkit-patch upload 44114 --request-commit -g HEAD

It will give you the opportunity to make the changelog, and will do the style checking for you. When adding the patch, the script also obsolete the previous patch and set the right flags for you.

The script is very convenient because it do all the boring stuff for you. :)

[Dawit A.]

(In reply to comment #37)
> (In reply to comment #36)
> > You forgot the changelog :(
> 
> You can try the script webkit-patch to detect problems early.
> 
> For example, to submit the git head to bugzilla you do:
> ./Tools/Scripts/webkit-patch upload 44114 --request-commit -g HEAD
>
> It will give you the opportunity to make the changelog, and will do the style checking for you. When adding the patch, the script also obsolete the previous patch and set the right flags for you.
> 
> The script is very convenient because it do all the boring stuff for you. :)

Did not know about that script either. I did all that by hand all this time. :(

@lindsay
Though I cannot reveiew the code directly, I have one suggestion with the patch you posted. Move the grammar details class into QWebSpellChecker as a struct. There is no good reason for it to be a standalone class.

[Lindsay Mathieson]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > You forgot the changelog :(
> > 
> > You can try the script webkit-patch to detect problems early.
> > 
> > For example, to submit the git head to bugzilla you do:
> > ./Tools/Scripts/webkit-patch upload 44114 --request-commit -g HEAD
> >
> > It will give you the opportunity to make the changelog, and will do the style checking for you. When adding the patch, the script also obsolete the previous patch and set the right flags for you.


Its popping up vim for me (I presume for the changelog), but the content is empty and when I try to save it I get a "No file" error

[Lindsay Mathieson]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > You forgot the changelog :(
> > 
> > You can try the script webkit-patch to detect problems early.
> > 
> > For example, to submit the git head to bugzilla you do:
> > ./Tools/Scripts/webkit-patch upload 44114 --request-commit -g HEAD
> >
> > It will give you the opportunity to make the changelog, and will do the style checking for you. When adding the patch, the script also obsolete the previous patch and set the right flags for you.

Also, how do I add Dawit to the credits as well?

[Lindsay Mathieson]

Created attachment 103840 [details]
Patch

[WebKit Review Bot]

Attachment 103840 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/qt/Graphi..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:91:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:97:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lindsay Mathieson]

Created attachment 103842 [details]
Patch

[WebKit Review Bot]

Attachment 103842 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/qt/Graphi..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:91:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:97:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 103842 [details]
Patch

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.

[Lindsay Mathieson]

(In reply to comment #45)
> (From update of attachment 103842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103842&action=review
> 
> Quick review.
> 

Thanks Benjamin,

I'd already addressed most of your points but the patch script didn't upload them or write the Changelog. Grrrr :( I'll do it manually and upload.

[Dawit A.]

(In reply to comment #46)
> (In reply to comment #45)
> > (From update of attachment 103842 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=103842&action=review
> > 
> > Quick review.
> > 
> 
> Thanks Benjamin,
> 
> I'd already addressed most of your points but the patch script didn't upload them or write the Changelog. Grrrr :( I'll do it manually and upload.

Lindasy(In reply to comment #40)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (In reply to comment #36)
> > > > You forgot the changelog :(
> > > 
> > > You can try the script webkit-patch to detect problems early.
> > > 
> > > For example, to submit the git head to bugzilla you do:
> > > ./Tools/Scripts/webkit-patch upload 44114 --request-commit -g HEAD
> > >
> > > It will give you the opportunity to make the changelog, and will do the style checking for you. When adding the patch, the script also obsolete the previous patch and set the right flags for you.
> 
> Also, how do I add Dawit to the credits as well?

Lindsay, do not bother with that. There is really no point in having my name in the ChangeLog files since it is already in the copyright information of the source files. Your name there is sufficient, especially since you did most of the work.

[Benjamin Poulain]

Please ping me on IRC as soon as you have something ready so we can have faster review rounds. I would like this to land quickly.

[Lindsay Mathieson]

Created attachment 103905 [details]
Patch

[Lindsay Mathieson]

(In reply to comment #48)
> Please ping me on IRC as soon as you have something ready so we can have faster review rounds. I would like this to land quickly.

Uploaded, but don't know your IRC details (never use it myself)

[Benjamin Poulain]

Comment on attachment 103905 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

You should removing this.

> Source/WebCore/ChangeLog:29
> [phone]  Lindsay Mathieson  <[e-mail]> and Dawit Alemayehu   <[e-mail]>
> +
> +        [Qt] Missing spell check support
> +        https://bugs.webkit.org/show_bug.cgi?id=44114
> +        Implements platform plugin  api for spell and grammar checking,
> +        draws error lines for same.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. (OOPS!)
> +
> +        * platform/graphics/qt/GraphicsContextQt.cpp:
> +        (WebCore::drawErrorUnderline):
> +        (WebCore::GraphicsContext::drawLineForTextChecking):
> +

Two changelogs :(

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:943
> +    // Bottom of squiggle

Period at the end.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:959
> +    // Top of squiggle

Period at the end.

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

This should go away now that you have QWebSpellChecker::GrammarDetail

> Source/WebKit/qt/ChangeLog:35
> [phone]  Lindsay Mathieson  <[e-mail]>
> +
> +        [Qt] Missing spell check support
> +        https://bugs.webkit.org/show_bug.cgi?id=44114
> +        Fixed style failure.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Api/qwebkitplatformplugin.h:
> +        * QtWebKit.pro:
> +        * WebCoreSupport/EditorClientQt.cpp:
> +        (WebCore::EditorClientQt::isContinuousSpellCheckingEnabled):
> +        (WebCore::EditorClientQt::isGrammarCheckingEnabled):
> +        (WebCore::EditorClientQt::toggleContinuousSpellChecking):
> +        (WebCore::EditorClientQt::toggleGrammarChecking):
> +        * WebCoreSupport/EditorClientQt.h:
> +        (WebCore::EditorClientQt::textChecker):
> +        * WebCoreSupport/QtPlatformPlugin.cpp:
> +        (WebCore::QtPlatformPlugin::createSpellChecker):
> +        * WebCoreSupport/QtPlatformPlugin.h:
> +        * WebCoreSupport/TextCheckerClientQt.cpp: Added.
> +        (WebCore::TextCheckerClientQt::ignoreWordInSpellDocument):
> +        (WebCore::TextCheckerClientQt::learnWord):
> +        (WebCore::TextCheckerClientQt::getAutoCorrectSuggestionForMisspelledWord):
> +        (WebCore::TextCheckerClientQt::checkSpellingOfString):
> +        (WebCore::TextCheckerClientQt::checkGrammarOfString):
> +        (WebCore::TextCheckerClientQt::getGuessesForWord):
> +        (WebCore::TextCheckerClientQt::isContinousSpellCheckingEnabled):
> +        (WebCore::TextCheckerClientQt::toggleContinousSpellChecking):
> +        (WebCore::TextCheckerClientQt::isGrammarCheckingEnabled):
> +        (WebCore::TextCheckerClientQt::toggleGrammarChecking):
> +        (WebCore::TextCheckerClientQt::loadSpellChecker):
> +        * WebCoreSupport/TextCheckerClientQt.h: Added.
> +        (WebCore::TextCheckerClientQt::requestCheckingOfString):
> +

Double changelog :(

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

Order of the file -> alphabetical?

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

Order of the file -> alphabetical?

[Lindsay Mathieson]

(In reply to comment #51)
> (From update of attachment 103905 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103905&action=review

Dammit - I fixed all this, that "webkit-patch upload" script is making a mess of it.

[Lindsay Mathieson]

Created attachment 103995 [details]
Added a platform plugin to allow spell and grammar check in QtWebKit.

[Benjamin Poulain]

Comment on attachment 103995 [details]
Added a platform plugin to allow spell and grammar check in QtWebKit.

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

Great.
I will land manually in order to remove the test line from the changelog.

> Source/WebCore/ChangeLog:11
> +        No new tests because the line rendering code is copied from the cairo port.
> +

That is not an excuse for not having tests :)
In our case, the problem is we do not have the plugin running on the bot, so we cannot test this particular change.

> Source/WebKit/qt/WebCoreSupport/TextCheckerClientQt.cpp:46
> +static void convertToVectorList(const QStringList& list, Vector<String>& vList)
> +{
> +    const int count = list.count();
> +    vList.resize(count);
> +    for (int i = 0; i < count; ++i)
> +        vList.append(list.at(i));
> +}

Nice!

[Benjamin Poulain]

Committed r93093: <http://trac.webkit.org/changeset/93093>

[Lindsay Mathieson]

Created attachment 104623 [details]
Patch

[Dawit A.]

(In reply to comment #56)
> Created an attachment (id=104623) [details]
> Patch

I think you need to a new ticket for this since this ticket is already closed and the patch submitted. Is this the fix for the the spell checker plugin crashing ?

[Lindsay Mathieson]

(In reply to comment #57)
> (In reply to comment #56)
> > Created an attachment (id=104623) [details] [details]
> > Patch
> 
> I think you need to a new ticket for this since this ticket is already closed and the patch submitted. Is this the fix for the the spell checker plugin crashing ?

Yup, that's the one

[Lindsay Mathieson]

(In reply to comment #57)
> (In reply to comment #56)
> > Created an attachment (id=104623) [details] [details]
> > Patch
> 
> I think you need to a new ticket for this since this ticket is already closed and the patch submitted. 

Should I just create a new ticket for it?

[Dawit A.]

(In reply to comment #59)
> (In reply to comment #57)
> > (In reply to comment #56)
> > > Created an attachment (id=104623) [details] [details] [details]
> > > Patch
> > 
> > I think you need to a new ticket for this since this ticket is already closed and the patch submitted. 
> 
> Should I just create a new ticket for it?

Yes. This one is already done. :)

[Benjamin Poulain]

Comment on attachment 104623 [details]
Patch

Please create a new bug report for the patch. We like to keep thing separated. That way, figuring out history, or reverting a patch, is easier.

The Changelog should also have [Qt] in the title, and a description of the bug and how you fix it.

[Lindsay Mathieson]

(In reply to comment #61)
> (From update of attachment 104623 [details])
> Please create a new bug report for the patch. We like to keep thing separated. That way, figuring out history, or reverting a patch, is easier.
> 
> The Changelog should also have [Qt] in the title, and a description of the bug and how you fix it.

done - https://bugs.webkit.org/show_bug.cgi?id=66628

I'll update the Changelog