Bug 234290 – Remove unnecessary TiledBacking virtual functions that aren't called by WebCore common layer

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Compositing

[Fujii Hironori]

Remove GraphicsLayerCA specific TiledBacking virtual functions

I'm going to implement TiledBacking subclass for WinCairo (Bug 233676).
TiledBacking has a lot of GraphicsLayerCA specific virtual functions.
Let's remove them.

Covariant return type will help to do that.

https://en.wikipedia.org/wiki/Covariant_return_type
https://en.cppreference.com/w/cpp/language/virtual

[Fujii Hironori]

Created attachment 447108 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 447108 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        TiledBacking had a lot of GraphicsLayerCA specific virtual
> +        functions. It unnecessary made difficult to implement a subclass

I'm not sure I agree with this. Doesn't any platform need the concepts of visible rect, coverage rect and might want to optimize live resize?

[Fujii Hironori]

Comment on attachment 447108 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        functions. It unnecessary made difficult to implement a subclass
> 
> I'm not sure I agree with this. Doesn't any platform need the concepts of visible rect, coverage rect and might want to optimize live resize?

This patch removes unnecessary virtual functions which are not called by WebCore common code.
They are called only by GraphicsLayerCA implementation layer.
If the WebCore common code will need the virtual methods, we can add them.

[Fujii Hironori]

Comment on attachment 447108 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        functions. It unnecessary made difficult to implement a subclass
>> 
>> I'm not sure I agree with this. Doesn't any platform need the concepts of visible rect, coverage rect and might want to optimize live resize?
> 
> This patch removes unnecessary virtual functions which are not called by WebCore common code.
> They are called only by GraphicsLayerCA implementation layer.
> If the WebCore common code will need the virtual methods, we can add them.

I misunderstood the reason why we remove them.
We should remove those TiledBacking virtual functions because they aren't called by WebCore layer, not because they are platform-specific.
I should change the bug title and the change log.

GraphicsContext interface has some platfrom specifix virtual functions.
But, we can't remove them because they are called by WebCore layer, they are needed.

[Simon Fraser (smfr)]

Comment on attachment 447108 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:282
> +    virtual TileController* tiledBacking() = 0;

All the tiledBacking() functions that return a TileController need to be renamed.

[Fujii Hironori]

Comment on attachment 447108 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:282
>> +    virtual TileController* tiledBacking() = 0;
> 
> All the tiledBacking() functions that return a TileController need to be renamed.

How do you think about an idea renaming TileController to CATiledBacking or TiledBackingCA?

[Fujii Hironori]

Comment on attachment 447108 [details]
Patch

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

>>> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:282
>>> +    virtual TileController* tiledBacking() = 0;
>> 
>> All the tiledBacking() functions that return a TileController need to be renamed.
> 
> How do you think about an idea renaming TileController to CATiledBacking or TiledBackingCA?

Because GraphicsLayerCA::tiledBacking() is a override function I can't rename even thouth this patch changed it to return TileController*.
I think renaming TileController seems better.
regardless of whether the method or the class will rename, I think I should separate patches for brevity.
WDYT?

[Fujii Hironori]

Umm, this refactoring shouldn't be controversial. But, I give up this patch.