Bug 234290

Summary: Remove unnecessary TiledBacking virtual functions that aren't called by WebCore common layer
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: CompositingAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED WONTFIX    
Severity: Normal CC: simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 233676    
Attachments:
Description Flags
Patch none

Description Fujii Hironori 2021-12-13 21:50:47 PST
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
Comment 1 Fujii Hironori 2021-12-13 22:08:31 PST
Created attachment 447108 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-12-14 09:31:55 PST
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?
Comment 3 Fujii Hironori 2021-12-14 12:01:50 PST
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.
Comment 4 Fujii Hironori 2021-12-14 12:19:09 PST
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.
Comment 5 Simon Fraser (smfr) 2021-12-14 23:10:38 PST
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.
Comment 6 Fujii Hironori 2021-12-15 03:24:14 PST
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?
Comment 7 Fujii Hironori 2021-12-15 23:05:26 PST
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?
Comment 8 Fujii Hironori 2021-12-16 13:18:32 PST
Umm, this refactoring shouldn't be controversial. But, I give up this patch.