Bug 159212 - [Coordinated Graphics] Modernize and cleanup CompositingCoordinator
Summary: [Coordinated Graphics] Modernize and cleanup CompositingCoordinator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 159209
Blocks: 159259
  Show dependency treegraph
 
Reported: 2016-06-28 06:03 PDT by Carlos Garcia Campos
Modified: 2016-07-04 23:16 PDT (History)
3 users (show)

See Also:


Attachments
Patch (40.52 KB, patch)
2016-06-28 06:09 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (40.84 KB, patch)
2016-06-30 00:37 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (41.06 KB, patch)
2016-07-04 23:02 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-06-28 06:03:01 PDT
And some of its related classes.
Comment 1 Carlos Garcia Campos 2016-06-28 06:09:45 PDT
Created attachment 282246 [details]
Patch
Comment 2 Carlos Garcia Campos 2016-06-30 00:37:32 PDT
Created attachment 282417 [details]
Rebased patch

It should apply to current trunk now
Comment 3 Zan Dobersek 2016-07-03 22:57:49 PDT
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.
Comment 4 Carlos Garcia Campos 2016-07-03 23:22:31 PDT
(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 5 Zan Dobersek 2016-07-04 01:24:27 PDT
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.
Comment 6 Carlos Garcia Campos 2016-07-04 01:29:17 PDT
(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!
Comment 7 Carlos Garcia Campos 2016-07-04 23:02:43 PDT
Created attachment 282748 [details]
Updated patch
Comment 8 Carlos Garcia Campos 2016-07-04 23:16:59 PDT
Committed r202812: <http://trac.webkit.org/changeset/202812>