Bug 143001 – [CoordinatedGraphics] Merge TILED_BACKING_STORE guard with COORDINATED_GRAPHICS

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

[Ryuan Choi]

Now, TILED_BACKING_STORE is only used with COORDINATED_GRAPHICS.

So, I think that we'd better to merge them.

[Ryuan Choi]

Created attachment 249363 [details]
Patch

[Ryuan Choi]

yoon, what do you think about this?

[Gwang Yoon Hwang]

(In reply to comment #2)
> yoon, what do you think about this?

I agree to remove TILED_BACKING_STORE. 
But I think it is better integrated
with Mac Port's TiledBacking because
it provides almost same feature set.

Also, AFAIK, Win Cairo ports using
TILED_BACKING_STORE without coordinated
graphics. We need to check also.

[Ryuan Choi]

(In reply to comment #3)
> (In reply to comment #2)
> > yoon, what do you think about this?
> 
> I agree to remove TILED_BACKING_STORE. 
> But I think it is better integrated
> with Mac Port's TiledBacking because
> it provides almost same feature set.
> 
Agree.
Can we use TiledBacking in Coordinated Graphics without big changes ?
Then, I will check it.
If I remember correctly, you had a plan to merge coordinated graphics and mac's one (RemoteLayerTree?).

> Also, AFAIK, Win Cairo ports using
> TILED_BACKING_STORE without coordinated
> graphics. We need to check also.

I don't think so. Now we can't use Tiled backing store without coordinated graphics.
For example, TileCairo.cpp was removed very long time ago.(I did)

[Gwang Yoon Hwang]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > yoon, what do you think about this?
> > 
> > I agree to remove TILED_BACKING_STORE. 
> > But I think it is better integrated
> > with Mac Port's TiledBacking because
> > it provides almost same feature set.
> > 
> Agree.
> Can we use TiledBacking in Coordinated Graphics without big changes ?
> Then, I will check it.
I cannot say it is a small change. :) But there is no big difference between
Apple's and our tiled backing store.


> If I remember correctly, you had a plan to merge coordinated graphics and
> mac's one (RemoteLayerTree?).

Yes, merging Coordinated Graphics and Apple's UI side compositing is planned as a todo.
Unifying TILED_BACKING_STORE and TiledBacking should be done for this plan.
Apparently, having TiledBackingStore and TiledBacking in same namespace is quite confusing.

> > Also, AFAIK, Win Cairo ports using
> > TILED_BACKING_STORE without coordinated
> > graphics. We need to check also.
> 
> I don't think so. Now we can't use Tiled backing store without coordinated
> graphics.
> For example, TileCairo.cpp was removed very long time ago.(I did)

Ah, I confused with TexMap.

[Ryuan Choi]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > yoon, what do you think about this?
> > > 
> > > I agree to remove TILED_BACKING_STORE. 
> > > But I think it is better integrated
> > > with Mac Port's TiledBacking because
> > > it provides almost same feature set.
> > > 
> > Agree.
> > Can we use TiledBacking in Coordinated Graphics without big changes ?
> > Then, I will check it.
> I cannot say it is a small change. :) But there is no big difference between
> Apple's and our tiled backing store.

I am not good to make big impact. :)
Anyway, I will start to investigate TiledBacking.

> 
> 
> > If I remember correctly, you had a plan to merge coordinated graphics and
> > mac's one (RemoteLayerTree?).
> 
> Yes, merging Coordinated Graphics and Apple's UI side compositing is planned
> as a todo.
> Unifying TILED_BACKING_STORE and TiledBacking should be done for this plan.
> Apparently, having TiledBackingStore and TiledBacking in same namespace is
> quite confusing.

Yes, so I want to move it.
I think that we can move it first until we have clear picture to share them.

[Gyuyoung Kim]

According to review comments, now we seem to need to decide whether we will merge tiled backing store with mac port's one. Is it easy work ? It seems not to easy work for me. Anyway I don't have big opinion about merging these two macros though, I usually like to remove unnecessary macro.

Yoon, do you agree that we can land this merge first, then try to merge with mac port's one ?

[Gwang Yoon Hwang]

(In reply to comment #7)
> According to review comments, now we seem to need to decide whether we will
> merge tiled backing store with mac port's one. Is it easy work ? It seems
> not to easy work for me. Anyway I don't have big opinion about merging these
> two macros though, I usually like to remove unnecessary macro.
> 
> Yoon, do you agree that we can land this merge first, then try to merge with
> mac port's one ?

It looks like Ryuan don't have a time to merge TBS to mac port's one.
I agree to remove TILED_BACKING_STORE first.

I'll merge tiled backing store with mac port's one later.

[Gyuyoung Kim]

Comment on attachment 249363 [details]
Patch

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

> Source/WebCore/PlatformGTK.cmake:-405
> -        platform/graphics/TiledBackingStore.cpp

I don't know if GTK port enables ENABLE_THREADED_COMPOSITOR though, don't you need to add "platform/graphics/texmap/coordinated/TiledBackingStore.cpp" instead of platform/graphics/TiledBackingStore.cpp for gtk port ?

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:7757
> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">true</ExcludedFromBuild>

I don't know .vcxproj file well, but why do we need add these options ?

[Gwang Yoon Hwang]

Comment on attachment 249363 [details]
Patch

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

>> Source/WebCore/PlatformGTK.cmake:-405
>> -        platform/graphics/TiledBackingStore.cpp
> 
> I don't know if GTK port enables ENABLE_THREADED_COMPOSITOR though, don't you need to add "platform/graphics/texmap/coordinated/TiledBackingStore.cpp" instead of platform/graphics/TiledBackingStore.cpp for gtk port ?

GTK port uses threaded compositor in optional manner. And again, we need to add TiledBackingStore in this source list.

[Ryuan Choi]

Comment on attachment 249363 [details]
Patch

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

>>> Source/WebCore/PlatformGTK.cmake:-405
>>> -        platform/graphics/TiledBackingStore.cpp
>> 
>> I don't know if GTK port enables ENABLE_THREADED_COMPOSITOR though, don't you need to add "platform/graphics/texmap/coordinated/TiledBackingStore.cpp" instead of platform/graphics/TiledBackingStore.cpp for gtk port ?
> 
> GTK port uses threaded compositor in optional manner. And again, we need to add TiledBackingStore in this source list.

Weird, I looks missing to add TiledBackingStore.cpp in PlatformGtk.cmake.

I will rebase.

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:7757
>> +      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">true</ExcludedFromBuild>
> 
> I don't know .vcxproj file well, but why do we need add these options ?

ExcludeFromBuild is not to build this file.
Without this patch, windows port has compiled TiledBackingStore.cpp which is guarded as TILED_BACKING_STORE so generates (almost) nothing.

Anyway, it might not be related to this commit in more thinking.
I will take care of this in different bug.

[Ryuan Choi]

Created attachment 251900 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 251900 [details]
Patch

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

LGTM. However it would be good if yoon take a final look before landing.

> Source/WebCore/ChangeLog:8
> +        TiledBackingStore is only used with Coordinated Graphics since Qt and WebKit1/Efl was dropped.

There are trivial typo.

"TiledBackingStore has only been used by Coordinated Graphics since Qt and WebKit1/Efl ports were dropped. So, this patch replaces USE(TILED_BACKING_STORE) with USE(COORDINATED_GRAPHICS) to merge the features."

[Ryuan Choi]

Created attachment 251923 [details]
fixed_typo

[Gwang Yoon Hwang]

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

It looks good for me, but it would be good to reduce change log before adding it to the commit queue.

> Source/WebCore/ChangeLog:43
> +        (WebCore::TiledBackingStore::TiledBackingStore):

You can remove methods from change log if you just moved a file to reduce redundant lines in the changeling.

[Ryuan Choi]

Created attachment 251924 [details]
patch

[Ryuan Choi]

(In reply to comment #15)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251900&action=review
> 
> It looks good for me, but it would be good to reduce change log before
> adding it to the commit queue.
> 
> > Source/WebCore/ChangeLog:43
> > +        (WebCore::TiledBackingStore::TiledBackingStore):
> 
> You can remove methods from change log if you just moved a file to reduce
> redundant lines in the changeling.

OK, done

[Ryuan Choi]

Comment on attachment 251924 [details]
patch

Clearing flags on attachment: 251924

Committed r183529: <http://trac.webkit.org/changeset/183529>

[Ryuan Choi]

All reviewed patches have been landed.  Closing bug.