Summary: | All WebKit/win classes should return COMPtrs from their static constructor members | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | NEW --- | ||||||||
Severity: | Normal | CC: | adachan, sfalken | ||||||
Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2009-04-20 07:42:57 PDT
Created attachment 29616 [details]
patch for MemoryStream
Comment on attachment 29616 [details] patch for MemoryStream > HRESULT STDMETHODCALLTYPE MemoryStream::Clone( > /* [out] */ IStream** ppstm) > { > - *ppstm = MemoryStream::createInstance(m_buffer); > + *ppstm = MemoryStream::createInstance(m_buffer).releaseRef(); > + // FIXME: MSDN says we should be returning STG_E_INSUFFICIENT_MEMORY instead of E_OUTOFMEMORY here. > return (*ppstm) ? S_OK : E_OUTOFMEMORY; Why not copyRefTo here? > - COMPtr<IStream> stream(AdoptCOM, MemoryStream::createInstance(buffer.release())); > + COMPtr<MemoryStream> stream = MemoryStream::createInstance(buffer.release()); > m_view->didReceiveData(stream.get()); Would this read better without the local variable? > - COMPtr<MemoryStream> result(AdoptCOM, MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get()))); > + COMPtr<MemoryStream> result = MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get())); > return result.copyRefTo(stream); Would this read better without the local variable? r=me (In reply to comment #3) > (From update of attachment 29616 [details] [review]) > > HRESULT STDMETHODCALLTYPE MemoryStream::Clone( > > /* [out] */ IStream** ppstm) > > { > > - *ppstm = MemoryStream::createInstance(m_buffer); > > + *ppstm = MemoryStream::createInstance(m_buffer).releaseRef(); > > + // FIXME: MSDN says we should be returning STG_E_INSUFFICIENT_MEMORY instead of E_OUTOFMEMORY here. > > return (*ppstm) ? S_OK : E_OUTOFMEMORY; > > Why not copyRefTo here? I originally thought that copyRefTo's behavior was incompatible with the existing behavior of this function. But now I see that it isn't. I will change to use copyRefTo. > > - COMPtr<IStream> stream(AdoptCOM, MemoryStream::createInstance(buffer.release())); > > + COMPtr<MemoryStream> stream = MemoryStream::createInstance(buffer.release()); > > m_view->didReceiveData(stream.get()); > > Would this read better without the local variable? Probably. I'll remove the variable. > > - COMPtr<MemoryStream> result(AdoptCOM, MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get()))); > > + COMPtr<MemoryStream> result = MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get())); > > return result.copyRefTo(stream); > > Would this read better without the local variable? Maybe. I'll remove the variable and see how it looks. Thanks for the review! Comment on attachment 29616 [details] patch for MemoryStream Landed as r42692 Created attachment 42574 [details]
Make CFDictionaryPropertyBag::createInstance return a COMPtr
Comment on attachment 42574 [details] Make CFDictionaryPropertyBag::createInstance return a COMPtr Landed as r50567 <http://trac.webkit.org/changeset/50567> |