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

[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 102875: proposed patch II
https://bugs.webkit.org/attachment.cgi?id=102875&action=review

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


Quick review, first round :)

> Source/WebCore/ChangeLog:1
> +2011-08-03  Dawit Alemayehu	<adawit at kde.org>

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  <adawit at kde.org> and Lindsay Mathieson
<lindsay.mathieson at gmail.com>"

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