Bug 25585

Summary: [Qt] Webkit in Qt does not have window.showModalDialog
Product: WebKit Reporter: Giacomo <euthymos>
Component: DOMAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, hausmann, kbalazs, kent.hansen, laszlo.gombos, ossy, yael
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 31552    
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3 none

Description Giacomo 2009-05-06 01:27:02 PDT
Webkit in Qt _does not have_ window.showModalDialog pseudo-JS function. To confirm this, window.alert(window.showModalDialog) pop ups "undefined".

But in the docs, see http://doc.trolltech.com/4.5/qwebpage.html#WebWindowType-enum, it seems modal browser dialogs can be generated. How, without window.showModalDialog?
Comment 1 Tor Arne Vestbø 2009-05-06 01:40:19 PDT
This is not implemented in the Qt chrome client right now.

bool ChromeClientQt::canRunModal()
{
    notImplemented();
    return false;
}

Will investigate, thanks.


Comment 2 Yael 2010-02-05 14:16:54 PST
Created attachment 48260 [details]
Patch v1

WebKit already supports window.showModalDialog as defined in http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#dialogs-implemented-using-separate-documents.
This patch adds the necessary bits for QtWebKit to support that.
Comment 3 Kenneth Rohde Christiansen 2010-02-06 07:05:10 PST
Comment on attachment 48260 [details]
Patch v1


> +    bool canRun = false;
> +    QMetaObject::invokeMethod(m_webPage, "canRunModal", Qt::DirectConnection, Q_RETURN_ARG(bool, canRun));
> +    return canRun;
>  }

Is the invokeMethod supposed to modify the canRun? I suppose the Q_RETURN_ARG is a macro modifying it, but if that is the case, please add a comment. 
 
>  void ChromeClientQt::runModal()
>  {
> -    notImplemented();
> +    QMetaObject::invokeMethod(m_webPage, "runModal", Qt::DirectConnection);
>  }
>  

> @@ -72,7 +72,8 @@
>      Q_PROPERTY(LinkDelegationPolicy linkDelegationPolicy READ linkDelegationPolicy WRITE setLinkDelegationPolicy)
>      Q_PROPERTY(QPalette palette READ palette WRITE setPalette)
>      Q_PROPERTY(bool contentEditable READ isContentEditable WRITE setContentEditable)
> -    Q_ENUMS(LinkDelegationPolicy NavigationType WebAction)
> +    Q_PROPERTY(WebWindowType windowType READ webWindowType)
> +    Q_ENUMS(LinkDelegationPolicy NavigationType WebAction WebWindowType)

This patch introduces new API, please add it to the API tracker bug.

>  public:
>      enum NavigationType {
>          NavigationTypeLinkClicked,
> @@ -247,6 +248,8 @@
>      void setContentEditable(bool editable);
>      bool isContentEditable() const;
>  
> +    WebWindowType webWindowType() const;
> +
>  #ifndef QT_NO_CONTEXTMENU
>      bool swallowContextMenuEvent(QContextMenuEvent *event);
>  #endif
> @@ -301,6 +304,8 @@
>  
>  public Q_SLOTS:
>      bool shouldInterruptJavaScript();
> +    void runModal();
> +    bool canRunModal();


Are these supposed to be virtual?
Comment 4 Yael 2010-02-06 10:11:31 PST
Kenneth, When I introduced QWebPage::shouldInterruptJavaScript() I was told that we cannot add new virtual methods because that would break BC.
I am using here the same (ugly) technique to add a new "semi-virtual" method without breaking BC. 
I will add a comment about Q_RETURN_ARG modifying canRun.
Comment 5 Yael 2010-02-08 06:17:49 PST
Created attachment 48332 [details]
Patch v2

Added comment to ChromeClientQt as per comment #c3
Comment 6 Yael 2010-02-09 07:34:35 PST
Created attachment 48412 [details]
Patch v3

Make runModalDialog work almost "out of the box", with the exception that the application must set the modality flag on the correct window. WebKit does not know which window is the correct one for setting the modality flag.
Comment 7 Kenneth Rohde Christiansen 2010-02-09 07:42:35 PST
Comment on attachment 48412 [details]
Patch v3

should we change "should" to "must" ?
Comment 8 Yael 2010-02-09 07:57:05 PST
Landed as:  http://trac.webkit.org/changeset/54550
Comment 9 Yael 2010-02-09 07:58:05 PST
Comment on attachment 48412 [details]
Patch v3

> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(revision 54545)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(working copy)
> @@ -44,6 +44,7 @@
>  #include "SecurityOrigin.h"
>  
>  #include <qdebug.h>
> +#include <qeventloop.h>
>  #include <qtextdocument.h>
>  #include <qtooltip.h>
>  
> @@ -62,13 +63,15 @@
>  
>  ChromeClientQt::ChromeClientQt(QWebPage* webPage)
>      : m_webPage(webPage)
> +    , m_eventLoop(0)
>  {
>      toolBarsVisible = statusBarVisible = menuBarVisible = true;
>  }
>  
>  ChromeClientQt::~ChromeClientQt()
>  {
> -
> +    if (m_eventLoop)
> +        m_eventLoop->exit();
>  }
>  
>  void ChromeClientQt::setWindowRect(const FloatRect& rect)
> @@ -173,14 +176,16 @@
>  
>  bool ChromeClientQt::canRunModal()
>  {
> -    notImplemented();
> -    return false;
> +    return true;
>  }
>  
>  
>  void ChromeClientQt::runModal()
>  {
> -    notImplemented();
> +    m_eventLoop = new QEventLoop();
> +    QEventLoop* eventLoop = m_eventLoop;
> +    m_eventLoop->exec();
> +    delete eventLoop;
>  }
>  
>  
> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.h
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.h	(revision 54545)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.h	(working copy)
> @@ -34,6 +34,7 @@
>  #include "KURL.h"
>  #include "PlatformString.h"
>  
> +class QEventLoop;
>  class QWebPage;
>  
>  namespace WebCore {
> @@ -158,6 +159,7 @@
>          bool toolBarsVisible;
>          bool statusBarVisible;
>          bool menuBarVisible;
> +        QEventLoop* m_eventLoop;
>      };
>  }
>  
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 54547)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,22 @@
> +2010-02-09  Yael Aharon  <yael.aharon@nokia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Webkit in Qt does not have window.showModalDialog
> +        https://bugs.webkit.org/show_bug.cgi?id=25585
> +
> +        Create a new eventloop when runModal() is called.
> +        Added comemnt in QWebPage::createWindow that the application is responsible
> +        for setting the modality of the appropriate window.
> +
> +        * Api/qwebpage.cpp:
> +        * WebCoreSupport/ChromeClientQt.cpp:
> +        (WebCore::ChromeClientQt::ChromeClientQt):
> +        (WebCore::ChromeClientQt::~ChromeClientQt):
> +        (WebCore::ChromeClientQt::canRunModal):
> +        (WebCore::ChromeClientQt::runModal):
> +        * WebCoreSupport/ChromeClientQt.h:
> +
>  2010-01-19  Kenneth Rohde Christiansen  <kenneth@webkit.org>
>  
>          Reviewed by Dave Hyatt.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp	(revision 54545)
> +++ WebKit/qt/Api/qwebpage.cpp	(working copy)
> @@ -1955,6 +1955,8 @@
>      If the view associated with the web page is a QWebView object, then the default implementation forwards
>      the request to QWebView's createWindow() function; otherwise it returns a null pointer.
>  
> +    If \a type is WebModalDialog, the application should call setWindowModality(Qt::ApplicationModal) on the new window.
> +
>      \sa acceptNavigationRequest()
>  */
>  QWebPage *QWebPage::createWindow(WebWindowType type)
> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 54547)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2010-02-09  Yael Aharon  <yael.aharon@nokia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Webkit in Qt does not have window.showModalDialog
> +        https://bugs.webkit.org/show_bug.cgi?id=25585
> +
> +        Set the modality flag when createWindow is called with window type WebWindowDialog.
> +
> +        * QtLauncher/main.cpp:
> +        (WebPage::createWindow):
> +
>  2010-02-09  Chang Shu  <Chang.Shu@nokia.com>
>  
>          Reviewed by Laszlo Gombos.
> Index: WebKitTools/QtLauncher/main.cpp
> ===================================================================
> --- WebKitTools/QtLauncher/main.cpp	(revision 54545)
> +++ WebKitTools/QtLauncher/main.cpp	(working copy)
> @@ -520,9 +520,11 @@
>  #endif
>  }
>  
> -QWebPage* WebPage::createWindow(QWebPage::WebWindowType)
> +QWebPage* WebPage::createWindow(QWebPage::WebWindowType type)
>  {
>      LauncherWindow* mw = new LauncherWindow;
> +    if (type == WebModalDialog)
> +        mw->setWindowModality(Qt::ApplicationModal);
>      mw->show();
>      return mw->page();
>  }
Clearing flags.
Comment 10 Csaba Osztrogonác 2010-02-09 15:19:15 PST
Qt expected files updated by http://trac.webkit.org/changeset/54568

But I'm not sure if platform/qt/http/tests/security/cross-frame-access-call-expected.txt is correct:
-PASS: window.showModalDialog.call(targetWindow); should be 'TypeError: Result of expression 'window.showModalDialog' [undefined] is not an object.' and is.
+*** FAIL: window.showModalDialog.call(targetWindow); should be 'TypeError: Result of expression 'window.showModalDialog' [undefined] is not an object.' but instead is undefined.

What do you think about it?
Comment 11 Simon Hausmann 2010-02-10 07:52:08 PST
(In reply to comment #10)
> Qt expected files updated by http://trac.webkit.org/changeset/54568
> 
> But I'm not sure if
> platform/qt/http/tests/security/cross-frame-access-call-expected.txt is
> correct:
> -PASS: window.showModalDialog.call(targetWindow); should be 'TypeError: Result
> of expression 'window.showModalDialog' [undefined] is not an object.' and is.
> +*** FAIL: window.showModalDialog.call(targetWindow); should be 'TypeError:
> Result of expression 'window.showModalDialog' [undefined] is not an object.'
> but instead is undefined.
> 
> What do you think about it?

That's most likely caused due to the lack of modal window support in the Qt DRT.
Comment 12 Yael 2010-02-10 17:55:34 PST
(In reply to comment #10)
> Qt expected files updated by http://trac.webkit.org/changeset/54568
> 
> But I'm not sure if
> platform/qt/http/tests/security/cross-frame-access-call-expected.txt is
> correct:
> -PASS: window.showModalDialog.call(targetWindow); should be 'TypeError: Result
> of expression 'window.showModalDialog' [undefined] is not an object.' and is.
> +*** FAIL: window.showModalDialog.call(targetWindow); should be 'TypeError:
> Result of expression 'window.showModalDialog' [undefined] is not an object.'
> but instead is undefined.
> 
> What do you think about it?

I actually think that this is the expected result. Mac port does not support canRunModal in DRT, and their expected results reflect that. Since we do support it, we have different results.
It all comes down to the fact that the implementation of canRunModal in WebChromeClient.mm returns false when running in DRT, and ChromeClientQt.cpp returns true. 
(this is being called from JSDOMWindow::getOwnPropertySlot() before we check cross frames access rights)
Comment 13 Tor Arne Vestbø 2010-03-22 09:38:43 PDT
As far as I can tell this is fixed?
Comment 14 Giacomo 2010-04-29 14:19:37 PDT
Is this fixed in Qt 4.6.2?
Comment 15 Balazs Kelemen 2011-10-30 15:06:09 PDT
I have just looked around in ChromeClientQt.cpp and I think there is a bug in this code: 

void ChromeClientQt::runModal()
{
    m_eventLoop = new QEventLoop();
    QEventLoop* eventLoop = m_eventLoop;
    m_eventLoop->exec();
    delete eventLoop;
}

It deletes the new object. I guess it should delete the old one. BTW I think we should not have a non-null loop here anyway.
Comment 16 Yael 2011-10-31 19:14:54 PDT
(In reply to comment #15)
> I have just looked around in ChromeClientQt.cpp and I think there is a bug in this code: 
> 
> void ChromeClientQt::runModal()
> {
>     m_eventLoop = new QEventLoop();
>     QEventLoop* eventLoop = m_eventLoop;
>     m_eventLoop->exec();
>     delete eventLoop;
> }
> 
> It deletes the new object. I guess it should delete the old one. BTW I think we should not have a non-null loop here anyway.

The point is that the line after "m_eventLoop->exec()" is executed AFTER the dialog is closed. There is no "old" or "new".