We currently store raw pointers to WebCore::GraphicsLayer in the LayerMap meaning that we need to delete them manually. We could use smart pointers and store them as OwnPtr in the LayerMap. The LayerMap would own the layers and we would not need to handle memory manually.
Created attachment 176759 [details] Patch
Comment on attachment 176759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176759&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:183 > + typedef HashMap< WebLayerID, OwnPtr<WebCore::GraphicsLayer> > LayerMap; I dont think the first space is needed > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:185 > + typedef HashMap<WebLayerID, WebCore::GraphicsLayer*> RawLayerMap; LayerPtrMap ?
Comment on attachment 176759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176759&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-375 > m_fixedLayers.remove(layerID); > #if USE(GRAPHICS_SURFACE) > m_surfaceBackingStores.remove(layerID); > #endif > - delete layer; so how are they stored here
Comment on attachment 176759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176759&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-375 >> - delete layer; > > so how are they stored here Sorry, I don't get the question. Is the comment on the right line? I'm merely removing the explicit deletion here since it will get deleted when the OwnPtr goes out of scope. >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:183 >> + typedef HashMap< WebLayerID, OwnPtr<WebCore::GraphicsLayer> > LayerMap; > > I dont think the first space is needed It is not but I like symmetry :) Anyway, if the other way is more common I will change. >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:185 >> + typedef HashMap<WebLayerID, WebCore::GraphicsLayer*> RawLayerMap; > > LayerPtrMap ? Is it really clearer? They are both pointers (Ptr), the different is that one is raw and the other isn't. LayerRawPtrMap ?
(In reply to comment #4) > (From update of attachment 176759 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176759&action=review > > >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:-375 > >> - delete layer; > > > > so how are they stored here > > Sorry, I don't get the question. Is the comment on the right line? > I'm merely removing the explicit deletion here since it will get deleted when the OwnPtr goes out of scope. I was refering to m_surfaceBackingStores.remove(layerID); but that is out of scope for this patch > >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:183 > >> + typedef HashMap< WebLayerID, OwnPtr<WebCore::GraphicsLayer> > LayerMap; > > > > I dont think the first space is needed > > It is not but I like symmetry :) Anyway, if the other way is more common I will change. Dont do this, it is uncommon and as you can see not done in RawLayerMap > > >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:185 > >> + typedef HashMap<WebLayerID, WebCore::GraphicsLayer*> RawLayerMap; > > > > LayerPtrMap ? > > Is it really clearer? They are both pointers (Ptr), the different is that one is raw and the other isn't. LayerRawPtrMap ? yes I like LayerRawPtrMap more
Created attachment 176771 [details] Patch Take Kenneth's feedback into consideration.
Comment on attachment 176771 [details] Patch This is fine with me. Leaving cq still at ? in case Kenneth has more comments :)
Comment on attachment 176771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176771&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:366 > + OwnPtr<GraphicsLayer> layer = m_layers.take(layerID); Is it OwnPtr(const OwnPtr<ValueType>&); ? // This copy constructor is used implicitly by gcc when it generates // transients for assigning a PassOwnPtr<T> object to a stack-allocated // OwnPtr<T> object. It should never be called explicitly and gcc // should optimize away the constructor when generating code.
Comment on attachment 176771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176771&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:366 >> + OwnPtr<GraphicsLayer> layer = m_layers.take(layerID); > > Is it OwnPtr(const OwnPtr<ValueType>&); ? > // This copy constructor is used implicitly by gcc when it generates > // transients for assigning a PassOwnPtr<T> object to a stack-allocated > // OwnPtr<T> object. It should never be called explicitly and gcc > // should optimize away the constructor when generating code. take() returns a PassOwnPtr here so it is fine.
Comment on attachment 176771 [details] Patch Clearing flags on attachment: 176771 Committed r136181: <http://trac.webkit.org/changeset/136181>
All reviewed patches have been landed. Closing bug.
I'm afraid whether it is safe for HashMap to contain OwnPtr. AFAIK, Vector<OwnPtr<T> > is unsafe because relocating can call destructor and constructor. I don't know detail of HashMap implementation, so I can not say it is unsafe surely. However, I'm afraid. In common, we don't store autoptr in stl::HashMap.
(In reply to comment #12) > I'm afraid whether it is safe for HashMap to contain OwnPtr. > > AFAIK, Vector<OwnPtr<T> > is unsafe because relocating can call destructor and constructor. > > I don't know detail of HashMap implementation, so I can not say it is unsafe surely. However, I'm afraid. > > In common, we don't store autoptr in stl::HashMap. The WTF::HashMap has a specific behaviour for OwnPtr defined in HashTraits.h. I don't think this is unsafe and it is also used in other places in WebKit.
(In reply to comment #13) > (In reply to comment #12) > > I'm afraid whether it is safe for HashMap to contain OwnPtr. > > > > AFAIK, Vector<OwnPtr<T> > is unsafe because relocating can call destructor and constructor. > > > > I don't know detail of HashMap implementation, so I can not say it is unsafe surely. However, I'm afraid. > > > > In common, we don't store autoptr in stl::HashMap. > > The WTF::HashMap has a specific behaviour for OwnPtr defined in HashTraits.h. I don't think this is unsafe and it is also used in other places in WebKit. Thanks for explanation!