Bug 41320 – Add adoptPtr and leakPtr functions for OwnPtr and PassOwnPtr

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Darin Adler]

Add adoptPtr and releasePtr functions for OwnPtr and PassOwnPtr

[Darin Adler]

Created attachment 60051 [details]
Patch

[WebKit Review Bot]

Attachment 60051 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/OwnPtr.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/PassOwnPtr.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 60060 [details]
Patch

[WebKit Review Bot]

Attachment 60060 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/OwnPtr.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/PassOwnPtr.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 60051 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3349242

[Darin Adler]

From the output of the Chromium EWS bot it looks like JavaScriptCore/wtf/MessageQueue.h has some release calls that have to be changed to call leakPtr instead.

[WebKit Review Bot]

Attachment 60060 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3313961

[WebKit Review Bot]

Attachment 60060 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3382020

[WebKit Review Bot]

Attachment 60060 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3376019

[Darin Adler]

Created attachment 60159 [details]
Patch

[Adam Barth]

Comment on attachment 60159 [details]
Patch

I'm slightly nervous about the CrossOriginPreflightResultCache part because I don't understand it fully.

JavaScriptCore/wtf/PassOwnPtr.h:79
 +          PassOwnPtr(PtrType ptr) : m_ptr(ptr) { }
Explicit?  I guess it wasn't explicit before....

JavaScriptCore/wtf/PassOwnPtr.h:85
 +          explicit PassOwnPtr(PtrType ptr) : m_ptr(ptr) { }
How does this differ from the above?

WebCore/css/CSSSegmentedFontFace.cpp:113
 +          if (const SimpleFontData* faceFontData = m_fontFaces[i]->getFontData(fontDescription, syntheticBold, syntheticItalic)) {
Seems slightly unrelated, but yay for removing static_cast

WebCore/loader/CrossOriginPreflightResultCache.cpp:155
 +          delete addResult.first->second;
Is this a real leak that you're fixing at the same time?  Might be better to do in a separate patch.  I don't understand this part well enough to know if it's right.

[Darin Adler]

(In reply to comment #11)
> JavaScriptCore/wtf/PassOwnPtr.h:79
>  +          PassOwnPtr(PtrType ptr) : m_ptr(ptr) { }
> Explicit?  I guess it wasn't explicit before....
> 
> JavaScriptCore/wtf/PassOwnPtr.h:85
>  +          explicit PassOwnPtr(PtrType ptr) : m_ptr(ptr) { }
> How does this differ from the above?

The difference is that it’s private.

> WebCore/css/CSSSegmentedFontFace.cpp:113
>  +          if (const SimpleFontData* faceFontData = m_fontFaces[i]->getFontData(fontDescription, syntheticBold, syntheticItalic)) {
> Seems slightly unrelated, but yay for removing static_cast

The history here is funny. An earlier patch had this erroneously as an OwnPtr.

> WebCore/loader/CrossOriginPreflightResultCache.cpp:155
>  +          delete addResult.first->second;
> Is this a real leak that you're fixing at the same time?  Might be better to do in a separate patch.  I don't understand this part well enough to know if it's right.

I do think it's a real, theoretical leak. I don’t know how to make it actually leak. I could definitely leave that delete out and instead put in a FIXME, then add the deletion in a separate patch.

[Darin Adler]

I’m worried that the Qt-bot said fail, but gave me no details!

[Adam Barth]

boo.  The Qt bot should have details.  The style bot also failed to give results.  Odd.

[Darin Adler]

Committed r62551: <http://trac.webkit.org/changeset/62551>

[WebKit Review Bot]

http://trac.webkit.org/changeset/62551 might have broken Chromium Linux Release