Coordinated Graphics : Move CoordinatedGraphics related files to WebCore

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jae Hyun Park]

Coordinated Graphics code should reside in WebCore in order for Threaded Coordinated Graphics code to reuse it.
Except for IPC related Code, all of the Coordinated Graphics code need to be moved from WebKit2 to WebCore.

[Jae Hyun Park]

Created attachment 185162 [details]
Patch

[WebKit Review Bot]

Attachment 185162 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/Target.pri', u'Source/WebCore/platform/graphics/texmap/coordinated/AreaAllocator.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/AreaAllocator.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBackingStore.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBackingStore.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedLayerInfo.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedTile.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedTile.h', u'Source/WebCore/platform/graphics/texmap/coordinated/LayerTreeRenderer.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/LayerTreeRenderer.h', u'Source/WebCore/platform/graphics/texmap/coordinated/SurfaceUpdateInfo.h', u'Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.h', u'Source/WebCore/platform/graphics/texmap/coordinated/WebCustomFilterOperation.h', u'Source/WebCore/platform/graphics/texmap/coordinated/WebCustomFilterProgram.h', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp', u'Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.h', u'Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedLayerInfo.h', u'Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h', u'Source/WebKit2/Shared/CoordinatedGraphics/SurfaceUpdateInfo.h', u'Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp', u'Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.h', u'Source/WebKit2/Shared/CoordinatedGraphics/WebCustomFilterOperation.h', u'Source/WebKit2/Shared/CoordinatedGraphics/WebCustomFilterProgram.h', u'Source/WebKit2/Target.pri', u'Source/WebKit2/UIProcess/API/efl/EwkView.cpp', u'Source/WebKit2/UIProcess/API/efl/EwkView.h', u'Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp', u'Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp', u'Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h', u'Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp', u'Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.h', u'Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp', u'Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h', u'Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.messages.in', u'Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp', u'Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h', u'Source/WebKit2/UIProcess/efl/PageClientBase.cpp', u'Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp', u'Source/WebKit2/UIProcess/qt/QtWebPageSGNode.cpp', u'Source/WebKit2/UIProcess/qt/QtWebPageSGNode.h', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/AreaAllocator.cpp', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/AreaAllocator.h', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.h', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.cpp', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedTile.h', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp', u'Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.h']" exit_code: 1
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:411:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Comment on attachment 185162 [details]
Patch

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

Apart from some rename suggestions, this seems to be going in the right direction for me.
The next step would be to remove most of the stuff from CoordinatedGraphicsArgumentCoders.

Anders, Simon, your feedback is of course welcome.

> Source/WebCore/ChangeLog:24
> +        3. WebCustomFilterProgram and WebCustomFilterOperation is moved to WebCore in

They should be renamed CoordinatedCustom*, the Web prefix is only for WebKit/WebKit2.

> Source/WebCore/ChangeLog:239
> +        (WebCore::LayerTreeRenderer::dispatchOnMainThread):

I think LayerTreeRenderer should be renamed TextureMapperScene.

[Noam Rosenthal]

Comment on attachment 185162 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        1. Removing CoordinatedLayerTreeHost dependency from LayerTreeRenderer. This
> +        patch introduces LayerTreeRendererClient, which is implemented in
> +        CoordinatedLayerTreeHost. LayerTreeRenderer uses this client, instead of using
> +        CoordinatedLayerTreeHost directly.

Maybe we should do this first, with a separate patch.

[Jae Hyun Park]

(In reply to comment #3)
> (From update of attachment 185162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185162&action=review
> 
> Apart from some rename suggestions, this seems to be going in the right direction for me.
> The next step would be to remove most of the stuff from CoordinatedGraphicsArgumentCoders.
> 
> Anders, Simon, your feedback is of course welcome.
> 
> > Source/WebCore/ChangeLog:24
> > +        3. WebCustomFilterProgram and WebCustomFilterOperation is moved to WebCore in
> 
> They should be renamed CoordinatedCustom*, the Web prefix is only for WebKit/WebKit2.
> 
> > Source/WebCore/ChangeLog:239
> > +        (WebCore::LayerTreeRenderer::dispatchOnMainThread):
> 
> I think LayerTreeRenderer should be renamed TextureMapperScene.

(In reply to comment #4)
> (From update of attachment 185162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185162&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        1. Removing CoordinatedLayerTreeHost dependency from LayerTreeRenderer. This
> > +        patch introduces LayerTreeRendererClient, which is implemented in
> > +        CoordinatedLayerTreeHost. LayerTreeRenderer uses this client, instead of using
> > +        CoordinatedLayerTreeHost directly.
> 
> Maybe we should do this first, with a separate patch.

Thanks for the review! 
In the separate patch that introduces LayerTreeRendererClient, should I just rename LayerTreeRenderer to TextureMapperScene?

[Noam Rosenthal]

> In the separate patch that introduces LayerTreeRendererClient, should I just rename LayerTreeRenderer to TextureMapperScene?
Let's do the rename as part of moving to WebCore. For now we should introduce the client without renames.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 185162 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        2. Create CoordinatedSurface using function pointer. Since CoordinatedSurface
> +        reside in WebCore now, it doesn't know WebCoordinatedSurface. So we pass
> +        CoordinatedSurface::Factory instead of using CoordinatedSurface::create
> +        method.

I think this could be done in a separate patch as well. This patch ideally will only move the files and fix whatever needs to be fixed to get it compiling again.

[Jae Hyun Park]

(In reply to comment #6)
> > In the separate patch that introduces LayerTreeRendererClient, should I just rename LayerTreeRenderer to TextureMapperScene?
> Let's do the rename as part of moving to WebCore. For now we should introduce the client without renames.

Ok. Created a patch in https://bugs.webkit.org/show_bug.cgi?id=108164

[Jae Hyun Park]

(In reply to comment #7)
> (From update of attachment 185162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185162&action=review
> 
> > Source/WebCore/ChangeLog:22
> > +        2. Create CoordinatedSurface using function pointer. Since CoordinatedSurface
> > +        reside in WebCore now, it doesn't know WebCoordinatedSurface. So we pass
> > +        CoordinatedSurface::Factory instead of using CoordinatedSurface::create
> > +        method.
> 
> I think this could be done in a separate patch as well. This patch ideally will only move the files and fix whatever needs to be fixed to get it compiling again.

Thanks for the review! 

I'll create another patch for using function pointer to create CoordinatedSurface.

[Jae Hyun Park]

Created attachment 185693 [details]
Patch

[Jae Hyun Park]

Created attachment 185714 [details]
rebased

[Jae Hyun Park]

Created attachment 185722 [details]
Patch

[Noam Rosenthal]

Comment on attachment 185722 [details]
Patch

Looks good to me! Let's see if the WK2 owners are ok with this.

[Anders Carlsson]

WK2 parts look good to me. You should remove all the function declarations from the file renames in the ChangeLog though.

[Jae Hyun Park]

Created attachment 185922 [details]
Patch

[Noam Rosenthal]

Comment on attachment 185922 [details]
Patch

Let's go!

[WebKit Review Bot]

Comment on attachment 185922 [details]
Patch

Rejecting attachment 185922 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 185922, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
  return self.open(self.click(*args, **kwds))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://queues.webkit.org/results/16302414

[Noam Rosenthal]

Comment on attachment 185922 [details]
Patch

Trying again with cq. If this fails I'll lent manually.

[WebKit Review Bot]

Comment on attachment 185922 [details]
Patch

Clearing flags on attachment: 185922

Committed r141543: <http://trac.webkit.org/changeset/141543>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Brianceau]

(In reply to comment #15)
> Created an attachment (id=185922) [details]
> Patch
Hello,

Unfortunately, this patch breaks our buildbot: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/19109

I'll take a look this week if I find time to do so.

[Noam Rosenthal]

(In reply to comment #21)
> (In reply to comment #15)
> > Created an attachment (id=185922) [details] [details]
> > Patch
> Hello,
> 
> Unfortunately, this patch breaks our buildbot: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/19109
> 
> I'll take a look this week if I find time to do so.

Seems like some GL includes are not protected with the right #ifdefs.

[Julien Brianceau]

(In reply to comment #22)
> 
> Seems like some GL includes are not protected with the right #ifdefs.

I think the issue is in Target.pri file. Shouldn't new files be into conditionnal use?(3D_GRAPHICS) block instead ?

[Julien Brianceau]

I've created a new bug for this: https://bugs.webkit.org/show_bug.cgi?id=108862

[Jae Hyun Park]

(In reply to comment #24)
> I've created a new bug for this: https://bugs.webkit.org/show_bug.cgi?id=108862

I'm sorry for breaking the build, and thanks for providing a fix!