Bug 39146 – [Qt] Pass all web notification layout tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Yael]

Continue the work that has Laszlo started on supporting web notifications in QtWebKit.

[Yael]

Created attachment 56133 [details]
Patch.

This patch does not yet add support for downloading icons or for html based notifications.
There is no documentation for the new API, since I expect to have some discussion about that API.

[Yael]

Created attachment 56487 [details]
Patch

This patch wraps the QSystemTrayIcon, so that we can pass layout tests even if QT_NO_SYSTEMTRAYICON is not defined.
Qt S60 porting team told me that they could provide a QSystemTrayIcon impleemntation for Symbian 10.1, so at this time, I don't think we need to hook up notifications with the platform plugin.

Loading the notifications' icons and markup is still under work.

[Yael]

Comment on attachment 56487 [details]
Patch

Removing review flag because I need to merge with Robert's work first:-)

[Yael]

Created attachment 56493 [details]
Updated patch after merge.

[Kenneth Rohde Christiansen]

Comment on attachment 56493 [details]
Updated patch after merge.


> +    enum NotificationPermission {
> +        PermissionAllowed,
> +        PermissionNotAllowed,
> +        PermissionDenied
> +    };

Maybe leave out Permission (we normally leave our or prepend part of the enum name)

What is the difference between NotAllowed and Denied? That is not obvious.

Are you sure this will never turn into a flag in the future?

Where are these used? I don't seem them used in the API.

> +
>      explicit QWebPage(QObject *parent = 0);
>      ~QWebPage();
>  
> @@ -258,6 +264,8 @@ public:
>  
>      QMenu *createStandardContextMenu();
>  
> +    void allowNotificationForOrigin(const QString& origin);

Origin makes me think of QWebSecurityOrigin, our other API's uses ForUrl (useAgentForUrl etc)

[Laszlo Gombos]

Comment on attachment 56493 [details]
Updated patch after merge.

Overall the "meat" of the patch looks really good. 

> Index: WebKit/qt/Api/qwebpage.h
> ===================================================================
> --- WebKit/qt/Api/qwebpage.h	(revision 59762)
> +++ WebKit/qt/Api/qwebpage.h	(working copy)
> @@ -192,6 +192,12 @@ public:
>          WebModalDialog
>      };
>  
> +    enum NotificationPermission {
> +        PermissionAllowed,
> +        PermissionNotAllowed,
> +        PermissionDenied
> +    };
> +
>      explicit QWebPage(QObject *parent = 0);
>      ~QWebPage();
>  
> @@ -258,6 +264,8 @@ public:
>  
>      QMenu *createStandardContextMenu();
>  
> +    void allowNotificationForOrigin(const QString& origin);
> +
>      enum Extension {
>          ChooseMultipleFilesExtension,
>          ErrorPageExtension
> @@ -335,6 +343,8 @@ Q_SIGNALS:
>  
>      void saveFrameStateRequested(QWebFrame* frame, QWebHistoryItem* item);
>      void restoreFrameStateRequested(QWebFrame* frame);
> +    void checkPermission(const QUrl&, QWebPage::NotificationPermission&);
> +    void requestPermission(const QString&);
>  

As the corresponding W3C specification is still in early draft (see also some comments from Kenneth above) we decided that it might be a bit early to create a public API for this. Let's put these interfaces to DumpRenderTreeSupportQt.h support for now until we have better confidence on the API. 

At the same time we should take some of the suggestions Kenneth had about the naming of the enum for example.

> -NotificationPresenter::Permission NotificationPresenterClientQt::checkPermission(const KURL&)
> +NotificationPresenter::Permission NotificationPresenterClientQt::checkPermission(const KURL& url)
>  {
> -    // FIXME Implement permission policy
> +    QWebPage::NotificationPermission permission = QWebPage::PermissionAllowed;

Now that this patch introduces permission support we should make the default permission NotAllowed (or Denied?) instead of Allowed.

> +    QString origin = url.string();
> +    emit m_page->checkPermission(origin, permission);
> +    switch (permission) {
> +    case QWebPage::PermissionAllowed:
> +        return NotificationPresenter::PermissionAllowed;
> +    case QWebPage::PermissionNotAllowed:
> +        return NotificationPresenter::PermissionNotAllowed;
> +    case QWebPage::PermissionDenied:
> +        return NotificationPresenter::PermissionDenied;
> +    }

ASSERT_NOT_REACHED would be appreciated here and maybe return just "permission" - which would default to NotAllowed (or Denied?).

>      return NotificationPresenter::PermissionAllowed;
>  }
>  

r- because I think it is a bit early to conclude a public API for this, otherwise it looks great.

[Yael]

Created attachment 56523 [details]
Patch

Moved the new API to DumpRenderTreeSupportQt class, so it is no longer considered public.
Added ASSERT_NOT_REACHED and turned notifications off by default.

[Kenneth Rohde Christiansen]

The Notification system should probably be hooked up in the platform plugin.

Also, neither KDE nor GNOME uses the systray for notifications and it will probably be considered an abuse to do so, so I am not so happy about that.

[Yael]

(In reply to comment #8)
> The Notification system should probably be hooked up in the platform plugin.
> 
> Also, neither KDE nor GNOME uses the systray for notifications and it will probably be considered an abuse to do so, so I am not so happy about that.

The wrapper class allows for multiple implementations, one of which is an interface to the plugin. I could add that code in a separate patch.

[Laszlo Gombos]

(In reply to comment #8)
> The Notification system should probably be hooked up in the platform plugin.

That is certainly the intent - see my previous comment on 

http://bugreports.qt.nokia.com/browse/QTWEBKIT-105?focusedCommentId=115615&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_115615.

> Also, neither KDE nor GNOME uses the systray for notifications and it will probably be considered an abuse to do so, so I am not so happy about that.

Well, Is there something missing from the QSysTray API that would not allow to take full advantage e.g. of the GNOME notification systemm. On MAC for example Qt can be setup so that QSysTray is hooked up with Growl.

QSysTray is a decent default implementation for notifications for QtWebKit and the plugin interface is there to overwrite the default implementation if needed.

[Simon Hausmann]

Comment on attachment 56523 [details]
Patch

WebKit/qt/Api/qwebpage.cpp:1689
 +      connect(mainFrame(), SIGNAL(javaScriptWindowObjectCleared()), d->notificationPresenterClient->d, SLOT(clearNotificationsList()));
Why is clearNotificationsList() connected to this signal?

[Yael]

(In reply to comment #11)
> (From update of attachment 56523 [details])
> WebKit/qt/Api/qwebpage.cpp:1689
>  +      connect(mainFrame(), SIGNAL(javaScriptWindowObjectCleared()), d->notificationPresenterClient->d, SLOT(clearNotificationsList()));
> Why is clearNotificationsList() connected to this signal?
When navigating to a new page we have to delete all the notifications of the current page, or we will have a memory leak.

[Simon Hausmann]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 56523 [details] [details])
> > WebKit/qt/Api/qwebpage.cpp:1689
> >  +      connect(mainFrame(), SIGNAL(javaScriptWindowObjectCleared()), d->notificationPresenterClient->d, SLOT(clearNotificationsList()));
> > Why is clearNotificationsList() connected to this signal?
> When navigating to a new page we have to delete all the notifications of the current page, or we will have a memory leak.

Hm, so shouldn't this be done through a callback from say FrameLoader::clear() to FrameLoaderClient and then propagated to the notifications? Or perhaps WebCore should call NotificationPresenter::clear() or so directly?

Where do the other ports clear the notifications when a new page is loaded?

[Yael]

(In reply to comment #13)
> Hm, so shouldn't this be done through a callback from say FrameLoader::clear() to FrameLoaderClient and then propagated to the notifications? Or perhaps WebCore should call NotificationPresenter::clear() or so directly?

I thought it is the Qt way to use signals and slots instead of direct callbacks.

> 
> Where do the other ports clear the notifications when a new page is loaded?
Only other port is Chrome, and they do leak the notifications object :-)

[Yael]

Comment on attachment 56523 [details]
Patch

Removing review flag for now. I'll replace the signal/slot with a direct call as Simon suggested.

[Yael]

Created attachment 57062 [details]
Patch, remove the signal/slot and bring the patch up-to-date

This patch is very similar to the previous one, just removing the signal/slot suggested by Simon.
While I was waiting for dpranke to re-submit the notifications code that he rolled out (r60193), I also implemented an icon loader for the notification icon.
Should I combine the 2 patches?

[Laszlo Gombos]

Comment on attachment 57062 [details]
Patch, remove the signal/slot and bring the patch up-to-date

Looks good except the ChangeLog seems to be out of date. Yael, can you change that ?

[Yael]

Created attachment 57163 [details]
Patch, updating changelog.

[Yael]

Created attachment 57228 [details]
Update change log comment, 

Previous patch still talks about new signals that are no longer there.

[Yael]

Created attachment 57231 [details]
Not sure why the patch failed to apply, try again.

[Laszlo Gombos]

Comment on attachment 57231 [details]
Not sure why the patch failed to apply, try again.

Looks good to me, r+. Thanks.

[WebKit Commit Bot]

Comment on attachment 57231 [details]
Not sure why the patch failed to apply, try again.

Clearing flags on attachment: 57231

Committed r60348: <http://trac.webkit.org/changeset/60348>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60348 might have broken Qt Linux Release minimal

[Simon Hausmann]

Comment on attachment 57231 [details]
Not sure why the patch failed to apply, try again.


> Index: WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
> ===================================================================
> --- WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp	(revision 60296)
> +++ WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp	(working copy)
> @@ -51,6 +51,7 @@
>  #include "HTMLFormElement.h"
>  #include "HTMLPlugInElement.h"
>  #include "HTTPParsers.h"
> +#include "NotificationPresenterClientQt.h"
>  #include "NotImplemented.h"
>  #include "QNetworkReplyHandler.h"
>  #include "ResourceHandleInternal.h"
> @@ -638,8 +639,12 @@ void FrameLoaderClientQt::dispatchDidCle
>      if (world != mainThreadNormalWorld())
>          return;
>  
> -    if (m_webFrame)
> +    if (m_webFrame) {
>          emit m_webFrame->javaScriptWindowObjectCleared();
> +#if ENABLE(NOTIFICATIONS)
> +        m_webFrame->page()->d->notificationPresenterClient->clearNotificationsList();
> +#endif
> +    }

This part _continues_ to look wrong :-(

Clearing the window object has nothing to do with notifications, and this function is called _per frame_ where as the notifications are cleared per page. So if one sub-frame reloads it clears the notifications for the entire page.

[Yael]

(In reply to comment #25)
> (From update of attachment 57231 [details])
> 
> This part _continues_ to look wrong :-(
> 
> Clearing the window object has nothing to do with notifications, and this function is called _per frame_ where as the notifications are cleared per page. So if one sub-frame reloads it clears the notifications for the entire page.

I have a new patch that changes the behavior drastically. Notifications will no longer be cleared with the window object, but with a timer. (QSystemTrayIcon does not emit a signal when it closes). Also, NotificationPresenter should be one per process, and not per page.

The problem is that it is 40K, and I do not know how to cut it into smaller chunks, without causing tests to fail.

What would be the best way for me to explain that patch? IRC? comments in code? please advice :-)