Bug 80702

Summary: [Qt][WK2] Implement web notifications support
Product: WebKit Reporter: Adenilson Cavalcanti Silva <savagobr>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
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    
Description Flags
Adding Desktop Notification permission type into Qt
Changelog, fixed coding style in HTML test file
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

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

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)


> 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


> 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

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

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.