Bug 90567

Summary: Make GC3D and E3D more maintainable for GLES platforms
Product: WebKit Reporter: Joshua Netterfield <jnetterfield>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, efidler, fpizlo, gustavo, kbr, ossy, philn, rakuco, rniwa, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90943    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Joshua Netterfield 2012-07-04 09:42:35 PDT 
The current structure of GraphicsContext3DOpenGL* and Extensions3DOpenGL makes improvements to GLES-specific code difficult.

Extensions3DOpenGL should be split into Extensions3DOpenGLCommon, Extensions3DOpenGLES, and Extensions3DOpenGL mirroring the structure of GC3D.

GraphicsContext3DOpenGLCommon should contain as much code as possible while limiting the amount of "#if"s
Comment 1 Joshua Netterfield 2012-07-04 11:23:41 PDT 
Created attachment 150826 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-04 11:49:07 PDT 
Comment on attachment 150826 [details]
Patch

Attachment 150826 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13131874
Comment 3 Build Bot 2012-07-04 12:10:48 PDT 
Comment on attachment 150826 [details]
Patch

Attachment 150826 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13136799
Comment 4 Joshua Netterfield 2012-07-04 14:45:07 PDT 
Created attachment 150844 [details]
Patch
Comment 5 WebKit Review Bot 2012-07-04 14:48:58 PDT 
Attachment 150844 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Joshua Netterfield 2012-07-04 14:59:26 PDT 
Created attachment 150848 [details]
Patch
Comment 7 Build Bot 2012-07-04 15:11:10 PDT 
Comment on attachment 150848 [details]
Patch

Attachment 150848 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13134880
Comment 8 Joshua Netterfield 2012-07-05 10:33:58 PDT 
Created attachment 150956 [details]
Patch
Comment 9 Build Bot 2012-07-05 10:45:45 PDT 
Comment on attachment 150956 [details]
Patch

Attachment 150956 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13154067
Comment 10 Joshua Netterfield 2012-07-05 11:21:00 PDT 
Created attachment 150965 [details]
Patch
Comment 11 Joshua Netterfield 2012-07-05 11:40:03 PDT 
Created attachment 150966 [details]
Patch
Comment 12 Joshua Netterfield 2012-07-05 11:58:19 PDT 
Created attachment 150969 [details]
Patch
Comment 13 Build Bot 2012-07-05 12:33:18 PDT 
Comment on attachment 150969 [details]
Patch

Attachment 150969 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13155041
Comment 14 Joshua Netterfield 2012-07-05 12:52:39 PDT 
Created attachment 150974 [details]
Patch
Comment 15 Joshua Netterfield 2012-07-05 13:19:50 PDT 
Created attachment 150976 [details]
Patch
Comment 16 Joshua Netterfield 2012-07-05 13:30:02 PDT 
Created attachment 150977 [details]
Patch
Comment 17 Rob Buis 2012-07-05 14:04:41 PDT 
Comment on attachment 150977 [details]
Patch

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

Looks good in general, can be cleaned up a bit more.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Should be RIM copyright I think.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:185
> +

No need for empty line.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:267
> +

Ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:277
> +

Ditto.
Comment 18 Joshua Netterfield 2012-07-05 14:06:34 PDT 
Created attachment 150982 [details]
Patch
Comment 19 Kenneth Russell 2012-07-06 11:59:48 PDT 
Comment on attachment 150982 [details]
Patch

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

This patch looks fine to me overall; I'll leave it to Rob or someone who works on one of the other affected ports to r+ it. One comment.

> Source/WebCore/platform/graphics/OpenGLESShims.h:2
> + * Copyright (C) 2012 Research In Motion Limited. All rights reserved.

This file should use the two-clause BSD license in Source/WebKit/LICENSE.
Comment 20 Joshua Netterfield 2012-07-09 08:20:50 PDT 
Created attachment 151250 [details]
Patch
Comment 21 Rob Buis 2012-07-09 08:42:48 PDT 
Comment on attachment 151250 [details]
Patch

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

Looks good, but needs the ChangeLog fix.

> Source/WebCore/ChangeLog:868
> +2012-07-04  Joshua Netterfield  <jnetterfield@rim.com>

Update date and make sure this is top of the ChangeLog.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLES.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Could this use RIM copyright?
Comment 22 Joshua Netterfield 2012-07-09 08:46:27 PDT 
Created attachment 151255 [details]
Patch
Comment 23 Rob Buis 2012-07-09 09:51:18 PDT 
Comment on attachment 151255 [details]
Patch

LGTM.
Comment 24 WebKit Review Bot 2012-07-09 11:00:30 PDT 
Comment on attachment 151255 [details]
Patch

Clearing flags on attachment: 151255

Committed r122116: <http://trac.webkit.org/changeset/122116>
Comment 25 WebKit Review Bot 2012-07-09 11:00:42 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 26 Ryosuke Niwa 2012-07-09 11:32:25 PDT 
Mac build fix landed in http://trac.webkit.org/changeset/122119.
Comment 27 Filip Pizlo 2012-07-09 13:34:43 PDT 
This broke five WebGL tests on Mac:

http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r122124%20(759)/results.html
Comment 28 Filip Pizlo 2012-07-09 13:54:16 PDT 
Reopening because it broke tests.
Comment 29 Filip Pizlo 2012-07-09 14:15:36 PDT 
Rolled out in http://trac.webkit.org/changeset/122156
Comment 30 Joshua Netterfield 2012-07-10 10:51:15 PDT 
Created attachment 151485 [details]
Patch
Comment 31 Rob Buis 2012-07-10 11:49:25 PDT 
Comment on attachment 151485 [details]
Patch

Looks great, and Joshua verified on OS X SL that the fast/canvas/webgl tests pass with this patch.
Comment 32 WebKit Review Bot 2012-07-10 12:52:54 PDT 
Comment on attachment 151485 [details]
Patch

Clearing flags on attachment: 151485

Committed r122250: <http://trac.webkit.org/changeset/122250>
Comment 33 WebKit Review Bot 2012-07-10 12:53:01 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 34 Csaba Osztrogonác 2012-07-10 23:29:43 PDT 
(In reply to comment #32)
> (From update of attachment 151485 [details])
> Clearing flags on attachment: 151485
> 
> Committed r122250: <http://trac.webkit.org/changeset/122250>

It broke USE(3D_GRAPHICS)=1 and ENABLE(WEBGL)=0 builds - https://bugs.webkit.org/show_bug.cgi?id=90943

Could you check and fix it please?