Bug 188344 – [Nicosia] Add the TextureMapper-specific ContentLayer::Impl derivative

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[Nicosia] Add the TextureMapper-specific ContentLayer::Impl derivative

[Zan Dobersek]

Created attachment 346623 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 346623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346623&action=review

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.cpp:47
> +    : m_proxy(adoptRef(*new WebCore::TextureMapperPlatformLayerProxy))

I would add a create() to TextureMapperPlatformLayerProxy

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.h:53
> +    ContentLayerTextureMapperImpl(Client&);

explicit. This should only be called by the factory, right? Can we make it private? or does it have to be public because of make_unique?

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.h:59
> +    const Ref<WebCore::TextureMapperPlatformLayerProxy>& proxy() const { return m_proxy; }

Why returning a const Ref<>&? why not just const WebCore::TextureMapperPlatformLayerProxy& instead?

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.h:66
> +        Client* object { nullptr };

I would call this client instead of object, even if redundant it's less confusing, I think.

[Zan Dobersek]

Comment on attachment 346623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346623&action=review

>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.cpp:47
>> +    : m_proxy(adoptRef(*new WebCore::TextureMapperPlatformLayerProxy))
> 
> I would add a create() to TextureMapperPlatformLayerProxy

I'll add it separately.

>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.h:53
>> +    ContentLayerTextureMapperImpl(Client&);
> 
> explicit. This should only be called by the factory, right? Can we make it private? or does it have to be public because of make_unique?

Can't be private because of std::make_unique<>().

>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.h:59
>> +    const Ref<WebCore::TextureMapperPlatformLayerProxy>& proxy() const { return m_proxy; }
> 
> Why returning a const Ref<>&? why not just const WebCore::TextureMapperPlatformLayerProxy& instead?

Has to be a non-const reference. But here this works because the implicit conversion operator on the Ref<> class drops the const reference through indirection.

>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.h:66
>> +        Client* object { nullptr };
> 
> I would call this client instead of object, even if redundant it's less confusing, I think.

OK, makes sense.

[Zan Dobersek]

Created attachment 346698 [details]
Patch

[Zan Dobersek]

Comment on attachment 346698 [details]
Patch

Clearing flags on attachment: 346698

Committed r234643: <https://trac.webkit.org/changeset/234643>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42999654>