Bug 90567 - Make GC3D and E3D more maintainable for GLES platforms
Summary: Make GC3D and E3D more maintainable for GLES platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 90943
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-04 09:42 PDT by Joshua Netterfield
Modified: 2012-07-10 23:29 PDT (History)
11 users (show)

See Also:


Attachments
Patch (57.20 KB, patch)
2012-07-04 11:23 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (60.32 KB, patch)
2012-07-04 14:45 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (59.79 KB, patch)
2012-07-04 14:59 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (62.15 KB, patch)
2012-07-05 10:33 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.03 KB, patch)
2012-07-05 11:21 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.00 KB, patch)
2012-07-05 11:40 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.00 KB, patch)
2012-07-05 11:58 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.06 KB, patch)
2012-07-05 12:52 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.63 KB, patch)
2012-07-05 13:19 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.63 KB, patch)
2012-07-05 13:30 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (64.38 KB, patch)
2012-07-05 14:06 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (65.71 KB, patch)
2012-07-09 08:20 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (65.66 KB, patch)
2012-07-09 08:46 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (67.79 KB, patch)
2012-07-10 10:51 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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?