Bug 80700 - [Qt] Move notification icon download out of WebCore
: [Qt] Move notification icon download out of WebCore
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Qt, QtTriaged
:
: 79666
  Show dependency treegraph
 
Reported: 2012-03-09 06:30 PST by
Modified: 2012-04-24 18:09 PST (History)


Attachments
Patch. (15.60 KB, patch)
2012-04-23 18:36 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch. (18.62 KB, patch)
2012-04-24 05:43 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 .
------- Comment #1 From 2012-04-20 16:36:00 PST -------
*** Bug 84484 has been marked as a duplicate of this bug. ***
------- Comment #2 From 2012-04-22 13:36:54 PST -------
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 ?
------- Comment #3 From 2012-04-23 00:50:14 PST -------
(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.
------- Comment #4 From 2012-04-23 18:36:26 PST -------
Created an attachment (id=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.
------- Comment #5 From 2012-04-24 01:36:07 PST -------
(From update of attachment 138485 [details])
Clearing flags on attachment: 138485

Committed r115011: <http://trac.webkit.org/changeset/115011>
------- Comment #6 From 2012-04-24 01:36:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #7 From 2012-04-24 02:11:36 PST -------
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.
------- Comment #8 From 2012-04-24 04:25:13 PST -------
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.
------- Comment #9 From 2012-04-24 05:01:22 PST -------
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?
------- Comment #10 From 2012-04-24 05:43:16 PST -------
Created an attachment (id=138543) [details]
Patch.

Remove http/tests/notifications after the code the tests were testing has been removed.
------- Comment #11 From 2012-04-24 18:09:29 PST -------
(From update of attachment 138543 [details])
Clearing flags on attachment: 138543

Committed r115153: <http://trac.webkit.org/changeset/115153>
------- Comment #12 From 2012-04-24 18:09:49 PST -------
All reviewed patches have been landed.  Closing bug.