[Webkit-unassigned] [Bug 40004] [Qt] Support for loading notification icons

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40004





--- Comment #8 from Yael <yael.aharon at nokia.com>  2010-06-07 10:48:07 PST ---
(In reply to comment #7)
> (From update of attachment 57803 [details])
> WebKit/qt/WebCoreSupport/NotificationIconLoader.cpp:44
>  +  NotificationIconLoader::NotificationIconLoader(NotificationPresenterClientQt* presenter, ScriptExecutionContext* context, const KURL& url)
> It this NotificationIconLoader supposed to be Qt only? Currently it seems that way as it received a *ClientQt. Should it be renamed to QtNotificationIconLoader or is it possible to make it take a NotificationPresenterClient instead?
> 

The design in Chrome is very different than our design. They simply wrap the Notification object with a WebNotification, and pass it to the client. I suggested to make the icon loader cross platform, but John Greg told me that if I do that, Chrome would not use it.

> WebKit/qt/WebCoreSupport/NotificationIconLoader.cpp:75
>  +      m_data += QByteArray(data, lengthReceived);
> Seems a bit Qt'ish so far :-)
> 
This class is for Qt port only. I always wonder if I should use WebCore types or Qt types for code that lives in WebKit/Qt ?

> WebKit/qt/WebCoreSupport/NotificationIconLoader.cpp:83
>  +  void NotificationIconLoader::didFail(const ResourceError&)
> Shouldn't you handle the error somehow?
> 
When loading fails for whatever reason, I cleanup the loader and display the notification without an icon. 
Can you suggest a better way to handle the error?

> 103     if (notification->iconURL().isEmpty())
> 104         displayNotification(notification, QByteArray());
> 105     else
> 106         startLoadNotificationIcon(notification);
ok.

> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:296
>  +  void NotificationPresenterClientQt::removeReplacedNotificationFromLoadQueue(Notification* notification)
> Why isnt it removed when being replaced?

"Replaced" is a name of a property :-)
The idea is to find another notification that has the same "Replaced" property and replace the old one with the new one.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.