[webkit-reviews] review denied: [Bug 39146] [Qt] Pass all web notification layout tests : [Attachment 56493] Updated patch after merge.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 19 12:10:00 PDT 2010


Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 39146: [Qt] Pass all web notification layout tests
https://bugs.webkit.org/show_bug.cgi?id=39146

Attachment 56493: Updated patch after merge.
https://bugs.webkit.org/attachment.cgi?id=56493&action=review

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
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.


More information about the webkit-reviews mailing list