Now, TILED_BACKING_STORE is only used with COORDINATED_GRAPHICS. So, I think that we'd better to merge them.
Created attachment 249363 [details] Patch
yoon, what do you think about this?
(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.
(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)
(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.
(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.
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 ?
(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.
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 ?
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.
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.
Created attachment 251900 [details] Patch
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."
Created attachment 251923 [details] fixed_typo
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.
Created attachment 251924 [details] patch
(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
Comment on attachment 251924 [details] patch Clearing flags on attachment: 251924 Committed r183529: <http://trac.webkit.org/changeset/183529>
All reviewed patches have been landed. Closing bug.