Bug 80702 – [Qt][WK2] Implement web notifications support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Adenilson Cavalcanti Silva]

Since last December support for WK2 has landed, missing to implement the notification provider and required infrastructure (permissions, etc) in Qt port.

For reference the spec is: http://www.w3.org/TR/notifications/

And discussion targeting to change its behavior is ongoing at: http://lists.w3.org/Archives/Public/public-web-notification/2012Mar/

[Adenilson Cavalcanti Silva]

Started it yesterday (it is really crude yet), it is missing
a) Finish the provider
b) Update the notification permission manager

https://gist.github.com/2006840

[Jon Lee]

*** Bug 84484 has been marked as a duplicate of this bug. ***

[Adenilson Cavalcanti Silva]

Created attachment 147101 [details]
Adding Desktop Notification permission type into Qt

I decided to split the patch in 2: a) Having support for permission type Desktop Notification (this patch) and b) The notification plugin (should be next patch)

[Adenilson Cavalcanti Silva]

Ouch! Forgot the CLs. I will update a new patch soon.

[WebKit Review Bot]

Attachment 147101 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/UIProcess/API/qt/qwebpermis..." exit_code: 1
Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/notification.html:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adenilson Cavalcanti Silva]

Created attachment 147135 [details]
Changelog, fixed coding style in HTML test file

Fixed code style, added ChangeLog information.

[Adenilson Cavalcanti Silva]

Added new bug #88899 to reference the plugin task.

[Alexis Menard (darktears)]

Could you please clear the review flag and obselete the patch that is not for review (I believe the first one is not).

[Adenilson Cavalcanti Silva]

Alexis

Done, previous patch is marked as obsolete.

[Kenneth Rohde Christiansen]

Comment on attachment 147135 [details]
Changelog, fixed coding style in HTML test file

View in context: https://bugs.webkit.org/attachment.cgi?id=147135&action=review

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:33
> +        , geoRequest(geo)
> +        , notifyRequest(notify)

geolocationRequest
notificationRequest

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:72
> +QWebPermissionRequest::QWebPermissionRequest(WKSecurityOriginRef securityOrigin
> +                                             , WKGeolocationPermissionRequestRef geo
> +                                             , WKNotificationPermissionRequestRef notify
> +                                             , QWebPermissionRequest::RequestType type
> +                                             , QObject* parent)

wrong indentation, this should just be one line

Wouldn't two different ctors make sense?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_notification.qml:20
> +        //Must be false by default

space

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_notification.qml:24
> +        if (!permission.allow) {
> +           permission.allow = true
> +        } else
> +           console.log("Fail: permission must be set to false")

inconsistent coding style

[Adenilson Cavalcanti Silva]

Kenneth

Thanks a lot for your comments, I will upload a patch with fixes.

I have just 1 question: about the indentation in QWebPermissionRequestPrivate constructor. Do you want all parameters to be in 1 single line?

If I understood correctly, from the webkit coding sytle, it says: "Each member (and superclass) should be indented on a separate line, with the colon or comma preceding the member on that line." and has an example similar to the patch.

Next, about the constructor: currently it has 4 parameters. I can see that if in future we need to treat more permission types, it would make sense to have multiple constructors. But ATM, there are only 2 permission types, so I feel that the code will be simpler (i.e. less member functions) in this way.


Best regards


Adenilson

[Adenilson Cavalcanti Silva]

Created attachment 147473 [details]
Fixing issues with naming convention, indenting

[Noam Rosenthal]

Comment on attachment 147473 [details]
Fixing issues with naming convention, indenting

LGTM, and no one seemed to have looked at it in a month...

[WebKit Review Bot]

Comment on attachment 147473 [details]
Fixing issues with naming convention, indenting

Clearing flags on attachment: 147473

Committed r122478: <http://trac.webkit.org/changeset/122478>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.