Simplify GfxLink using smart pointers

Uses std::shared_ptr for sharing graphic data

Changed constructor to std::unique_ptr<sal_uInt8[]> to ensure
the delete[] operator is called when GfxLink internals takes
ownership of the data

Change-Id: I4edd4634df8d6ba4d94953260c1a7ac560ccf04a
Reviewed-on: https://gerrit.libreoffice.org/25402
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noelgrandin@gmail.com>

2 files changed, 76 insertions, 176 deletions

diff --git a/vcl/source/filter/graphicfilter.cxx b/vcl/source/filter/graphicfilter.cxx
index 2fab78563a68..14088b510eab 100644
--- a/vcl/source/filter/graphicfilter.cxx
+++ b/vcl/source/filter/graphicfilter.cxx
@@ -1334,8 +1334,7 @@ sal_uInt16 GraphicFilter::ImportGraphic( Graphic& rGraphic, const OUString& rPat
     bool                bAllowPartialStreamRead = false;
     bool                bCreateNativeLink = true;
 
-    sal_uInt8* pGraphicContent = nullptr;
-    bool bGraphicContentOwned = true;
+    std::unique_ptr<sal_uInt8[]> pGraphicContent;
     sal_Int32  nGraphicContentSize = 0;
 
     ResetLastError();
@@ -1459,9 +1458,9 @@ sal_uInt16 GraphicFilter::ImportGraphic( Graphic& rGraphic, const OUString& rPat
                             const std::vector<sal_uInt8>& rData = aIter->aData;
                             nGraphicContentSize = nChunkSize - 11;
                             SvMemoryStream aIStrm(const_cast<sal_uInt8*>(&rData[11]), nGraphicContentSize, StreamMode::READ);
-                            pGraphicContent = new sal_uInt8[nGraphicContentSize];
+                            pGraphicContent = std::unique_ptr<sal_uInt8[]>(new sal_uInt8[nGraphicContentSize]);
                             sal_uInt64 aCurrentPosition = aIStrm.Tell();
-                            aIStrm.ReadBytes(pGraphicContent, nGraphicContentSize);
+                            aIStrm.ReadBytes(pGraphicContent.get(), nGraphicContentSize);
                             aIStrm.Seek(aCurrentPosition);
                             ImportGIF(aIStrm, rGraphic);
                             eLinkType = GfxLinkType::NativeGif;
@@ -1534,8 +1533,8 @@ sal_uInt16 GraphicFilter::ImportGraphic( Graphic& rGraphic, const OUString& rPat
 
                         // Make a uncompressed copy for GfxLink
                         nGraphicContentSize = nMemoryLength;
-                        pGraphicContent = new sal_uInt8[nGraphicContentSize];
-                        std::copy(aNewData.begin(), aNewData.end(), pGraphicContent);
+                        pGraphicContent = std::unique_ptr<sal_uInt8[]>(new sal_uInt8[nGraphicContentSize]);
+                        std::copy(aNewData.begin(), aNewData.end(), pGraphicContent.get());
 
                         if(!aMemStream.GetError() )
                         {
@@ -1739,7 +1738,7 @@ sal_uInt16 GraphicFilter::ImportGraphic( Graphic& rGraphic, const OUString& rPat
 
     if( nStatus == GRFILTER_OK && bCreateNativeLink && ( eLinkType != GfxLinkType::NONE ) && !rGraphic.GetContext() && !bLinkSet )
     {
-        if (pGraphicContent == nullptr)
+        if (!pGraphicContent)
         {
             const sal_uLong nStreamEnd = rIStream.Tell();
             nGraphicContentSize = nStreamEnd - nStreamBegin;
@@ -1748,7 +1747,7 @@ sal_uInt16 GraphicFilter::ImportGraphic( Graphic& rGraphic, const OUString& rPat
             {
                 try
                 {
-                    pGraphicContent = new sal_uInt8[nGraphicContentSize];
+                    pGraphicContent =  std::unique_ptr<sal_uInt8[]>(new sal_uInt8[nGraphicContentSize]);
                 }
                 catch (const std::bad_alloc&)
                 {
@@ -1758,20 +1757,16 @@ sal_uInt16 GraphicFilter::ImportGraphic( Graphic& rGraphic, const OUString& rPat
                 if( nStatus == GRFILTER_OK )
                 {
                     rIStream.Seek(nStreamBegin);
-                    rIStream.ReadBytes(pGraphicContent, nGraphicContentSize);
+                    rIStream.ReadBytes(pGraphicContent.get(), nGraphicContentSize);
                 }
             }
         }
         if( nStatus == GRFILTER_OK )
         {
-            rGraphic.SetLink( GfxLink( pGraphicContent, nGraphicContentSize, eLinkType ) );
-            bGraphicContentOwned = false; //ownership passed to the GfxLink
+            rGraphic.SetLink( GfxLink( std::move(pGraphicContent), nGraphicContentSize, eLinkType ) );
         }
     }
 
-    if (bGraphicContentOwned)
-        delete[] pGraphicContent;
-
     // Set error code or try to set native buffer
     if( nStatus != GRFILTER_OK )
     {
diff --git a/vcl/source/gdi/gfxlink.cxx b/vcl/source/gdi/gfxlink.cxx
index c6db9866c3ef..30e74541c1bf 100644
--- a/vcl/source/gdi/gfxlink.cxx
+++ b/vcl/source/gdi/gfxlink.cxx
@@ -29,68 +29,31 @@
 #include <vcl/cvtgrf.hxx>
 #include <com/sun/star/ucb/CommandAbortedException.hpp>
 #include <memory>
+#include <o3tl/make_shared.hxx>
 
-GfxLink::GfxLink() :
-    meType      ( GfxLinkType::NONE ),
-    mpBuf       ( nullptr ),
-    mpSwap      ( nullptr ),
-    mnBufSize   ( 0 ),
-    mnUserId    ( 0UL ),
-    mpImpData   ( new ImpGfxLink )
+GfxLink::GfxLink()
 {
 }
 
-GfxLink::GfxLink( const GfxLink& rGfxLink ) :
-    mpImpData( new ImpGfxLink )
-{
-    ImplCopy( rGfxLink );
-}
-
-GfxLink::GfxLink( sal_uInt8* pBuf, sal_uInt32 nSize, GfxLinkType nType ) :
-    mpImpData( new ImpGfxLink )
+GfxLink::GfxLink( std::unique_ptr<sal_uInt8[]> pBuf, sal_uInt32 nSize, GfxLinkType nType )
 {
     DBG_ASSERT( pBuf != nullptr && nSize,
                 "GfxLink::GfxLink(): empty/NULL buffer given" );
 
     meType = nType;
-    mnBufSize = nSize;
-    mpSwap = nullptr;
-    mnUserId = 0UL;
-    mpBuf = new ImpBuffer( pBuf );
+    mnSwapInDataSize = nSize;
+    mpSwapInData = std::shared_ptr<sal_uInt8>(pBuf.release(), pBuf.get_deleter());  // std::move(pBuf) does not compile on Jenkins MacOSX (24 May 2016)
 }
 
 GfxLink::~GfxLink()
 {
-    if( mpBuf && !( --mpBuf->mnRefCount ) )
-        delete mpBuf;
-
-    if( mpSwap && !( --mpSwap->mnRefCount ) )
-        delete mpSwap;
-
-    delete mpImpData;
-}
-
-GfxLink& GfxLink::operator=( const GfxLink& rGfxLink )
-{
-    if( &rGfxLink != this )
-    {
-        if ( mpBuf && !( --mpBuf->mnRefCount ) )
-            delete mpBuf;
-
-        if( mpSwap && !( --mpSwap->mnRefCount ) )
-            delete mpSwap;
-
-        ImplCopy( rGfxLink );
-    }
-
-    return *this;
 }
 
 bool GfxLink::IsEqual( const GfxLink& rGfxLink ) const
 {
     bool bIsEqual = false;
 
-    if ( ( mnBufSize == rGfxLink.mnBufSize ) && ( meType == rGfxLink.meType ) )
+    if ( ( mnSwapInDataSize == rGfxLink.mnSwapInDataSize ) && ( meType == rGfxLink.meType ) )
     {
         const sal_uInt8* pSource = GetData();
         const sal_uInt8* pDest = rGfxLink.GetData();
@@ -106,22 +69,6 @@ bool GfxLink::IsEqual( const GfxLink& rGfxLink ) const
     return bIsEqual;
 }
 
-void GfxLink::ImplCopy( const GfxLink& rGfxLink )
-{
-    mnBufSize = rGfxLink.mnBufSize;
-    meType = rGfxLink.meType;
-    mpBuf = rGfxLink.mpBuf;
-    mpSwap = rGfxLink.mpSwap;
-    mnUserId = rGfxLink.mnUserId;
-    *mpImpData = *rGfxLink.mpImpData;
-
-    if( mpBuf )
-        mpBuf->mnRefCount++;
-
-    if( mpSwap )
-        mpSwap->mnRefCount++;
-}
-
 
 bool GfxLink::IsNative() const
 {
@@ -134,21 +81,21 @@ const sal_uInt8* GfxLink::GetData() const
     if( IsSwappedOut() )
         const_cast<GfxLink*>(this)->SwapIn();
 
-    return( mpBuf ? mpBuf->mpBuffer : nullptr );
+    return( mpSwapInData.get() );
 }
 
 
 void GfxLink::SetPrefSize( const Size& rPrefSize )
 {
-    mpImpData->maPrefSize = rPrefSize;
-    mpImpData->mbPrefSizeValid = true;
+    maPrefSize = rPrefSize;
+    mbPrefSizeValid = true;
 }
 
 
 void GfxLink::SetPrefMapMode( const MapMode& rPrefMapMode )
 {
-    mpImpData->maPrefMapMode = rPrefMapMode;
-    mpImpData->mbPrefMapModeValid = true;
+    maPrefMapMode = rPrefMapMode;
+    mbPrefMapModeValid = true;
 }
 
 
@@ -156,7 +103,7 @@ bool GfxLink::LoadNative( Graphic& rGraphic )
 {
     bool bRet = false;
 
-    if( IsNative() && mnBufSize )
+    if( IsNative() && mnSwapInDataSize )
     {
         const sal_uInt8* pData = GetData();
 
@@ -165,15 +112,12 @@ bool GfxLink::LoadNative( Graphic& rGraphic )
             SvMemoryStream    aMemStm;
             ConvertDataFormat nCvtType;
 
-            aMemStm.SetBuffer( const_cast<sal_uInt8*>(pData), mnBufSize, mnBufSize );
+            aMemStm.SetBuffer( const_cast<sal_uInt8*>(pData), mnSwapInDataSize, mnSwapInDataSize );
 
             switch( meType )
             {
                 case GfxLinkType::NativeGif: nCvtType = ConvertDataFormat::GIF; break;
-
-                // #i15508# added BMP type for better exports (reload when swapped - checked, works)
                 case GfxLinkType::NativeBmp: nCvtType = ConvertDataFormat::BMP; break;
-
                 case GfxLinkType::NativeJpg: nCvtType = ConvertDataFormat::JPG; break;
                 case GfxLinkType::NativePng: nCvtType = ConvertDataFormat::PNG; break;
                 case GfxLinkType::NativeTif: nCvtType = ConvertDataFormat::TIF; break;
@@ -195,21 +139,28 @@ bool GfxLink::LoadNative( Graphic& rGraphic )
 
 void GfxLink::SwapOut()
 {
-    if( !IsSwappedOut() && mpBuf )
+    if( !IsSwappedOut() && mpSwapInData && mnSwapInDataSize )
     {
-        mpSwap = new ImpSwap( mpBuf->mpBuffer, mnBufSize );
+        ::utl::TempFile aTempFile;
 
-        if( !mpSwap->IsSwapped() )
-        {
-            delete mpSwap;
-            mpSwap = nullptr;
-        }
-        else
+        OUString aURL = aTempFile.GetURL();
+
+        if( !aURL.isEmpty() )
         {
-            if( !( --mpBuf->mnRefCount ) )
-                delete mpBuf;
+            std::shared_ptr<GfxLink::SwapOutData> pSwapOut = std::make_shared<SwapOutData>(aURL);    // aURL is removed in the destructor
+            std::unique_ptr<SvStream> xOStm(::utl::UcbStreamHelper::CreateStream( aURL, STREAM_READWRITE | StreamMode::SHARE_DENYWRITE ));
+            if( xOStm )
+            {
+                xOStm->WriteBytes( mpSwapInData.get(), mnSwapInDataSize );
+                bool bError = ( ERRCODE_NONE != xOStm->GetError() );
+                xOStm.reset();
 
-            mpBuf = nullptr;
+                if( !bError )
+                {
+                    mpSwapOutData = pSwapOut;
+                    mpSwapInData.reset();
+                }
+            }
         }
     }
 }
@@ -218,12 +169,12 @@ void GfxLink::SwapIn()
 {
     if( IsSwappedOut() )
     {
-        mpBuf = new ImpBuffer( mpSwap->GetData() );
-
-        if( !( --mpSwap->mnRefCount ) )
-            delete mpSwap;
-
-        mpSwap = nullptr;
+        auto pData = GetSwapInData();
+        if (pData)
+        {
+            mpSwapInData = pData;
+            mpSwapOutData.reset();
+        }
     }
 }
 
@@ -231,10 +182,9 @@ bool GfxLink::ExportNative( SvStream& rOStream ) const
 {
     if( GetDataSize() )
     {
-        if( IsSwappedOut() )
-            mpSwap->WriteTo( rOStream );
-        else if( GetData() )
-            rOStream.WriteBytes( GetData(), GetDataSize() );
+        auto pData = GetSwapInData();
+        if (pData)
+            rOStream.WriteBytes( pData.get(), mnSwapInDataSize );
     }
 
     return ( rOStream.GetError() == ERRCODE_NONE );
@@ -255,10 +205,9 @@ SvStream& WriteGfxLink( SvStream& rOStream, const GfxLink& rGfxLink )
 
     if( rGfxLink.GetDataSize() )
     {
-        if( rGfxLink.IsSwappedOut() )
-            rGfxLink.mpSwap->WriteTo( rOStream );
-        else if( rGfxLink.GetData() )
-            rOStream.WriteBytes( rGfxLink.GetData(), rGfxLink.GetDataSize() );
+        auto pData = rGfxLink.GetSwapInData();
+        if (pData)
+            rOStream.WriteBytes( pData.get(), rGfxLink.mnSwapInDataSize );
     }
 
     return rOStream;
@@ -270,8 +219,7 @@ SvStream& ReadGfxLink( SvStream& rIStream, GfxLink& rGfxLink)
     MapMode         aMapMode;
     sal_uInt32      nSize;
     sal_uInt32      nUserId;
-    sal_uInt16          nType;
-    sal_uInt8*          pBuf;
+    sal_uInt16      nType;
     bool            bMapAndSizeValid( false );
     std::unique_ptr<VersionCompat>  pCompat(new VersionCompat( rIStream, StreamMode::READ ));
 
@@ -287,10 +235,10 @@ SvStream& ReadGfxLink( SvStream& rIStream, GfxLink& rGfxLink)
 
     pCompat.reset(); // destructor writes stuff into the header
 
-    pBuf = new sal_uInt8[ nSize ];
-    rIStream.ReadBytes( pBuf, nSize );
+    std::unique_ptr<sal_uInt8[]> pBuf(new sal_uInt8[ nSize ]);
+    rIStream.ReadBytes( pBuf.get(), nSize );
 
-    rGfxLink = GfxLink( pBuf, nSize, (GfxLinkType) nType );
+    rGfxLink = GfxLink( std::move(pBuf), nSize, (GfxLinkType) nType );
     rGfxLink.SetUserId( nUserId );
 
     if( bMapAndSizeValid )
@@ -302,83 +250,40 @@ SvStream& ReadGfxLink( SvStream& rIStream, GfxLink& rGfxLink)
     return rIStream;
 }
 
-ImpSwap::ImpSwap( sal_uInt8* pData, sal_uLong nDataSize ) :
-            mnDataSize( nDataSize ),
-            mnRefCount( 1UL )
+GfxLink::SwapOutData::SwapOutData(const OUString &aURL) : maURL(aURL)
 {
-    if( pData && mnDataSize )
-    {
-        ::utl::TempFile aTempFile;
-
-        maURL = aTempFile.GetURL();
-        if( !maURL.isEmpty() )
-        {
-            std::unique_ptr<SvStream> xOStm(::utl::UcbStreamHelper::CreateStream( maURL, STREAM_READWRITE | StreamMode::SHARE_DENYWRITE ));
-            if( xOStm )
-            {
-                xOStm->WriteBytes( pData, mnDataSize );
-                bool bError = ( ERRCODE_NONE != xOStm->GetError() );
-                xOStm.reset();
-
-                if( bError )
-                {
-                    osl_removeFile( maURL.pData );
-                    maURL.clear();
-                }
-            }
-        }
-    }
 }
 
-ImpSwap::~ImpSwap()
+GfxLink::SwapOutData::~SwapOutData()
 {
-    if( IsSwapped() )
+    if( maURL.getLength() > 0 )
         osl_removeFile( maURL.pData );
 }
 
-sal_uInt8* ImpSwap::GetData() const
+std::shared_ptr<sal_uInt8> GfxLink::GetSwapInData() const
 {
-    sal_uInt8* pData;
+    if( !IsSwappedOut() )
+        return mpSwapInData;
+
+    std::shared_ptr<sal_uInt8> pData;
 
-    if( IsSwapped() )
+    std::unique_ptr<SvStream> xIStm(::utl::UcbStreamHelper::CreateStream( mpSwapOutData->maURL, STREAM_READWRITE ));
+    if( xIStm )
     {
-        std::unique_ptr<SvStream> xIStm(::utl::UcbStreamHelper::CreateStream( maURL, STREAM_READWRITE ));
-        if( xIStm )
+        pData = o3tl::make_shared_array<sal_uInt8>(mnSwapInDataSize);
+        xIStm->ReadBytes( pData.get(), mnSwapInDataSize );
+        bool bError = ( ERRCODE_NONE != xIStm->GetError() );
+        sal_Size nActReadSize = xIStm->Tell();
+        if (nActReadSize != mnSwapInDataSize)
         {
-            pData = new sal_uInt8[ mnDataSize ];
-            xIStm->ReadBytes( pData, mnDataSize );
-            bool bError = ( ERRCODE_NONE != xIStm->GetError() );
-            sal_Size nActReadSize = xIStm->Tell();
-            if (nActReadSize != mnDataSize)
-            {
-                bError = true;
-            }
-            xIStm.reset();
-
-            if( bError )
-            {
-                delete[] pData;
-                pData = nullptr;
-            }
+            bError = true;
         }
-        else
-            pData = nullptr;
-    }
-    else
-        pData = nullptr;
+        xIStm.reset();
 
-    return pData;
-}
-
-void ImpSwap::WriteTo( SvStream& rOStm ) const
-{
-    sal_uInt8* pData = GetData();
-
-    if( pData )
-    {
-        rOStm.WriteBytes( pData, mnDataSize );
-        delete[] pData;
+        if( bError )
+            pData.reset();
     }
+    return pData;
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */