Bug 80700

Summary: [Qt] Move notification icon download out of WebCore
Product: WebKit Reporter: Yael <yael>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, jianli, jonlee, ossy, savagobr, szledan, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79666    
Attachments:
Description Flags
Patch.
none
Patch. none

Yael
Reported 2012-03-09 06:30:14 PST
Initially, I thought that ports other than Qt would want to use this code, but it seems that only Qt is using this. Moreover, the downloaded icon is not passed to WebKit2 and that triggers 2 downloads. We should move the code that downloads the notification icon to WebKit/qt .
Attachments
Patch. (15.60 KB, patch)
2012-04-23 18:36 PDT, Yael 		no flags
DetailsFormatted DiffDiff
Patch. (18.62 KB, patch)
2012-04-24 05:43 PDT, Yael 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Yael
Comment 1 2012-04-20 16:36:00 PDT
*** Bug 84484 has been marked as a duplicate of this bug. ***
Yael
Comment 2 2012-04-22 13:36:54 PDT
Simon, if I remove the code to download the notifications icon out of webcore, can I change the API in the platform plugin to take a url instead of the icon itself, or should I reimplement the download code in webkit1 ?
Simon Hausmann
Comment 3 2012-04-23 00:50:14 PDT
(In reply to comment #2) > Simon, if I remove the code to download the notifications icon out of webcore, can I change the API in the platform plugin to take a url instead of the icon itself, or should I reimplement the download code in webkit1 ? Feel free to change the API of the platform plugin to suit your needs. Just remember to change the version part of the interface string at the bottom of qwebkitplatformplugin.h and also adjust the example that has a copy of the file.
Yael
Comment 4 2012-04-23 18:36:26 PDT
Created attachment 138485 [details] Patch. Remove the code that handles the notification's icon and use the icon url instead. The "display" event has to be asynchronous, so a timer was added to achieve that. The platform plugin API and example were updated to reflect this change. This change does not address the recent changes in web notifications spec.
WebKit Review Bot
Comment 5 2012-04-24 01:36:07 PDT
Comment on attachment 138485 [details] Patch. Clearing flags on attachment: 138485 Committed r115011: <http://trac.webkit.org/changeset/115011>
WebKit Review Bot
Comment 6 2012-04-24 01:36:11 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 7 2012-04-24 02:11:36 PDT
Reopen, because it broke a test: --- /ramdisk/qt-linux-release/build/layout-test-results/http/tests/notifications/icon-exists-cancel-expected.txt +++ /ramdisk/qt-linux-release/build/layout-test-results/http/tests/notifications/icon-exists-cancel-actual.txt @@ -1,5 +1,7 @@ +DESKTOP NOTIFICATION: icon http://127.0.0.1:8000/notifications/resources/icon-exists.png, title New E-mail, text Meet me tonight at 8! +DESKTOP NOTIFICATION CLOSED: New E-mail DESKTOP NOTIFICATION: icon http://127.0.0.1:8000/notifications/resources/icon-exists.png, title Second E-mail, text Actually, meet me tonight at 9! Sending notification, cancelling it while the icon is loading and sending a second notification. Only the second notification should be displayed -PASS: display event invoked. +FAIL: close event invoked.
Szilard Ledan
Comment 8 2012-04-24 04:25:13 PDT
This patch made http/tests/notifications/icon-exists-cancel.html fail. This test has been skipped until it is fixed. See http://trac.webkit.org/changeset/115024 Please unskip it with the proper fix.
Yael
Comment 9 2012-04-24 05:01:22 PDT
Sorry for this :) I added http/tests/notifications tests in order to test the icon loading, which I just removed. Any objection to removing the tests, since the code is removed too?
Yael
Comment 10 2012-04-24 05:43:16 PDT
Created attachment 138543 [details] Patch. Remove http/tests/notifications after the code the tests were testing has been removed.
WebKit Review Bot
Comment 11 2012-04-24 18:09:29 PDT
Comment on attachment 138543 [details] Patch. Clearing flags on attachment: 138543 Committed r115153: <http://trac.webkit.org/changeset/115153>
WebKit Review Bot
Comment 12 2012-04-24 18:09:49 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.