Bug 40003

Created attachment 57554 [details] Patch. This patch addresses the issues listed in the bug description.

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!

(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 :-)

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

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.

Created attachment 57590 [details] Patch. Added the extra line break in the header file.

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

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

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

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