Bug 80700 - [Qt] Move notification icon download out of WebCore
Summary: [Qt] Move notification icon download out of WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
: 84484 (view as bug list)
Depends on:
Blocks: 79666
  Show dependency treegraph
 
Reported: 2012-03-09 06:30 PST by Yael
Modified: 2012-04-24 18:09 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 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 Yael 2012-04-20 16:36:00 PDT
*** Bug 84484 has been marked as a duplicate of this bug. ***
Comment 2 Yael 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 ?
Comment 3 Simon Hausmann 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.
Comment 4 Yael 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.
Comment 5 WebKit Review Bot 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>
Comment 6 WebKit Review Bot 2012-04-24 01:36:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 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.
Comment 8 Szilard Ledan 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.
Comment 9 Yael 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?
Comment 10 Yael 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.
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2012-04-24 18:09:49 PDT
All reviewed patches have been landed.  Closing bug.