[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 #2 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-06-01 12:03:13 PST ---
(From update of attachment 57554)
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!

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