Bug 133337

Summary: [EFL][CoordinatedGraphics] Remove CoordinatedTileClient and CoordinatedTileBackend
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, gyuyoung.kim, hw1008.kim, kondapallykalyan, lucas.de.marchi, luiz, noam, yoon
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2014-05-27 21:52:11 PDT 
Both CoordinatedTileClient and TiledBackingStore::Client are implemented by CoordinatedGraphicsLayer.
So, I suggest to simplify them by removing CoordinatedTileClient.
Comment 1 Ryuan Choi 2014-05-27 22:01:02 PDT 
Created attachment 232168 [details]
Patch
Comment 2 Hyowon Kim 2014-05-27 22:28:56 PDT 
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.
Comment 3 Ryuan Choi 2014-05-28 00:06:20 PDT 
(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.
Comment 4 Ryuan Choi 2014-05-28 20:39:56 PDT 
(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.
Comment 5 Hyowon Kim 2014-05-28 21:25:34 PDT 
(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.
Comment 6 Ryuan Choi 2014-06-02 01:26:20 PDT 
yoon, could you take a loot at this?
Comment 7 Gyuyoung Kim 2015-02-03 07:34:35 PST 
Ryuan, is this patch still valid for EFL port ?
Comment 8 Ryuan Choi 2015-02-25 18:53:23 PST 
Created attachment 247389 [details]
Patch
Comment 9 Ryuan Choi 2015-02-25 19:00:15 PST 
(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.
Comment 10 Ryuan Choi 2015-05-26 05:29:30 PDT 
Created attachment 253704 [details]
Patch
Comment 11 WebKit Commit Bot 2015-05-26 14:39:14 PDT 
Comment on attachment 253704 [details]
Patch

Clearing flags on attachment: 253704

Committed r184876: <http://trac.webkit.org/changeset/184876>
Comment 12 WebKit Commit Bot 2015-05-26 14:39:23 PDT 
All reviewed patches have been landed.  Closing bug.