Bug 40003 – [Qt] Fix the lifecycle of notification objects

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Yael]

Notification objects are not tightly related to the page that created them, and should be decoupled from the page.

- Create one NotificationPresenter that handles all notifications.
- Add ref/deref to the notification objects when they are added/removed from the queue of active notifications. The same technique is used for XMLHttpRequest.
- Instead of deleting all notifications associated with a page when the page is navigated, delete them on a timer, using the same timeout that QSystemTrayIcon is using.
- Break up the show() method into smaller methods.
- Use OwnPtr instead of raw pointer for QSystemTrayIcon.
- Move creating the QIcon to just before showing it in the QSyetemTrayIcon.

[Yael]

Created attachment 57554 [details]
Patch.

This patch addresses the issues listed in the bug description.

[Kenneth Rohde Christiansen]

Comment on attachment 57554 [details]
Patch.

WebKit/qt/Api/qwebpage.cpp:302
 +      NotificationPresenterClientQt::notificationPresenter()->addPage();
isn't it more like a addClient? You are adding a page as a client right?

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:57
 +      s_notificationPresenter = new NotificationPresenterClientQt();
So we always leak this one? (im not finished reading the patch), maybe we should use STATIC_LOCAL

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:67
 +      m_closeTimer.startOneShot(10.0);
Maybe create a static global for the timeout value.

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:88
 +          delete this;
Ah you delete it here.

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:83
 +  void NotificationPresenterClientQt::removePage()
I don't like these names, remove/addPage, because it is not obvious from the names that they are used for ref counting.

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h:83
 +      void addPage() { m_pageCount++; }
even harder when one part of the implementation is defined in the header

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h:93
 +      int m_pageCount;
add a new line between the dumpshowText() and the variables

Apart from these nits, I like the patch!

[Yael]

(In reply to comment #2)
> (From update of attachment 57554 [details])
> WebKit/qt/Api/qwebpage.cpp:302
>  +      NotificationPresenterClientQt::notificationPresenter()->addPage();
> isn't it more like a addClient? You are adding a page as a client right?
ok

> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:57
>  +      s_notificationPresenter = new NotificationPresenterClientQt();
> So we always leak this one? (im not finished reading the patch), maybe we should use STATIC_LOCAL
> 
The last page will trigger deleting it, It will not leak. 
Will add STATIC_LOCAL.

> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:67
>  +      m_closeTimer.startOneShot(10.0);
> Maybe create a static global for the timeout value.
> 
ok

> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:88
>  +          delete this;
> Ah you delete it here.
> 
:-)

> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:83
>  +  void NotificationPresenterClientQt::removePage()
> I don't like these names, remove/addPage, because it is not obvious from the names that they are used for ref counting.
> 
Will change to add/removeClient as you suggested.

> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h:83
>  +      void addPage() { m_pageCount++; }
> even harder when one part of the implementation is defined in the header
> 


> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h:93
>  +      int m_pageCount;
> add a new line between the dumpshowText() and the variables
> 
ok

> Apart from these nits, I like the patch!
Thank you, and thank you for the review :-)

[Yael]

(In reply to comment #2)
> (From update of attachment 57554 [details])
> WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:57
>  +      s_notificationPresenter = new NotificationPresenterClientQt();
> So we always leak this one? (im not finished reading the patch), maybe we should use STATIC_LOCAL

Maybe I am missing something, but if I use DEFINE_STATIC_LOCAL, I don't see how to get access to the static variable in order to delete it in the end.

[Yael]

Created attachment 57587 [details]
Patch.

Rename addPage/removePage/m_pageCount to addClient/removeClient/m_clientCount and make the timeout a static global.

I did not define the notification presenter with DEFINE_STATIC_LOCAL, because I could not find a way to delete it if I made it a static in a local function. Please advice if there is a way to do that.

[Yael]

Created attachment 57590 [details]
Patch.

Added the extra line break in the header file.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 57554 [details] [details])
> > WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:57
> >  +      s_notificationPresenter = new NotificationPresenterClientQt();
> > So we always leak this one? (im not finished reading the patch), maybe we should use STATIC_LOCAL
> 
> Maybe I am missing something, but if I use DEFINE_STATIC_LOCAL, I don't see how to get access to the static variable in order to delete it in the end.

I should have removed that comment. I did it before I had seen all of the patch, sorry.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> Created an attachment (id=57587) [details]
> Patch.
> 
> Rename addPage/removePage/m_pageCount to addClient/removeClient/m_clientCount and make the timeout a static global.
> 
> I did not define the notification presenter with DEFINE_STATIC_LOCAL, because I could not find a way to delete it if I made it a static in a local function. Please advice if there is a way to do that.

DEFINE_STATIC_LOCAL is for when you do not intent deleting it, ever :-) it makes sure that when you exit the application it will leak. Why? Because the OS is quicker at cleaning up then C++ :-)

Please disregard my original comment.

[Yael]

Committed r60566 <http://trac.webkit.org/changeset/60566>

[Simon Hausmann]

Revision r60566 cherry-picked into qtwebkit-2.1 with commit deb68f295591791d00a7a27b8d554be78d7ca69e