[Webkit-unassigned] [Bug 39146] [Qt] Pass all web notification layout tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39146


Laszlo Gombos <laszlo.1.gombos at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56493|review?                     |review-
               Flag|                            |




--- Comment #6 from Laszlo Gombos <laszlo.1.gombos at nokia.com>  2010-05-19 12:10:01 PST ---
(From update of attachment 56493)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.