Bug 69274 – [Qt] [WK2] Support JS alert/confirm/prompt in QDesktopWebView

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

[Caio Marcelo de Oliveira Filho]

[Qt] [WK2] Support JS alert/confirm/prompt in QDesktopWebView

[Caio Marcelo de Oliveira Filho]

Created attachment 109503 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 109503 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:211
> +void QDesktopWebViewPrivate::runJavaScriptAlert(const QString& alertText)
> +{
> +#ifndef QT_NO_MESSAGEBOX
> +    const QString title = tr("JavaScript Alert - %1").arg(q->url().host());
> +    disableMouseEvents();
> +    QMessageBox::information(0, title, escapeHtml(alertText), QMessageBox::Ok);
> +    enableMouseEvents();
> +#else

We're currently trying to get rid of dependencies of QtWebKit to QtGui to slim down memory uses.
It would also be nice to give the the choice of using QMessageBox or a homemade/QtComponent dialog to the user code.

Virtual method reimplementations aren't friendly to straight QML use cases, though I'm not sure if JS alerts should be considered a common use case for simple apps.

In any case, emitting a signal instead might be better.

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:164
> +static inline WKStringRef createNullWKString()
> +{
> +    RefPtr<WebString> webString = WebString::createNull();
> +    return toAPI(webString.release().leakRef());
> +}

I think that WKStringCreateWithQString should return a null WKString if given a null QString as input. Is it possible to use it instead?

[Alexis Menard (darktears)]

Comment on attachment 109503 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:-25
> -

Useless.

>> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:211

> 
> We're currently trying to get rid of dependencies of QtWebKit to QtGui to slim down memory uses.
> It would also be nice to give the the choice of using QMessageBox or a homemade/QtComponent dialog to the user code.
> 
> Virtual method reimplementations aren't friendly to straight QML use cases, though I'm not sure if JS alerts should be considered a common use case for simple apps.
> 
> In any case, emitting a signal instead might be better.

But how you spin off an event loop from the QML side?

[Alexis Menard (darktears)]

(In reply to comment #3)
> (From update of attachment 109503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109503&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:-25
> > -
> 
> Useless.
> 
> >> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:211
> 
> > 
> > We're currently trying to get rid of dependencies of QtWebKit to QtGui to slim down memory uses.
> > It would also be nice to give the the choice of using QMessageBox or a homemade/QtComponent dialog to the user code.
> > 
> > Virtual method reimplementations aren't friendly to straight QML use cases, though I'm not sure if JS alerts should be considered a common use case for simple apps.
> > 
> > In any case, emitting a signal instead might be better.
> 
> But how you spin off an event loop from the QML side?

We don't have yet such component and also the API call is sync from the WebProcess, so I'm not sure how a signal could work here. Also I'm wondering how this API could look like as the popup needs to be modal. But being optional yes.

This entire QML styling thing should be properly studied for WebKit2. I mean, in theory we should have some kind of RenderThemeQML where youu could provide QML styling (but this is not exactly what we want here)

[Jocelyn Turcotte]

(In reply to comment #3)
> But how you spin off an event loop from the QML side?

Humm that's a good point, there is nothing that prevents us from running an event loop beside the sync message system that is bound to the stack. Though since most web sites stopped using them I guess that we should keep it simple.

Depending on how we manage our QtGui dependecies QT_NO_MESSAGEBOX might be enough.
Adding Pierre on the loop.

[Caio Marcelo de Oliveira Filho]

Hello Jocelyn, thanks for the promptly review.

(In reply to comment #2)
> Virtual method reimplementations aren't friendly to straight QML use cases, though I'm not sure if JS alerts should be considered a common use case for simple apps.

Note that the virtual methods weren't exposed to the QDesktopWebView API. We can expose this in API in different ways.

A constraint I had was to keep the message exchange from WebProcess to UIProcess synchronous. For the desktop case I wasn't expecting customization to be a major thing, and once we had QML dialogs we could replace them.


> In any case, emitting a signal instead might be better.

I don't think emitting signal will solve the problem. If we assume the responsibility of spawing a QML Component, maybe we can get a good enough API for QML. I'll give it a shot here and update you guys.


> I think that WKStringCreateWithQString should return a null WKString if given a null QString as input. Is it possible to use it instead?

Currently it doesn't. The current conversion from WTF::String -> WKStringRef turns Null strings into Empty strings, and QString conversion relies on that.

Maybe would be better to treat this special case inside WKStringCreateWithQString()?

[Caio Marcelo de Oliveira Filho]

(In reply to comment #6)
> I don't think emitting signal will solve the problem. If we assume the responsibility of spawing a QML Component, maybe we can get a good enough API for QML. I'll give it a shot here and update you guys.

For the record, I followed up this on the IRC, basically one working way to provide API for customization is to allow QML set an alertDialog component that is used in runJavaScriptAlert() to create a QML item. Like QMessageBox and QInputDialog static methods, we run our own QEventLoop waiting for a dismiss signal.

Although I like this approach, this seems overkill for alert/confirm/prompt cases.

[Andreas Kling]

Comment on attachment 109503 [details]
Patch

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

Looks generally good, but I gotta run, so here's some nitpicking and I'll be back tomorrow:

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:207
> +    const QString title = tr("JavaScript Alert - %1").arg(q->url().host());

QLatin1String?

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:220
> +    const QString title = tr("JavaScript Confirm - %1").arg(q->url().host());

QLatin1String?

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:233
> +    const QString title = tr("JavaScript Prompt - %1").arg(q->url().host());

QLatin1String?

[Andreas Kling]

(In reply to comment #8)
> (From update of attachment 109503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109503&action=review
> 
> Looks generally good, but I gotta run, so here's some nitpicking and I'll be back tomorrow:
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:207
> > +    const QString title = tr("JavaScript Alert - %1").arg(q->url().host());
> 
> QLatin1String?
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:220
> > +    const QString title = tr("JavaScript Confirm - %1").arg(q->url().host());
> 
> QLatin1String?
> 
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:233
> > +    const QString title = tr("JavaScript Prompt - %1").arg(q->url().host());
> 
> QLatin1String?

Dammit, I just realized that this nitpicking is completely wrong! Never mind this comment :)

[Andreas Kling]

Comment on attachment 109503 [details]
Patch

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

LGTM!

> Source/WebKit2/ChangeLog:15
> +        empty and null strings.

between* empty and null strings

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:148
> +void qt_wk_runJavaScriptAlert(WKPageRef page, WKStringRef alertText, WKFrameRef frame, const void *clientInfo)

Style, * placement.

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:154
> +bool qt_wk_runJavaScriptConfirm(WKPageRef, WKStringRef message, WKFrameRef, const void *clientInfo)

Ditto.

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:160
> +static inline WKStringRef createNullWKString()

C API candidate? Maybe not.

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:166
> +WKStringRef qt_wk_runJavaScriptPrompt(WKPageRef, WKStringRef message, WKStringRef defaultValue, WKFrameRef, const void *clientInfo)

Style, * placement.

> Source/WebKit2/UIProcess/qt/ClientImpl.h:44
> +void qt_wk_runJavaScriptAlert(WKPageRef, WKStringRef alertText, WKFrameRef, const void *clientInfo);
> +bool qt_wk_runJavaScriptConfirm(WKPageRef, WKStringRef message, WKFrameRef, const void *clientInfo);
> +WKStringRef qt_wk_runJavaScriptPrompt(WKPageRef, WKStringRef message, WKStringRef defaultValue, WKFrameRef, const void *clientInfo);

Ditto.

[Caio Marcelo de Oliveira Filho]

Committed r96743: <http://trac.webkit.org/changeset/96743>