[webkit-reviews] review granted: [Bug 72910] [Qt] [WK2] Move PageUIClient related code to QtWebPageUIClient : [Attachment 116129] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>'s request for review:
Bug 72910: [Qt] [WK2] Move PageUIClient related code to QtWebPageUIClient
https://bugs.webkit.org/show_bug.cgi?id=72910

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116129&action=review


Love it! r=me with some nitpicking.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:115
> +    // FIXME: Remove scoped pointer when possible.

When will this be possible?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:116
> +    QScopedPointer<QtWebPageUIClient> m_pageUIClient;

Per Qt API style, this member should not have the "m_" prefix.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:95
> +    if (!wkSelectedFileNames.isEmpty())

This check can be removed.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96
> +	   for (unsigned i = 0; wkSelectedFileNames.size(); ++i)

Vector::size() returns size_t, not unsigned.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:39
> +    // WKPageUIClient callbacks.

Let's make these private instead of public.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:43
> +    static void setStatusText(WKPageRef, WKStringRef text, const void
*clientInfo);

Nit: The 'text' argument name is unnecessary.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:49
> +    void runJavaScriptAlert(const QString& alertText);
> +    bool runJavaScriptConfirm(const QString& message);
> +    QString runJavaScriptPrompt(const QString& message, const QString&
defaultValue, bool& ok);

Nit: The 'alertText' argument name should probably also be called 'message'.