Bug 86257 - [chromium] Move implementation of WebCore::GraphicsContext3D and related from WebKit/chromium/src to WebCore/platform/chromium/support
Summary: [chromium] Move implementation of WebCore::GraphicsContext3D and related from...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 86524
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 15:07 PDT by James Robinson
Modified: 2012-06-06 17:38 PDT (History)
2 users (show)

See Also:


Attachments
Patch (173.76 KB, patch)
2012-05-11 15:09 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (114.34 KB, patch)
2012-06-06 17:05 PDT, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-05-11 15:07:54 PDT
[chromium] Move implementation of WebCore::GraphicsContext3D and related from WebKit/chromium/src to WebCore/platform/chromium/support
Comment 1 James Robinson 2012-05-11 15:09:36 PDT
Created attachment 141504 [details]
Patch
Comment 2 James Robinson 2012-05-11 15:13:12 PDT
Diffs that aren't file moves or ChangeLogs:

diff --git a/Source/WebCore/WebCore.gypi b/Source/WebCore/WebCore.gypi
index e7746e6..6c06e2a 100644
--- a/Source/WebCore/WebCore.gypi
+++ b/Source/WebCore/WebCore.gypi
@@ -8275,6 +8275,9 @@
             '<(PRODUCT_DIR)/DerivedSources/WebCore/XPathGrammar.h',
         ],
         'webcore_platform_support_files': [
+            'platform/chromium/support/Extensions3DChromium.cpp',
+            'platform/chromium/support/GraphicsContext3DChromium.cpp',
+            'platform/chromium/support/GraphicsContext3DPrivate.h',
             'platform/chromium/support/WebAudioBus.cpp',
             'platform/chromium/support/WebData.cpp',
             'platform/chromium/support/WebHTTPBody.cpp',
diff --git a/Source/WebKit/chromium/WebKit.gyp b/Source/WebKit/chromium/WebKit.gyp
index a775f54..801a887 100644
--- a/Source/WebKit/chromium/WebKit.gyp
+++ b/Source/WebKit/chromium/WebKit.gyp
@@ -401,7 +401,6 @@
                 'src/EditorClientImpl.h',
                 'src/EventListenerWrapper.cpp',
                 'src/EventListenerWrapper.h',
-                'src/Extensions3DChromium.cpp',
                 'src/ExternalPopupMenu.cpp',
                 'src/ExternalPopupMenu.h',
                 'src/FrameLoaderClientImpl.cpp',
@@ -409,8 +408,6 @@
                 'src/FrameNetworkingContextImpl.h',
                 'src/GeolocationClientProxy.cpp',
                 'src/GeolocationClientProxy.h',
-                'src/GraphicsContext3DChromium.cpp',
-                'src/GraphicsContext3DPrivate.h',
                 'src/gtk/WebInputEventFactory.cpp',
                 'src/IDBCallbacksProxy.cpp',
                 'src/IDBCallbacksProxy.h',

diff --git a/Source/WebCore/platform/chromium/support/Extensions3DChromium.cpp b/Source/WebCore/platform/chromium/support/Extensions3DChromium.cpp
index 84ad0f7..48b679d 100644
--- a/Source/WebCore/platform/chromium/support/Extensions3DChromium.cpp
+++ b/Source/WebCore/platform/chromium/support/Extensions3DChromium.cpp
@@ -25,8 +25,6 @@
 
 #include "config.h"
 
-#if ENABLE(WEBGL)
-
 #include "Extensions3DChromium.h"
 
 #include "GraphicsContext3D.h"
@@ -206,5 +204,3 @@ void Extensions3DChromium::getQueryObjectuivEXT(Platform3DObject query, GC3Denum
 }
 
 } // namespace WebCore
-
-#endif // ENABLE(WEBGL)
Comment 3 Adam Barth 2012-05-11 15:14:27 PDT
> -#if ENABLE(WEBGL)

Why remove this enabled?
Comment 4 James Robinson 2012-05-11 15:15:08 PDT
(In reply to comment #3)
> > -#if ENABLE(WEBGL)
> 
> Why remove this enabled?

It's silly old.  We use Extension3DChromium in all sorts of code that isn't related to WebGL these days, and we don't have any other compile guard for 3d functionality in general in Chromium (it's just always on).
Comment 5 James Robinson 2012-05-11 15:18:49 PDT
Committed r116811: <http://trac.webkit.org/changeset/116811>
Comment 6 James Robinson 2012-05-15 11:49:24 PDT
Actually, this was stupid - GraphicsContext3DPrivate has references to WebViewImpl and WebViewClient, which are things you clearly should not be allowed to talk about in WebCore/platform.  I'm going to revert and then try again since this is clearly confusing other folks patching this code.
Comment 7 James Robinson 2012-05-15 11:53:30 PDT
Committed r117110: <http://trac.webkit.org/changeset/117110>
Comment 8 James Robinson 2012-05-15 13:37:58 PDT
Looks like I should first move createOffscreenGraphicsContext3D() out of WebKitPlatformSupport into Platform and remove the unused WebViewClient.h / WebViewImpl.h #includes from GraphicsContext3DChromium.cpp, then do this move again.  Sorry about the churn.
Comment 9 Adam Barth 2012-05-15 14:04:59 PDT
Comment on attachment 141504 [details]
Patch

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

> Source/WebCore/platform/chromium/support/GraphicsContext3DChromium.cpp:50
> +#include "WebViewClient.h"
> +#include "WebViewImpl.h"

Sorry, I should have looked at these include paths more closely.  :(
Comment 10 James Robinson 2012-05-15 15:04:21 PDT
GraphicsContext3DChromium.cpp also references many concepts that exist in WebCore but are not in WebCore/platform - for example, CanvasRenderingContext.  It looks like most of this can be mechanically translated to using WebCore/platform concepts like ImageBuffer, but this probably should also be done first.
Comment 11 James Robinson 2012-06-06 17:05:57 PDT
Created attachment 146152 [details]
Patch
Comment 12 James Robinson 2012-06-06 17:07:25 PDT
Here we go again!  This is just a straight move, no changes to code:


# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	renamed:    Source/WebKit/chromium/src/Extensions3DChromium.cpp -> Source/WebCore/platform/chromium/support/Extensions3DChromium.cpp
#	renamed:    Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp -> Source/WebCore/platform/chromium/support/GraphicsContext3DChromium.cpp
#	renamed:    Source/WebKit/chromium/src/GraphicsContext3DPrivate.cpp -> Source/WebCore/platform/chromium/support/GraphicsContext3DPrivate.cpp
#	renamed:    Source/WebKit/chromium/src/GraphicsContext3DPrivate.h -> Source/WebCore/platform/chromium/support/GraphicsContext3DPrivate.h
#
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   Source/WebCore/WebCore.gypi
#	modified:   Source/WebKit/chromium/WebKit.gyp
#


I've cleaned up the non - WebCore/platform/ dependencies from GraphicsContext3DChromium with the single exception of ImageData, which is in WebCore/html/.  This type is referred to in the cross-platform GraphicsContext3D interface, so while I'm propagating the bad layering I'm not making it any worse.  It doesn't seem trivial to fix.
Comment 13 Kenneth Russell 2012-06-06 17:16:38 PDT
Comment on attachment 146152 [details]
Patch

Sounds fine as long as it compiles and this is the desired organization of the code. r=me
Comment 14 James Robinson 2012-06-06 17:38:50 PDT
Committed r119651: <http://trac.webkit.org/changeset/119651>