Bug 159212 – [Coordinated Graphics] Modernize and cleanup CompositingCoordinator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

And some of its related classes.

[Carlos Garcia Campos]

Created attachment 282246 [details]
Patch

[Carlos Garcia Campos]

Created attachment 282417 [details]
Rebased patch

It should apply to current trunk now

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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!

[Carlos Garcia Campos]

Created attachment 282748 [details]
Updated patch

[Carlos Garcia Campos]

Committed r202812: <http://trac.webkit.org/changeset/202812>