SharedBuffer::createNSData should return a RetainPtr<NSData>

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

SharedBuffer::createNSData should return a RetainPtr<NSData>

[Anders Carlsson]

Created attachment 211451 [details]
Patch

[Darin Adler]

Comment on attachment 211451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211451&action=review

You said you fixed two leaks. I saw the mention of one in the ChangeLog but didn’t spot it, nor the other one. Also, patch did not apply.

> Source/WebCore/loader/ResourceBuffer.h:79
> +    SharedBuffer::NSDataRetainPtr createNSData();

The name SharedBuffer::NSDataRetainPtr does not make it clear why RetainPtr<NSData> is not used; would be better if the name made that clear. And not really sure this type should be a member of SharedBuffer. It doesn’t seem to be an issue that’s specific to that class, even if we are only using it there for now.

Your name suggestion NSDataRetainPtrWithoutImplicitConversionOperator is good.

> Source/WebCore/loader/archive/ArchiveFactory.cpp:67
> +    mimeTypes.set("application/x-webarchive", static_cast<RawDataCreationFunction*>(&archiveFactoryCreate<LegacyWebArchive>));

I don’t know why this cast is needed, nor why it’s safe. Change log doesn’t say.

> Source/WebCore/platform/SharedBuffer.h:68
> +    // Once both Mac and iOS builds with this change we can change the return type to be RetainPtr<NSData>.

Comment should say something like "because this mistake is unlikely in new code, although a real worry during the transition".

> Source/WebCore/platform/mac/SharedBufferMac.mm:101
> +    return adoptNS(static_cast<NSData *>([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]));

Why is the cast to NSData * needed?

> Source/WebKit/mac/WebView/WebDataSource.mm:408
> -    return [mainResourceData->createNSData() autorelease];
> +    return [mainResourceData->createNSData().leakRef() autorelease];

Does RetainPtr need a leakRef/autorelease function of some sort?

[Darin Adler]

Comment on attachment 211451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211451&action=review

>> Source/WebKit/mac/WebView/WebDataSource.mm:408
>> +    return [mainResourceData->createNSData().leakRef() autorelease];
> 
> Does RetainPtr need a leakRef/autorelease function of some sort?

Probably would want to name it autorelease() and then have to decide if it’s legal on CF types or not.

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 211451 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211451&action=review
> 
> You said you fixed two leaks. I saw the mention of one in the ChangeLog but didn’t spot it, nor the other one. Also, patch did not apply.

One was in -[WebHTMLView namesOfPromisedFilesDroppedAtDestination:], the other one was PDFDocumentImage::createPDFDocument.

> 
> > Source/WebCore/loader/archive/ArchiveFactory.cpp:67
> > +    mimeTypes.set("application/x-webarchive", static_cast<RawDataCreationFunction*>(&archiveFactoryCreate<LegacyWebArchive>));
> 
> I don’t know why this cast is needed, nor why it’s safe. Change log doesn’t say.

I’ll remove this.

> 
> > Source/WebCore/platform/SharedBuffer.h:68
> > +    // Once both Mac and iOS builds with this change we can change the return type to be RetainPtr<NSData>.
> 
> Comment should say something like "because this mistake is unlikely in new code, although a real worry during the transition".
> 
> > Source/WebCore/platform/mac/SharedBufferMac.mm:101
> > +    return adoptNS(static_cast<NSData *>([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]));
> 
> Why is the cast to NSData * needed?

It’s because I didn’t make the NSDataRetainPtrWithoutImplicitConversionOperator constructor work on any RetainPtr that can be converted to RetainPtr<NSData>. I’ll fix that.

> 
> > Source/WebKit/mac/WebView/WebDataSource.mm:408
> > -    return [mainResourceData->createNSData() autorelease];
> > +    return [mainResourceData->createNSData().leakRef() autorelease];
> 
> Does RetainPtr need a leakRef/autorelease function of some sort?

If this pattern is common enough, then maybe!

[Anders Carlsson]

Committed r155641: <http://trac.webkit.org/changeset/155641>

[Tim Horton]

Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier.

[Anders Carlsson]

(In reply to comment #6)
> Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier.

Absolutely.

[Pratik Solanki]

(In reply to comment #7)
> (In reply to comment #6)
> > Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier.
> 
> Absolutely.

https://bugs.webkit.org/show_bug.cgi?id=135219