Bug 90506 – Shared code that is guarded with ENABLE(WEBGL) should be guarded with USE()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Noam Rosenthal]

Some code like GraphicsContext3D is used outside of WebGL, e.g. for accelerated canvas and TextureMapper.
It should be guarded with a USE flag, maybe USE(GRAPHICS_CONTEXT_3D), and be turned on when any of the features requiring it is enabled.

[Kenneth Russell]

Sounds reasonable.

[Noam Rosenthal]

Created attachment 150849 [details]
Patch

[Noam Rosenthal]

I'm using USE(3D_GRAPHICS) as a guard, but I don't mind using something else like HAVE(OPENGL) or USE(GRAPHICS_CONTEXT_3D), whatever can get a concensus and is not tied to ENABLE(WEBGL).

[Tor Arne Vestbø]

Comment on attachment 150849 [details]
Patch

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

Nice!

> Source/WebCore/WebCore.pri:211
> +haveQt(5)|contains(QT_CONFIG, opengl) {

This check should go in features.prf

> Source/WebCore/WebCore.pri:213
> +    CONFIG += graphics3d
> +    DEFINES += WTF_USE_3D_GRAPHICS=1

And instead of going through an intermediate CONFIG variable, define only DEFINES += WTF_USE_3D_GRAPHICS=1, and check for that wherever you have contains(CONFIG, graphics3d). I know, it's a bit cumbersome, but it's the pattern we use everywhere else for features.

> Source/WebCore/WebCore.pri:214
> +    contains(QT_CONFIG, opengles2): LIBS += -lEGL

This part will then still live inside contains(DEFINES, WTF_USE_3D_GRAPHICS=1) here.

[Noam Rosenthal]

Created attachment 150852 [details]
Patch

[Noam Rosenthal]

(In reply to comment #4)
> (From update of attachment 150849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150849&action=review

>  instead of going through an intermediate CONFIG variable, define only DEFINES += WTF_USE_3D_GRAPHICS=1, and check for that wherever you have contains(CONFIG, graphics3d). I know, it's a bit cumbersome, but it's the pattern we use everywhere else for features.
> 
> > Source/WebCore/WebCore.pri:214
> > +    contains(QT_CONFIG, opengles2): LIBS += -lEGL
> 
> This part will then still live inside contains(DEFINES, WTF_USE_3D_GRAPHICS=1) here.

Got it :) All of that should be fixed in the new patch.

[Noam Rosenthal]

Created attachment 150856 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 150856 [details]
Patch

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

looks good from a Qt pov

> Source/WebCore/Target.pri:3943
> +

Extra line

> Source/WebCore/Target.pri:4101
> +

Whitespace-hunk

[Noam Rosenthal]

Created attachment 151006 [details]
Patch

[Martin Robinson]

Comment on attachment 151006 [details]
Patch

This looks good to me, but since you are turning on WTF_USE_3D_GRAPHICS for all ports, perhaps it would be good to update all ENABLE(WEBGL)to read USE(3D_GRAPHICS) in all GC3D files? For instance, skia/GraphicsContext3DSkia.cpp is unchanged.

[Noam Rosenthal]

Created attachment 151159 [details]
Patch

[Noam Rosenthal]

(In reply to comment #10)
> (From update of attachment 151006 [details])
> This looks good to me, but since you are turning on WTF_USE_3D_GRAPHICS for all ports, perhaps it would be good to update all ENABLE(WEBGL)to read USE(3D_GRAPHICS) in all GC3D files? For instance, skia/GraphicsContext3DSkia.cpp is unchanged.

OK, I uploaded another patch with the proposed approach. Not sure which one is better; I agree that all ports should move to using USE(3D_GRAPHICS) for GC3D, DrawingBuffer and the CSS shaders stuff, but maybe it's better to do that gradually whenever the port sees fit rather than try to cram it all in this patch?

Either way, both patches are up to review and I'm OK with either way.

[Noam Rosenthal]

Created attachment 151263 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151263 [details]
Patch for landing

Rejecting attachment 151263 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ext3DSkia.cpp
patching file Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h
patching file Source/WebCore/platform/qt/QWebPageClient.h
patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp
patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.h
patching file Tools/ChangeLog
patching file Tools/qmake/mkspecs/features/features.prf

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

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

[Noam Rosenthal]

Created attachment 151303 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151303 [details]
Patch for landing

Rejecting attachment 151303 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 11988 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 11988.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

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

[Noam Rosenthal]

Created attachment 151338 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151338 [details]
Patch for landing

Rejecting attachment 151338 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e/platform/graphics/texmap/GraphicsLayerTextureMapper.h
patching file Source/WebCore/platform/qt/QWebPageClient.h
patching file Source/WebKit/qt/ChangeLog
patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp
patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.h
patching file Tools/ChangeLog
patching file Tools/qmake/mkspecs/features/features.prf

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue/

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

[Noam Rosenthal]

Created attachment 151347 [details]
Patch for landing

[Noam Rosenthal]

Created attachment 151350 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151350 [details]
Patch for landing

Rejecting attachment 151350 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ext3DSkia.cpp
patching file Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h
patching file Source/WebCore/platform/qt/QWebPageClient.h
patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp
patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.h
patching file Tools/ChangeLog
patching file Tools/qmake/mkspecs/features/features.prf

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue/

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

[Noam Rosenthal]

Committed manually, http://trac.webkit.org/changeset/122175

[Noam Rosenthal]

Sorry for the faulty commit message, an oversight.

[WebKit Review Bot]

Re-opened since this is blocked by 90847

[Csaba Osztrogonác]

(In reply to comment #22)
> Committed manually, http://trac.webkit.org/changeset/122175

It broke the Qt win build: https://bugs.webkit.org/show_bug.cgi?id=90850

[Csaba Osztrogonác]

(In reply to comment #22)
> Committed manually, http://trac.webkit.org/changeset/122175

GTK buidlfix: https://trac.webkit.org/changeset/122202
Chromium buildfix: https://trac.webkit.org/changeset/122193