Bug 107438 - [Qt][WK1] Permission request callbacks for non-legacy notifications
Summary: [Qt][WK1] Permission request callbacks for non-legacy notifications
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on: 106699
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-21 03:50 PST by Allan Sandfeld Jensen
Modified: 2013-01-21 06:31 PST (History)
1 user (show)

See Also:


Attachments
Patch (6.01 KB, patch)
2013-01-21 03:56 PST, Allan Sandfeld Jensen
jturcotte: review+
Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2013-01-21 06:02 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-01-21 03:50:58 PST
QtWebKit(1) is missing an implementation tracking and calling the permission request callbacks when using Web Nofications in non-legacy mode.
Comment 1 Allan Sandfeld Jensen 2013-01-21 03:56:37 PST
Created attachment 183756 [details]
Patch
Comment 2 Jocelyn Turcotte 2013-01-21 05:37:20 PST
Comment on attachment 183756 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:332
> +#if ENABLE(NOTIFICATIONS)
> +void NotificationPresenterClientQt::requestPermission(ScriptExecutionContext* context, PassRefPtr<NotificationPermissionCallback> callback)
> +{

This is pretty much duplicated code, could you factor out the common code of the two implementations in a separate method?
Comment 3 Allan Sandfeld Jensen 2013-01-21 06:02:15 PST
Created attachment 183768 [details]
Patch
Comment 4 Jocelyn Turcotte 2013-01-21 06:18:59 PST
Comment on attachment 183756 [details]
Patch

Humm, it doesn't look as good as I thought, so we might as well duplicate the code.
- NotificationPermissionCallback is defined behind ENABLE(NOTIFICATIONS)
- Adding to a different list depending on the callback type isn't great

r+ing the first one
Comment 5 Allan Sandfeld Jensen 2013-01-21 06:31:54 PST
Committed r140330: <http://trac.webkit.org/changeset/140330>