Bug 80702

Summary: [Qt][WK2] Implement web notifications support
Product: WebKit Reporter: Adenilson Cavalcanti Silva <savagobr>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dherbolt, jonlee, kenneth, menard, savagobr, vestbo, webkit.review.bot, yael, zalan, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 88899    
Attachments:
Description Flags
Adding Desktop Notification permission type into Qt
none
Changelog, fixed coding style in HTML test file
none
Fixing issues with naming convention, indenting none

Description Adenilson Cavalcanti Silva 2012-03-09 06:46:28 PST
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/
Comment 1 Adenilson Cavalcanti Silva 2012-03-09 06:59:41 PST
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
Comment 2 Jon Lee 2012-04-20 13:17:04 PDT
*** Bug 84484 has been marked as a duplicate of this bug. ***
Comment 3 Adenilson Cavalcanti Silva 2012-06-12 09:42:12 PDT
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)
Comment 4 Adenilson Cavalcanti Silva 2012-06-12 09:44:30 PDT
Ouch! Forgot the CLs. I will update a new patch soon.
Comment 5 WebKit Review Bot 2012-06-12 09:45:29 PDT
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.
Comment 6 Adenilson Cavalcanti Silva 2012-06-12 12:41:23 PDT
Created attachment 147135 [details]
Changelog, fixed coding style in HTML test file

Fixed code style, added ChangeLog information.
Comment 7 Adenilson Cavalcanti Silva 2012-06-12 12:51:05 PDT
Added new bug #88899 to reference the plugin task.
Comment 8 Alexis Menard (darktears) 2012-06-12 13:51:25 PDT
Could you please clear the review flag and obselete the patch that is not for review (I believe the first one is not).
Comment 9 Adenilson Cavalcanti Silva 2012-06-12 16:06:31 PDT
Alexis

Done, previous patch is marked as obsolete.
Comment 10 Kenneth Rohde Christiansen 2012-06-13 00:46:19 PDT
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
Comment 11 Adenilson Cavalcanti Silva 2012-06-13 20:18:11 PDT
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
Comment 12 Adenilson Cavalcanti Silva 2012-06-13 20:23:05 PDT
Created attachment 147473 [details]
Fixing issues with naming convention, indenting
Comment 13 Noam Rosenthal 2012-07-12 10:14:02 PDT
Comment on attachment 147473 [details]
Fixing issues with naming convention, indenting

LGTM, and no one seemed to have looked at it in a month...
Comment 14 WebKit Review Bot 2012-07-12 10:21:23 PDT
Comment on attachment 147473 [details]
Fixing issues with naming convention, indenting

Clearing flags on attachment: 147473

Committed r122478: <http://trac.webkit.org/changeset/122478>
Comment 15 WebKit Review Bot 2012-07-12 10:21:29 PDT
All reviewed patches have been landed.  Closing bug.