SharedBuffer::createNSData should return a RetainPtr<NSData>
Created attachment 211451 [details] Patch
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?
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.
(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!
Committed r155641: <http://trac.webkit.org/changeset/155641>
Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier.
(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.
(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