[Webkit-unassigned] [Bug 40003] [Qt] Fix the lifecycle of notification objects

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Yael <yael.aharon at nokia.com>  2010-06-01 12:52:42 PST ---
(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 :-)

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