Bug 86257 – [chromium] Move implementation of WebCore::GraphicsContext3D and related from WebKit/chromium/src to WebCore/platform/chromium/support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Move implementation of WebCore::GraphicsContext3D and related from WebKit/chromium/src to WebCore/platform/chromium/support

[James Robinson]

Created attachment 141504 [details]
Patch

[James Robinson]

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)

[Adam Barth]

> -#if ENABLE(WEBGL)

Why remove this enabled?

[James Robinson]

(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).

[James Robinson]

Committed r116811: <http://trac.webkit.org/changeset/116811>

[James Robinson]

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.

[James Robinson]

Committed r117110: <http://trac.webkit.org/changeset/117110>

[James Robinson]

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.

[Adam Barth]

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.  :(

[James Robinson]

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.

[James Robinson]

Created attachment 146152 [details]
Patch

[James Robinson]

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.

[Kenneth Russell]

Comment on attachment 146152 [details]
Patch

Sounds fine as long as it compiles and this is the desired organization of the code. r=me

[James Robinson]

Committed r119651: <http://trac.webkit.org/changeset/119651>