Bug 40004 – [Qt] Support for loading notification icons

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Yael]

Add an icon loader to load the icon for the notification.
A patch is ready but it depends heavily on the patch at bug #40003, so it will not apply until that patch is committed.

[Yael]

Created attachment 57798 [details]
Patch

If the icon url exists, create a ThreadableLoader to handle loading the icon, and keep the notification in a load queue.
Once the icon is loaded, or an error occurred, move the notification to the active queue and continue handling as before.

Added some more lookup logic for the second queue. If reviewers prefer, I will create a template for that logic, to reduce the amount of code.

[Yael]

Created attachment 57801 [details]
Patch

Previous patch contained an unrelated change. Sorry.

[WebKit Review Bot]

Attachment 57801 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/WebCoreSupport/NotificationIconLoader.h:35:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/NotificationIconLoader.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 57803 [details]
Fix style issues.

[Kenneth Rohde Christiansen]

Comment on attachment 57803 [details]
Fix style issues.

Looks good to me, thought I probably need some more time to fully understand the code.

Is Chrome doing this the name way? or what are the differences?

[Yael]

(In reply to comment #5)
> (From update of attachment 57803 [details])
> Looks good to me, thought I probably need some more time to fully understand the code.
> 
> Is Chrome doing this the name way? or what are the differences?

Thank you for taking the time to look at this.
Chrome's approach is very different than mine. They wrap the Notification object with a WebNotification object, and call the UI to handle it. They keep their queue outside of WebKit.
I was asked that notifications will work "out of the box" in QtWebKit, so I could not use the same approach.

Also, Chrome was not deleting the Notification objects until r60569 which caused an unfortunate regression (r60590), so I think they have some more work to do.

[Kenneth Rohde Christiansen]

Comment on attachment 57803 [details]
Fix style issues.

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?

WebKit/qt/WebCoreSupport/NotificationIconLoader.cpp:75
 +      m_data += QByteArray(data, lengthReceived);
Seems a bit Qt'ish so far :-)

WebKit/qt/WebCoreSupport/NotificationIconLoader.cpp:83
 +  void NotificationIconLoader::didFail(const ResourceError&)
Shouldn't you handle the error somehow?

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

It would seem more logical to do this the other way around

if (has icon)
   load it


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

[Yael]

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

[Yael]

(In reply to comment #8)
> (In reply to comment #7)
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.

To be more specific: 
If the "old" notification, that has the same "Replaced" attribute is in the load queue, we cancel its load and remove it.
If the "old" notification is currently showing, and is in the active notifications queue, we first download the new icon, and then we replace the notification.

[Kenneth Rohde Christiansen]

> 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.

What about the other ports? Have you talked to the Gtk guys?

[Yael]

I can think of 2 ways of implementing an icon loader in cross platform code.
1. Notification object could inherit ThreadableLoaderClient, and handle the download itself, the same way as XMLHttpRequest does.
2. Create a NotificationPresenterProxy class to handle the download. This new class will also implement the interface NotificationPresenter, and will proxy between the NotificationCenter and the platform specific NotificationPresenter.

Advantage of first solution is that it will encapsulate all Notification logic in the Notification object itself.
Advantage of the second solution is that it does not require #ifdef in the code. Ports that don't want to download icons in cross platform code, don't use the NotificationPresenterProxy.

Any opinions on which way to go?

[Laszlo Gombos]

(In reply to comment #10)
> > 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.
> 
> What about the other ports? Have you talked to the Gtk guys?

I do not see a problem keeping the implementation specific to Qt initially. As Yael mentioned for technical reasons it would be difficult to share much code with Chrome, which is the only other ports at the moment that supports notifications. 

If other ports come along (e.g. Gtk) we can certainly move the implementation cross-platform, but at the moment QtWebKit is the only port where this patch can be really tested.

[John Gregg]

I would support putting the icon loading code in Notification rather than creating the PresenterProxy.  Chrome may not use it in the current implementation, but could still have reasons to use it in the future, and it seems like the better cross-platform design in general.

[Yael]

Created attachment 58402 [details]
Patch

Take into use the icon downloading from r60960.

[Yael]

Committed r60981: <http://trac.webkit.org/changeset/60981>

[Simon Hausmann]

Revision r60981 cherry-picked into qtwebkit-2.1 with commit 18e3033811c283d0fa84ca78a828056cdbe35e6b