Bug 95100

Summary: [WK2] Add SPI to retrieve internal IDs for web notifications
Product: WebKit Reporter: Jon Lee <jonlee>
Component: Tools / TestsAssignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, ap, cmarcelo, d-r, gustavo, hausmann, jberlin, kenneth, menard, ossy, philn, sam, tmpsantos, tonikitoo, vestbo, webkit-bug-importer, webkit.review.bot, xan.lopez, zherczeg, zoltan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 77969    
Attachments:
Description Flags
Patch
none
Patch
none
Patch ap: review+

Description Jon Lee 2012-08-27 10:14:57 PDT
The IDs used internally by WebKit2 to keep track of notifications need to be exposed to WTR. WTR will have a notification provider, and for that provider to simulate a user click on a shown notification, it needs to be able to map the JS Notification object to an ID.
Comment 1 Radar WebKit Bug Importer 2012-08-27 10:15:05 PDT
<rdar://problem/12180208>
Comment 2 Jon Lee 2012-08-27 14:37:42 PDT
Created attachment 160808 [details]
Patch
Comment 3 Jon Lee 2012-08-27 14:40:57 PDT
The WebCore part of the patch is the same as that in 95099. I included it in both so that I could try to parallelize the approval paths.
Comment 4 Build Bot 2012-08-27 15:07:09 PDT
Comment on attachment 160808 [details]
Patch

Attachment 160808 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13622252
Comment 5 Gyuyoung Kim 2012-08-27 15:33:26 PDT
Comment on attachment 160808 [details]
Patch

Attachment 160808 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13637046
Comment 6 Early Warning System Bot 2012-08-27 15:53:07 PDT
Comment on attachment 160808 [details]
Patch

Attachment 160808 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13639050
Comment 7 Jon Lee 2012-08-27 16:54:17 PDT
Created attachment 160859 [details]
Patch
Comment 8 Early Warning System Bot 2012-08-27 18:00:44 PDT
Comment on attachment 160859 [details]
Patch

Attachment 160859 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13645146
Comment 9 Jon Lee 2012-08-28 10:59:02 PDT
CC'ing a couple Qt guys for assistance with the qt-wk2 EWS error:

...ews/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:56:36: error: WebCore/JSNotification.h: No such file or directory

I need to include a derived source header (JSNotification.h) as a private header in WebCore for WK2. (Which you can see reflected in the mac port in WebCore.xcodeproj/project.pbxproj.) I'm not sure how to do that for Qt. Any pointers?
Comment 10 Csaba Osztrogonác 2012-08-28 23:43:59 PDT
(In reply to comment #9)
> CC'ing a couple Qt guys for assistance with the qt-wk2 EWS error:
> 
> ...ews/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:56:36: error: WebCore/JSNotification.h: No such file or directory
> 
> I need to include a derived source header (JSNotification.h) as a private header in WebCore for WK2. (Which you can see reflected in the mac port in WebCore.xcodeproj/project.pbxproj.) I'm not sure how to do that for Qt. Any pointers?

Including a derived source header as forwarding header is tricky a 
little bit on Qt. The following snippet will make the build happy:

--- a/Source/WebKit2/DerivedSources.pri
+++ b/Source/WebKit2/DerivedSources.pri
@@ -22,6 +22,7 @@ WEBCORE_GENERATED_HEADERS_FOR_WEBKIT2 += \
     $$WEBCORE_GENERATED_SOURCES_DIR/JSElement.h \
     $$WEBCORE_GENERATED_SOURCES_DIR/JSHTMLElement.h \
     $$WEBCORE_GENERATED_SOURCES_DIR/JSNode.h \
+    $$WEBCORE_GENERATED_SOURCES_DIR/JSNotification.h \
     $$WEBCORE_GENERATED_SOURCES_DIR/JSRange.h \
Comment 11 Jon Lee 2012-08-29 00:24:29 PDT
Created attachment 161154 [details]
Patch
Comment 12 Alexey Proskuryakov 2012-08-29 09:49:11 PDT
Comment on attachment 161154 [details]
Patch

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

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:115
> +uint64_t WebNotificationManager::notificationIDForTesting(WebCore::Notification* notification)

No need for WebCore prefix - there is a using directive in this file.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.h:75
>      // For testing purposes only.
>      void removeAllPermissionsForTesting();
> +    uint64_t notificationIDForTesting(WebCore::Notification*);

The comment looks redundant.
Comment 13 Jon Lee 2012-08-29 10:43:31 PDT
(In reply to comment #12)
> (From update of attachment 161154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161154&action=review
> 
> > Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:115
> > +uint64_t WebNotificationManager::notificationIDForTesting(WebCore::Notification* notification)
> 
> No need for WebCore prefix - there is a using directive in this file.
> 
> > Source/WebKit2/WebProcess/Notifications/WebNotificationManager.h:75
> >      // For testing purposes only.
> >      void removeAllPermissionsForTesting();
> > +    uint64_t notificationIDForTesting(WebCore::Notification*);
> 
> The comment looks redundant.

Fixed. Thanks!
Comment 14 Jon Lee 2012-08-29 10:47:25 PDT
Committed r127019: <http://trac.webkit.org/changeset/127019>