Created attachment 345088 [details]
Patch

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?

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.

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.

Created attachment 345089 [details]
Patch for landing

Comment on attachment 345089 [details]
Patch for landing

Clearing flags on attachment: 345089

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

All reviewed patches have been landed.  Closing bug.

<rdar://problem/42237305>