Bug 188344 - [Nicosia] Add the TextureMapper-specific ContentLayer::Impl derivative
Summary: [Nicosia] Add the TextureMapper-specific ContentLayer::Impl derivative
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks: 187385
  Show dependency treegraph
 
Reported: 2018-08-06 04:05 PDT by Zan Dobersek
Modified: 2018-08-07 02:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.01 KB, patch)
2018-08-06 04:23 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2018-08-07 02:03 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2018-08-06 04:05:45 PDT
[Nicosia] Add the TextureMapper-specific ContentLayer::Impl derivative
Comment 1 Zan Dobersek 2018-08-06 04:23:22 PDT
Created attachment 346623 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-08-07 00:00:36 PDT
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.
Comment 3 Zan Dobersek 2018-08-07 01:58:59 PDT
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.
Comment 4 Zan Dobersek 2018-08-07 02:03:01 PDT
Created attachment 346698 [details]
Patch
Comment 5 Zan Dobersek 2018-08-07 02:03:59 PDT
Comment on attachment 346698 [details]
Patch

Clearing flags on attachment: 346698

Committed r234643: <https://trac.webkit.org/changeset/234643>
Comment 6 Zan Dobersek 2018-08-07 02:04:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-08-07 02:05:20 PDT
<rdar://problem/42999654>