Bug 80700 – [Qt] Move notification icon download out of WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yael]

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 .

[Yael]

*** Bug 84484 has been marked as a duplicate of this bug. ***

[Yael]

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]

(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]

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 on attachment 138485 [details]
Patch.

Clearing flags on attachment: 138485

Committed r115011: <http://trac.webkit.org/changeset/115011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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]

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]

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]

Created attachment 138543 [details]
Patch.

Remove http/tests/notifications after the code the tests were testing has been removed.

[WebKit Review Bot]

Comment on attachment 138543 [details]
Patch.

Clearing flags on attachment: 138543

Committed r115153: <http://trac.webkit.org/changeset/115153>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.