And some of its related classes.
Created attachment 282246 [details] Patch
Created attachment 282417 [details] Rebased patch It should apply to current trunk now
Comment on attachment 282417 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=282417&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:217 > +void CompositingCoordinator::updateImageBacking(CoordinatedImageBackingID imageID, RefPtr<CoordinatedSurface>& coordinatedSurface) I don't think this should be receiving a reference to RefPtr. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:286 > +void CompositingCoordinator::createUpdateAtlas(uint32_t atlasID, RefPtr<CoordinatedSurface>& coordinatedSurface) I don't think this should be receiving a reference to RefPtr. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:357 > + if (auto layer = m_registeredLayers.get(layerID)) auto*, since m_registeredLayers is mapping IDs to pointers to CoordinatedGraphicsLayer objects. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:88 > - std::unique_ptr<CompositingCoordinator> m_coordinator; > + CompositingCoordinator m_coordinator; You can drop WTF_MAKE_FAST_ALLOCATED from CompositingCoordinator class now.
(In reply to comment #3) > Comment on attachment 282417 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282417&action=review Thanks for the review. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:217 > > +void CompositingCoordinator::updateImageBacking(CoordinatedImageBackingID imageID, RefPtr<CoordinatedSurface>& coordinatedSurface) > > I don't think this should be receiving a reference to RefPtr. Could you elaborate? what's wrong with the reference and what should I do instead? just copying the RefPtr? I don't understand whey PassRefPtr is used there TBH. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:286 > > +void CompositingCoordinator::createUpdateAtlas(uint32_t atlasID, RefPtr<CoordinatedSurface>& coordinatedSurface) > > I don't think this should be receiving a reference to RefPtr. > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:357 > > + if (auto layer = m_registeredLayers.get(layerID)) > > auto*, since m_registeredLayers is mapping IDs to pointers to > CoordinatedGraphicsLayer objects. Isn't that automatic? doesn't auto deduce it's a pointer? > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:88 > > - std::unique_ptr<CompositingCoordinator> m_coordinator; > > + CompositingCoordinator m_coordinator; > > You can drop WTF_MAKE_FAST_ALLOCATED from CompositingCoordinator class now. Indeed.
Comment on attachment 282417 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=282417&action=review >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:217 >>> +void CompositingCoordinator::updateImageBacking(CoordinatedImageBackingID imageID, RefPtr<CoordinatedSurface>& coordinatedSurface) >> >> I don't think this should be receiving a reference to RefPtr. > > Could you elaborate? what's wrong with the reference and what should I do instead? just copying the RefPtr? I don't understand whey PassRefPtr is used there TBH. Passing a reference to a non-const RefPtr object here, you'd expect the method to modify it. That's not what's happening here, and if it were, you'd expect the method to return a new RefPtr. A rvalue reference to the RefPtr would make sense, so you'd copyRef() it into the method call, and move it in this method to the std::make_pair() call. >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:286 >>> +void CompositingCoordinator::createUpdateAtlas(uint32_t atlasID, RefPtr<CoordinatedSurface>& coordinatedSurface) >> >> I don't think this should be receiving a reference to RefPtr. > > Isn't that automatic? doesn't auto deduce it's a pointer? It does, but it doesn't tell me that when I look at it. auto* provides more info, shows that it's not a RefPtr<> or an iterator or anything.
(In reply to comment #5) > Comment on attachment 282417 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282417&action=review > > >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:217 > >>> +void CompositingCoordinator::updateImageBacking(CoordinatedImageBackingID imageID, RefPtr<CoordinatedSurface>& coordinatedSurface) > >> > >> I don't think this should be receiving a reference to RefPtr. > > > > Could you elaborate? what's wrong with the reference and what should I do instead? just copying the RefPtr? I don't understand whey PassRefPtr is used there TBH. > > Passing a reference to a non-const RefPtr object here, you'd expect the > method to modify it. That's not what's happening here, and if it were, you'd > expect the method to return a new RefPtr. Ah, I see, I didn't notice it was non-const. > A rvalue reference to the RefPtr would make sense, so you'd copyRef() it > into the method call, and move it in this method to the std::make_pair() > call. > > >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:286 > >>> +void CompositingCoordinator::createUpdateAtlas(uint32_t atlasID, RefPtr<CoordinatedSurface>& coordinatedSurface) > >> > >> I don't think this should be receiving a reference to RefPtr. > > > > Isn't that automatic? doesn't auto deduce it's a pointer? > > It does, but it doesn't tell me that when I look at it. auto* provides more > info, shows that it's not a RefPtr<> or an iterator or anything. Ah, it's just for readability, I agree indeed. I'll submit an updated patch, thanks!
Created attachment 282748 [details] Updated patch
Committed r202812: <http://trac.webkit.org/changeset/202812>