Bug 39146 - [Qt] Pass all web notification layout tests
Summary: [Qt] Pass all web notification layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-05-14 19:16 PDT by Yael
Modified: 2010-05-31 18:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch. (19.12 KB, patch)
2010-05-14 19:34 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (19.56 KB, patch)
2010-05-19 07:31 PDT, Yael
no flags Details | Formatted Diff | Diff
Updated patch after merge. (19.89 KB, patch)
2010-05-19 08:36 PDT, Yael
laszlo.gombos: review-
Details | Formatted Diff | Diff
Patch (22.60 KB, patch)
2010-05-19 15:21 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch, remove the signal/slot and bring the patch up-to-date (22.58 KB, patch)
2010-05-25 19:03 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch, updating changelog. (22.62 KB, patch)
2010-05-26 14:50 PDT, Yael
no flags Details | Formatted Diff | Diff
Update change log comment, (204.62 KB, patch)
2010-05-27 05:49 PDT, Yael
no flags Details | Formatted Diff | Diff
Not sure why the patch failed to apply, try again. (22.62 KB, patch)
2010-05-27 06:12 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2010-05-14 19:16:53 PDT
Continue the work that has Laszlo started on supporting web notifications in QtWebKit.
Comment 1 Yael 2010-05-14 19:34:17 PDT
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.
Comment 2 Yael 2010-05-19 07:31:41 PDT
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.
Comment 3 Yael 2010-05-19 07:34:57 PDT
Comment on attachment 56487 [details]
Patch

Removing review flag because I need to merge with Robert's work first:-)
Comment 4 Yael 2010-05-19 08:36:08 PDT
Created attachment 56493 [details]
Updated patch after merge.
Comment 5 Kenneth Rohde Christiansen 2010-05-19 11:24:07 PDT
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)
Comment 6 Laszlo Gombos 2010-05-19 12:10:01 PDT
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.
Comment 7 Yael 2010-05-19 15:21:38 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2010-05-19 16:57:01 PDT
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.
Comment 9 Yael 2010-05-19 17:12:27 PDT
(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.
Comment 10 Laszlo Gombos 2010-05-20 05:22:46 PDT
(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.
Comment 11 Simon Hausmann 2010-05-21 01:36:48 PDT
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?
Comment 12 Yael 2010-05-21 05:30:33 PDT
(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.
Comment 13 Simon Hausmann 2010-05-21 06:41:21 PDT
(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?
Comment 14 Yael 2010-05-21 10:20:54 PDT
(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 :-)
Comment 15 Yael 2010-05-21 14:30:32 PDT
Comment on attachment 56523 [details]
Patch

Removing review flag for now. I'll replace the signal/slot with a direct call as Simon suggested.
Comment 16 Yael 2010-05-25 19:03:52 PDT
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?
Comment 17 Laszlo Gombos 2010-05-26 14:26:18 PDT
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 ?
Comment 18 Yael 2010-05-26 14:50:15 PDT
Created attachment 57163 [details]
Patch, updating changelog.
Comment 19 Yael 2010-05-27 05:49:52 PDT
Created attachment 57228 [details]
Update change log comment, 

Previous patch still talks about new signals that are no longer there.
Comment 20 Yael 2010-05-27 06:12:44 PDT
Created attachment 57231 [details]
Not sure why the patch failed to apply, try again.
Comment 21 Laszlo Gombos 2010-05-28 05:00:25 PDT
Comment on attachment 57231 [details]
Not sure why the patch failed to apply, try again.

Looks good to me, r+. Thanks.
Comment 22 WebKit Commit Bot 2010-05-28 06:00:08 PDT
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>
Comment 23 WebKit Commit Bot 2010-05-28 06:00:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Review Bot 2010-05-28 06:07:10 PDT
http://trac.webkit.org/changeset/60348 might have broken Qt Linux Release minimal
Comment 25 Simon Hausmann 2010-05-30 23:06:50 PDT
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.
Comment 26 Yael 2010-05-31 18:59:35 PDT
(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 :-)