Bug 108149 - Coordinated Graphics : Move CoordinatedGraphics related files to WebCore
Summary: Coordinated Graphics : Move CoordinatedGraphics related files to WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 108164 108259
Blocks: 102994
  Show dependency treegraph
 
Reported: 2013-01-28 22:26 PST by Jae Hyun Park
Modified: 2013-02-04 15:26 PST (History)
17 users (show)

See Also:


Attachments
Patch (351.34 KB, patch)
2013-01-28 22:44 PST, Jae Hyun Park
no flags Details | Formatted Diff | Diff
Patch (378.41 KB, patch)
2013-01-30 23:58 PST, Jae Hyun Park
no flags Details | Formatted Diff | Diff
rebased (378.41 KB, patch)
2013-01-31 01:32 PST, Jae Hyun Park
no flags Details | Formatted Diff | Diff
Patch (379.18 KB, patch)
2013-01-31 02:14 PST, Jae Hyun Park
no flags Details | Formatted Diff | Diff
Patch (359.18 KB, patch)
2013-01-31 20:37 PST, Jae Hyun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jae Hyun Park 2013-01-28 22:26:00 PST
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.
Comment 1 Jae Hyun Park 2013-01-28 22:44:43 PST
Created attachment 185162 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-28 22:49:56 PST
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.
Comment 3 Noam Rosenthal 2013-01-28 23:30:36 PST
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.
Comment 4 Noam Rosenthal 2013-01-28 23:32:54 PST
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.
Comment 5 Jae Hyun Park 2013-01-28 23:37:20 PST
(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?
Comment 6 Noam Rosenthal 2013-01-29 06:31:42 PST
> 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.
Comment 7 Caio Marcelo de Oliveira Filho 2013-01-29 12:09:13 PST
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.
Comment 8 Jae Hyun Park 2013-01-29 15:16:33 PST
(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
Comment 9 Jae Hyun Park 2013-01-29 15:18:10 PST
(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.
Comment 10 Jae Hyun Park 2013-01-30 23:58:20 PST
Created attachment 185693 [details]
Patch
Comment 11 Jae Hyun Park 2013-01-31 01:32:34 PST
Created attachment 185714 [details]
rebased
Comment 12 Jae Hyun Park 2013-01-31 02:14:52 PST
Created attachment 185722 [details]
Patch
Comment 13 Noam Rosenthal 2013-01-31 05:15:34 PST
Comment on attachment 185722 [details]
Patch

Looks good to me! Let's see if the WK2 owners are ok with this.
Comment 14 Anders Carlsson 2013-01-31 15:24:37 PST
WK2 parts look good to me. You should remove all the function declarations from the file renames in the ChangeLog though.
Comment 15 Jae Hyun Park 2013-01-31 20:37:21 PST
Created attachment 185922 [details]
Patch
Comment 16 Noam Rosenthal 2013-01-31 22:38:45 PST
Comment on attachment 185922 [details]
Patch

Let's go!
Comment 17 WebKit Review Bot 2013-01-31 22:58:43 PST
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
Comment 18 Noam Rosenthal 2013-01-31 23:14:34 PST
Comment on attachment 185922 [details]
Patch

Trying again with cq. If this fails I'll lent manually.
Comment 19 WebKit Review Bot 2013-01-31 23:34:02 PST
Comment on attachment 185922 [details]
Patch

Clearing flags on attachment: 185922

Committed r141543: <http://trac.webkit.org/changeset/141543>
Comment 20 WebKit Review Bot 2013-01-31 23:34:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Julien Brianceau 2013-02-04 09:20:59 PST
(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.
Comment 22 Noam Rosenthal 2013-02-04 09:57:34 PST
(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.
Comment 23 Julien Brianceau 2013-02-04 13:54:34 PST
(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 ?
Comment 24 Julien Brianceau 2013-02-04 14:44:17 PST
I've created a new bug for this: https://bugs.webkit.org/show_bug.cgi?id=108862
Comment 25 Jae Hyun Park 2013-02-04 15:26:56 PST
(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!