WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
40003
[Qt] Fix the lifecycle of notification objects
https://bugs.webkit.org/show_bug.cgi?id=40003
Summary
[Qt] Fix the lifecycle of notification objects
Yael
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Yael
Comment 1
2010-06-01 09:48:38 PDT
Created
attachment 57554
[details]
Patch. This patch addresses the issues listed in the bug description.
Kenneth Rohde Christiansen
Comment 2
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!
Yael
Comment 3
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 :-)
Yael
Comment 4
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.
Yael
Comment 5
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.
Yael
Comment 6
2010-06-01 13:39:33 PDT
Created
attachment 57590
[details]
Patch. Added the extra line break in the header file.
Kenneth Rohde Christiansen
Comment 7
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.
Kenneth Rohde Christiansen
Comment 8
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.
Yael
Comment 9
2010-06-02 07:35:06 PDT
Committed
r60566
<
http://trac.webkit.org/changeset/60566
>
Simon Hausmann
Comment 10
2010-08-03 08:30:45 PDT
Revision
r60566
cherry-picked into qtwebkit-2.1 with commit deb68f295591791d00a7a27b8d554be78d7ca69e
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug