[webkit-reviews] review granted: [Bug 121237] SharedBuffer::createNSData should return a RetainPtr<NSData> : [Attachment 211451] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 121237: SharedBuffer::createNSData should return a RetainPtr<NSData>
https://bugs.webkit.org/show_bug.cgi?id=121237

Attachment 211451: Patch
https://bugs.webkit.org/attachment.cgi?id=211451&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?