Bug 133337 – [EFL][CoordinatedGraphics] Remove CoordinatedTileClient and CoordinatedTileBackend

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

Both CoordinatedTileClient and TiledBackingStore::Client are implemented by CoordinatedGraphicsLayer.
So, I suggest to simplify them by removing CoordinatedTileClient.

[Ryuan Choi]

Created attachment 232168 [details]
Patch

[Hyowon Kim]

Your patch replaces CoordinatedTileClient with CoordinatedTiledBackingStoreClient inherited from TiledBackingStoreClient.
But we don't have a CoordinatedTiledBackingStore class.
I'm not sure this is a proper approach.

[Ryuan Choi]

(In reply to comment #2)
> Your patch replaces CoordinatedTileClient with CoordinatedTiledBackingStoreClient inherited from TiledBackingStoreClient.
> But we don't have a CoordinatedTiledBackingStore class.
> I'm not sure this is a proper approach.

Right.
I can't find similar cases in WebKit.
Although there are some examples which extends Client (TrackPrivateBaseClient and InbandTextTrackPrivateClient), they also have instances.

I can make CoordinatedTiledBackingStore, but I think that it looks bit weird.

In my understanding, CoordinatedGraphicsLayer uses the extension of TiledBackingStore system, especially Tile and callbacks of TiledBackingStore.


By the way, TiledBackingStore is now only used for Coordinated Graphics system.

So, another suggestion is merging these extension into TiledBackingStore.

[Ryuan Choi]

(In reply to comment #3)
> (In reply to comment #2)
> > Your patch replaces CoordinatedTileClient with CoordinatedTiledBackingStoreClient inherited from TiledBackingStoreClient.
> > But we don't have a CoordinatedTiledBackingStore class.
> > I'm not sure this is a proper approach.
> 
> Right.
> I can't find similar cases in WebKit.
> Although there are some examples which extends Client (TrackPrivateBaseClient and InbandTextTrackPrivateClient), they also have instances.
> 
> I can make CoordinatedTiledBackingStore, but I think that it looks bit weird.
> 
> In my understanding, CoordinatedGraphicsLayer uses the extension of TiledBackingStore system, especially Tile and callbacks of TiledBackingStore.
> 
> 
> By the way, TiledBackingStore is now only used for Coordinated Graphics system.
> 
> So, another suggestion is merging these extension into TiledBackingStore.

Although I removed at Bug 133341, DocumentThreadableLoaderClient inherit ThreadableLoaderClient like CoordinatedTiledBackingStoreClient.
(It was dead code since chromium port is out)

Another suggestion, which merges CoordinatedTiledBackingStoreClient to TiledBackingStoreClient, looks not simple because Atlas and CoordinatedSurface and CoordinatedTile::Client are tightly coupled.

In my analyzing, it requires the changes of callback structure so I want to do it in another bug.

[Hyowon Kim]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Your patch replaces CoordinatedTileClient with CoordinatedTiledBackingStoreClient inherited from TiledBackingStoreClient.
> > > But we don't have a CoordinatedTiledBackingStore class.
> > > I'm not sure this is a proper approach.
> > 
> > Right.
> > I can't find similar cases in WebKit.
> > Although there are some examples which extends Client (TrackPrivateBaseClient and InbandTextTrackPrivateClient), they also have instances.
> > 
> > I can make CoordinatedTiledBackingStore, but I think that it looks bit weird.
> > 
> > In my understanding, CoordinatedGraphicsLayer uses the extension of TiledBackingStore system, especially Tile and callbacks of TiledBackingStore.
> > 
> > 
> > By the way, TiledBackingStore is now only used for Coordinated Graphics system.
> > 
> > So, another suggestion is merging these extension into TiledBackingStore.
> 
> Although I removed at Bug 133341, DocumentThreadableLoaderClient inherit ThreadableLoaderClient like CoordinatedTiledBackingStoreClient.
> (It was dead code since chromium port is out)
> 
> Another suggestion, which merges CoordinatedTiledBackingStoreClient to TiledBackingStoreClient, looks not simple because Atlas and CoordinatedSurface and CoordinatedTile::Client are tightly coupled.
> 
> In my analyzing, it requires the changes of callback structure so I want to do it in another bug.

To extend TiledBackingStoreClient looks better than to add CoordinatedTiledBackingStoreClient. And I think the rename of interfaces in CoordinatedTileClient is also needed when merging them into TiledBackingStoreClient.

[Ryuan Choi]

yoon, could you take a loot at this?

[Gyuyoung Kim]

Ryuan, is this patch still valid for EFL port ?

[Ryuan Choi]

Created attachment 247389 [details]
Patch

[Ryuan Choi]

(In reply to comment #7)
> Ryuan, is this patch still valid for EFL port ?

Sure, this is just a kind of clean up.

I rebased.

I think that TiledBackingStore is now just a part of Coordinated Graphics.
So, I think that we can simplify them more.

[Ryuan Choi]

Created attachment 253704 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253704 [details]
Patch

Clearing flags on attachment: 253704

Committed r184876: <http://trac.webkit.org/changeset/184876>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.