Summary: | [Qt] Memory leaks for QWebPageClient | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yi Shen <max.hong.shen> | ||||||||||||||||||
Component: | WebKit Qt | Assignee: | Yi Shen <max.hong.shen> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, kenneth, kling, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Yi Shen
2010-11-30 13:28:18 PST
Created attachment 75192 [details]
first try
Created attachment 75229 [details]
remove unnecessary stuff
Comment on attachment 75229 [details]
remove unnecessary stuff
Could it be an OwnPtr instead?
Comment on attachment 75229 [details] remove unnecessary stuff Rejecting patch 75229 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1 ERROR: Working directory has local commits, pass --force-clean to continue. Full output: http://queues.webkit.org/results/6789010 Comment on attachment 75229 [details]
remove unnecessary stuff
try it again
Comment on attachment 75229 [details] remove unnecessary stuff Rejecting patch 75229 from commit-queue. yi.4.shen@nokia.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. (In reply to comment #3) > (From update of attachment 75229 [details]) > Could it be an OwnPtr instead? hmm, I will look at it later today. Thanks for the suggestion :) Comment on attachment 75229 [details] remove unnecessary stuff Rejecting patch 75229 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1 ERROR: Working directory has local commits, pass --force-clean to continue. Full output: http://queues.webkit.org/results/6793007 Created attachment 75579 [details]
try again
Don't know why it was rejected by the commit bot, yael said it is a bug of commit bot, so just try it again. Thanks!
(In reply to comment #3) > (From update of attachment 75229 [details]) > Could it be an OwnPtr instead? Tried OwnPtr but seems many places need to be changed... Comment on attachment 75579 [details]
try again
Eek. Manual delete for the loss! Don't we have smart pointers we can use here?
(In reply to comment #11) > (From update of attachment 75579 [details]) > Eek. Manual delete for the loss! Don't we have smart pointers we can use here? Qt Smart pointer like QPointer, doesn't work for QWebPageClient, since it is not derived from QObject. (In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 75579 [details] [details]) > > Eek. Manual delete for the loss! Don't we have smart pointers we can use here? > Qt Smart pointer like QPointer, doesn't work for QWebPageClient, since it is not derived from QObject. You should just make the changes for it to be a OwnPtr or similar. It is not part of our public api, so you can easily do that. (In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 75579 [details] [details] [details]) > > > Eek. Manual delete for the loss! Don't we have smart pointers we can use here? > > Qt Smart pointer like QPointer, doesn't work for QWebPageClient, since it is not derived from QObject. > > You should just make the changes for it to be a OwnPtr or similar. It is not part of our public api, so you can easily do that. OK, I will do it :) Created attachment 75821 [details]
use OwnPtr
Attachment 75821 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75821 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75821 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75833 [details]
try again
Does buildbot hate my patch? :( I run the check-webkit-style on my machine and there is no error at all.
Attachment 75821 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75835 [details]
remove unnecessary stuff
Attachment 75821 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061
Died at WebKitTools/Scripts/update-webkit line 132.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75833 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061
Died at WebKitTools/Scripts/update-webkit line 132.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75835 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061
Died at WebKitTools/Scripts/update-webkit line 132.
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 75835 [details] remove unnecessary stuff View in context: https://bugs.webkit.org/attachment.cgi?id=75835&action=review > WebKit/qt/Api/qgraphicswebview.cpp:83 > + return static_cast<PageClientQGraphicsWidget*>(page->d->client.get())->overlay; Seems there should be a d->pageClient() accessor which does this cast for you, no? > WebKit/qt/Api/qwebview.cpp:-355 > - if (page->d->client && page->d->client->isQWidgetClient()) So the isQWidgetClient path was always taken? > WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:-1538 > - QWebPageClient* client = m_webFrame->page()->d->client; The local was helpful to readabilty here. A .get() would have worked fine. Created attachment 76240 [details]
updated with eric's suggestion
Comment on attachment 76240 [details] updated with eric's suggestion View in context: https://bugs.webkit.org/attachment.cgi?id=76240&action=review > WebKit/qt/Api/qwebpage.cpp:379 > +WebCore::PageClientQWidget* QWebPagePrivate::pageClientQWidget() this is normally not how we do the cast wrappers. Somethink like static PageClientQWidget* toPageClientQWidget(QWebPageClient* client) you also want thwse to be inline it just seems more appropriate adding a method like this to QWebViewPrivate. like PageClientQWidget* QWebViewPrivate::pageClient() (In reply to comment #27) > (From update of attachment 76240 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76240&action=review > > > WebKit/qt/Api/qwebpage.cpp:379 > > +WebCore::PageClientQWidget* QWebPagePrivate::pageClientQWidget() > > this is normally not how we do the cast wrappers. > > Somethink like > > static PageClientQWidget* toPageClientQWidget(QWebPageClient* client) > > you also want thwse to be inline > > it just seems more appropriate adding a method like this to QWebViewPrivate. like > > PageClientQWidget* QWebViewPrivate::pageClient() Thanks Kenneth, I see what you mean. I will update patch today :) Created attachment 76310 [details]
try again
Comment on attachment 76310 [details] try again View in context: https://bugs.webkit.org/attachment.cgi?id=76310&action=review > WebKit/qt/Api/qwebpage_p.h:164 > + OwnPtr<QWebPageClient> client; we should rename QWebPageClient to just QtPageClient to not make it seem as a class defined in Qt while still showing that it is a qt port specific class The commit-queue encountered the following flaky tests while processing attachment 76310 [details]: fast/preloader/script.html bug 50879 (author: abarth@webkit.org) fast/loader/recursive-before-unload-crash.html bug 50880 (authors: beidson@apple.com and eric@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 76310 [details] try again Clearing flags on attachment: 76310 Committed r73867: <http://trac.webkit.org/changeset/73867> All reviewed patches have been landed. Closing bug. |