Bug 72910 – [Qt] [WK2] Move PageUIClient related code to QtWebPageUIClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

[Qt] [WK2] Move PageUIClient related code to QtWebPageUIClient

[Caio Marcelo de Oliveira Filho]

Created attachment 116129 [details]
Patch

[Andreas Kling]

Comment on attachment 116129 [details]
Patch

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

[Rafael Brandao]

Comment on attachment 116129 [details]
Patch

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

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

I don't get this line. This is an infinite loop isn't? Probably missing the for condition "i < wkSelectedFileNames.size()".

[Caio Marcelo de Oliveira Filho]

Created attachment 116175 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 116129 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:115
>> +    // FIXME: Remove scoped pointer when possible.
> 
> When will this be possible?

I had the plan of making this object directly a member instead of (smart)pointer. For this to happen, we need to make sure we can create the QtWebPageProxy in the initalization list. But this is an unrelated change, let's not do it now.

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

Fixed.

>> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:95
>> +    if (!wkSelectedFileNames.isEmpty())
> 
> This check can be removed.

Fixed. Note that this and next comment refer to code in functions that were just moved from QtWebPageProxy. I fixed them anyway.

>>> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96
>>> +        for (unsigned i = 0; wkSelectedFileNames.size(); ++i)
>> 
>> Vector::size() returns size_t, not unsigned.
> 
> I don't get this line. This is an infinite loop isn't? Probably missing the for condition "i < wkSelectedFileNames.size()".

Fixed the size_t and the bug, nice catch Rafael :-)

>> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:39
>> +    // WKPageUIClient callbacks.
> 
> Let's make these private instead of public.

Fixed.

>> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:43
>> +    static void setStatusText(WKPageRef, WKStringRef text, const void *clientInfo);
> 
> Nit: The 'text' argument name is unnecessary.

Fixed.

>> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.h:49
>> +    QString runJavaScriptPrompt(const QString& message, const QString& defaultValue, bool& ok);
> 
> Nit: The 'alertText' argument name should probably also be called 'message'.

Fixed.

[Caio Marcelo de Oliveira Filho]

Created attachment 116181 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #3)
> (From update of attachment 116129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116129&action=review
> 
> >> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:96
> >> +        for (unsigned i = 0; wkSelectedFileNames.size(); ++i)
> > 
> > Vector::size() returns size_t, not unsigned.
> 
> I don't get this line. This is an infinite loop isn't? Probably missing the for condition "i < wkSelectedFileNames.size()".

I remembered you ran the API tests. We do have an awesome coverage :D. ahaha

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)
> I remembered you ran the API tests. We do have an awesome coverage :D. ahaha

This will be easier to test when we move to QML for the file selection dialog. Stay tuned ;)

[Andreas Kling]

Comment on attachment 116181 [details]
Patch

Woosh!

[Caio Marcelo de Oliveira Filho]

Committed r100990: <http://trac.webkit.org/changeset/100990>