Bug 40003 - [Qt] Fix the lifecycle of notification objects
Summary: [Qt] Fix the lifecycle of notification objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 39995 40004 40005
  Show dependency treegraph
 
Reported: 2010-06-01 09:20 PDT by Yael
Modified: 2010-08-03 08:30 PDT (History)
2 users (show)

See Also:


Attachments
Patch. (24.42 KB, patch)
2010-06-01 09:48 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (24.49 KB, patch)
2010-06-01 13:31 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (24.49 KB, patch)
2010-06-01 13:39 PDT, Yael
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2010-06-01 09:20:12 PDT
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.
Comment 1 Yael 2010-06-01 09:48:38 PDT
Created attachment 57554 [details]
Patch.

This patch addresses the issues listed in the bug description.
Comment 2 Kenneth Rohde Christiansen 2010-06-01 12:03:13 PDT
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!
Comment 3 Yael 2010-06-01 12:52:42 PDT
(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 :-)
Comment 4 Yael 2010-06-01 13:10:48 PDT
(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.
Comment 5 Yael 2010-06-01 13:31:43 PDT
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.
Comment 6 Yael 2010-06-01 13:39:33 PDT
Created attachment 57590 [details]
Patch.

Added the extra line break in the header file.
Comment 7 Kenneth Rohde Christiansen 2010-06-01 18:23:17 PDT
(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.
Comment 8 Kenneth Rohde Christiansen 2010-06-01 18:25:04 PDT
(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.
Comment 9 Yael 2010-06-02 07:35:06 PDT
Committed r60566 <http://trac.webkit.org/changeset/60566>
Comment 10 Simon Hausmann 2010-08-03 08:30:45 PDT
Revision r60566 cherry-picked into qtwebkit-2.1 with commit deb68f295591791d00a7a27b8d554be78d7ca69e