[Webkit-unassigned] [Bug 25294] All WebKit/win classes should return COMPtrs from their static constructor members

[bugzilla-daemon at webkit.org]

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





------- Comment #4 from aroben at apple.com  2009-04-20 09:23 PDT -------
(In reply to comment #3)
> (From update of attachment 29616 [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!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.