Bug 90506

Summary: Shared code that is guarded with ENABLE(WEBGL) should be guarded with USE()
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: WebGLAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, jesus, kbr, mrobinson, ossy, simon.fraser, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90847, 90850    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing
webkit.review.bot: commit-queue-
Patch for landing
none
Patch for landing
none
Patch for landing webkit.review.bot: commit-queue-

Noam Rosenthal
Reported 2012-07-03 16:31:55 PDT
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.
Attachments
Patch (29.14 KB, patch)
2012-07-04 15:51 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (31.03 KB, patch)
2012-07-04 17:13 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (31.03 KB, patch)
2012-07-04 18:29 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (31.07 KB, patch)
2012-07-05 17:47 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (59.62 KB, patch)
2012-07-07 23:38 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch for landing (59.49 KB, patch)
2012-07-09 09:54 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch for landing (61.00 KB, patch)
2012-07-09 12:51 PDT, Noam Rosenthal 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (56.67 KB, patch)
2012-07-09 15:55 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch for landing (60.01 KB, patch)
2012-07-09 16:21 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch for landing (60.01 KB, patch)
2012-07-09 16:30 PDT, Noam Rosenthal 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.
Kenneth Russell
Comment 1 2012-07-04 12:56:58 PDT
Sounds reasonable.
Noam Rosenthal
Comment 2 2012-07-04 15:51:31 PDT
Created attachment 150849 [details] Patch
Noam Rosenthal
Comment 3 2012-07-04 15:54:10 PDT
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 4 2012-07-04 16:35:08 PDT
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
Comment 5 2012-07-04 17:13:49 PDT
Created attachment 150852 [details] Patch
Noam Rosenthal
Comment 6 2012-07-04 17:14:36 PDT
(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
Comment 7 2012-07-04 18:29:01 PDT
Created attachment 150856 [details] Patch
Tor Arne Vestbø
Comment 8 2012-07-05 04:26:24 PDT
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
Comment 9 2012-07-05 17:47:49 PDT
Created attachment 151006 [details] Patch
Martin Robinson
Comment 10 2012-07-07 10:59:20 PDT
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
Comment 11 2012-07-07 23:38:28 PDT
Created attachment 151159 [details] Patch
Noam Rosenthal
Comment 12 2012-07-07 23:40:53 PDT
(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
Comment 13 2012-07-09 09:54:46 PDT
Created attachment 151263 [details] Patch for landing
WebKit Review Bot
Comment 14 2012-07-09 11:21:10 PDT
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
Comment 15 2012-07-09 12:51:26 PDT
Created attachment 151303 [details] Patch for landing
WebKit Review Bot
Comment 16 2012-07-09 14:51:22 PDT
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
Comment 17 2012-07-09 15:55:14 PDT
Created attachment 151338 [details] Patch for landing
WebKit Review Bot
Comment 18 2012-07-09 16:07:30 PDT
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
Comment 19 2012-07-09 16:21:46 PDT
Created attachment 151347 [details] Patch for landing
Noam Rosenthal
Comment 20 2012-07-09 16:30:21 PDT
Created attachment 151350 [details] Patch for landing
WebKit Review Bot
Comment 21 2012-07-09 17:31:03 PDT
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
Comment 22 2012-07-09 18:07:57 PDT
Committed manually, http://trac.webkit.org/changeset/122175
Noam Rosenthal
Comment 23 2012-07-09 18:17:40 PDT
Sorry for the faulty commit message, an oversight.
WebKit Review Bot
Comment 24 2012-07-09 21:43:08 PDT
Re-opened since this is blocked by 90847
Csaba Osztrogonác
Comment 25 2012-07-09 23:58:27 PDT
(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
Comment 26 2012-07-10 02:33:56 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.