Bug 187693 – [Nicosia] Add Nicosia::PlatformLayer, Nicosia::CompositionLayer classes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[Nicosia] Add Nicosia::PlatformLayer, Nicosia::CompositionLayer classes

[Zan Dobersek]

Created attachment 345088 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 345088 [details]
Patch

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

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:45
> +    PlatformLayer(uint64_t);

explicit. If this is abstract, it's probably better to make the constructor protected.

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:48
> +    virtual bool isCompositionLayer() const { return false; }

Should this be pure virtual instead?

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:62
> +    CompositionLayer(uint64_t);

explicit. Since this is refcounted, I think it would be better to add a create function returning a Ref<> and make the constructor private.

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:70
> +                : value(0)

Could we init this below?

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:89
> +                uint32_t value;

with { 0 } here?

> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:112
> +                uint32_t value;

Same here, and only init in the constructor the values that are true, is that possible?

[Zan Dobersek]

Comment on attachment 345088 [details]
Patch

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

>> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:45
>> +    PlatformLayer(uint64_t);
> 
> explicit. If this is abstract, it's probably better to make the constructor protected.

OK.

>> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:48
>> +    virtual bool isCompositionLayer() const { return false; }
> 
> Should this be pure virtual instead?

Other methods corresponding to additional derived classes will be added here. By default they all return false, but in the derived classes the appropriate method is overridden to return true. This is typical for class hierarchies that leverage is<>() and downcast<>() typecasting.

>> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:62
>> +    CompositionLayer(uint64_t);
> 
> explicit. Since this is refcounted, I think it would be better to add a create function returning a Ref<> and make the constructor private.

OK.

>> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:89
>> +                uint32_t value;
> 
> with { 0 } here?

I don't think we can because of this being an union.

>> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:112
>> +                uint32_t value;
> 
> Same here, and only init in the constructor the values that are true, is that possible?

Similar here, don't think it's possible because of the union.

[Zan Dobersek]

Comment on attachment 345088 [details]
Patch

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

>>> Source/WebCore/platform/graphics/nicosia/NicosiaPlatformLayer.h:89
>>> +                uint32_t value;
>> 
>> with { 0 } here?
> 
> I don't think we can because of this being an union.

Turns out we can.

[Zan Dobersek]

Created attachment 345089 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 345089 [details]
Patch for landing

Clearing flags on attachment: 345089

Committed r233845: <https://trac.webkit.org/changeset/233845>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42237305>