Moved the majority of WebGL functionality into WebGLRenderingContextBase

Moved the majority of WebGL functionality into WebGLRenderingContextBase

This is in anticipation of the requirements for implementing WebGL2

BUG=295792
R=kbr@chromium.org, marja@chromium.org, zmo@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167715

[bajones, 2013-09-20 19:24:24]

Putting this up for the sake of discussion. No WebGL 2 functionality here, but
this gets the class hierarchy in order for it's implementation later on.

Apologies for the large size of the patch, but this is one of those "all or
nothing" things. Theres a couple of things I'd like feedback on specifically
noted below.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/ANGLE...
File Source/core/html/canvas/ANGLEInstancedArrays.h (right):

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/ANGLE...
Source/core/html/canvas/ANGLEInstancedArrays.h:44: static
PassRefPtr<ANGLEInstancedArrays> create(WebGLRenderingContextBase*);
Lots and lots of these kind of changes. This could be avoided by keeping the
base class named "WebGLRenderingContext" and naming the other class
"WebGL1RenderingContext" (It looks like Firefoxes current commits are doing
this) but since we don't want to change the name of the Javascript type (I
assume?) this would create a disconnect between the Javascript and C++ class
names/hierarchy. Personally I'd rather go through the pain of a large rename
once and preserve consistency moving forward.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContext.cpp:57:
PassOwnPtr<WebGLRenderingContext>
WebGLRenderingContext::create(HTMLCanvasElement* canvas, WebGLContextAttributes*
attrs)
I'd like to abstract this function some more, because the current setup means it
will have to be copied verbatim in the WebGL2 context. I'm thinking that this
may be a good candidate for a template function.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContext.cpp:113: void
WebGLRenderingContext::registerContextExtensions()
Moving out of base ensures that the WebGL2 context won't pick up this context's
extensions.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContextBase.h:128: GC3Dsizei height,
GC3Dint border, ArrayBufferView* data);
Apologies for the clutter, but cl upload decided to flag me on all the
tab-misalignment cruft that had accumulated in this file over time. I figured
there was no time like the present to clean it up.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContextBase.h:517: // Enabled extension
objects.
This is an area worth evaluating. Currently the context checks these extension
objects against NULL to determine if an extension has been enabled. It would be
nice to move these to the non-base context, but we would need to replace that
check somehow. The "OOP" way would probably be to make any function that checks
for extensions virtual or similar. A cleaner way may be to flag extension names
(enums) as enabled in a hash. That way the check can become:

if (extensionEnabled(WebGLExtension::EXTFragDepthName)) { ... }

[Zhenyao Mo, 2013-09-20 23:04:44]

I like this CL.  Thanks for this effort to start the wheel.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLDepthTexture.h (right):

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLDepthTexture.h:38: static bool
supported(WebGLRenderingContextBase*);
I suggest we change supported(WebGLRenderingContextBase*) to
supported(GraphicsContext3D*), but it's not about whether this extension is
supported in a webgl context, but whether the underlying gl impl supports this. 
Using GraphicsContext3D will be semantically clear.

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContextBase.h:381: const String
m_contextName;
I still feel like we should have a unsigned int m_version number.  contextName
might change ("experimental" or without), but the version number should be
consistent.

[bajones, 2013-09-20 23:08:49]

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/24096029/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContextBase.h:381: const String
m_contextName;
On 2013/09/20 23:04:45, Zhenyao Mo wrote:
> I still feel like we should have a unsigned int m_version number.  contextName
> might change ("experimental" or without), but the version number should be
> consistent.

If we want to branch functionality based on WebGL version at some point I
totally agree. But that's not what this is for.

In this case there are several errors generated that include
"WebGLRenderingContext" as the source. The string was added so that
errors/logging/etc. can correctly identify the context type that they originated
from.

[bajones, 2013-09-24 00:12:00]

Some weekend changes for your consideration.

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLExtensionNames.h (right):

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLExtensionNames.h:32: enum WebGLExtensionName {
These used to live under WebGLExtension, moved out to avoid circular dependency
when included by WebGLRenderingContextBase

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLExtensionNames.h:51: WebGLExtensionNameCount, //
Must be the last entry
Allows the array of "extensionEnabled" bools in the context to be statically
allocated. I'm assuming that we should never have name conflicts between a WebGL
extension and a WebGL2 extension.

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLRenderingContext.h (right):

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContext.h:43: // Enabled extension
objects.
Technically these aren't even being used anymore. Not sure if we want to
investigate removing them entirely.

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:501: ,
m_featureLevel(featureLevel)
This would be the feature level indicator that @zmo alluded to earlier. It would
primarily allow us to not depend on extension checks for certain features. eg:

if (extensionEnabled(OESElementIndexUintName) || m_featureLevel >= 2)

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5126: if
(extensionEnabled(OESElementIndexUintName))
Lots of changes like this throughout. More verbose, but allows for cleaner
abstraction.

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/24096029/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLRenderingContextBase.h:632: inline bool
extensionEnabled(WebGLExtensionName name)
It turns out that every single use of the WebGL extension objects in the context
were nothing more than tests to see if the extension had been queried. As such
we can replace the extension objects in the base with a simple boolean check.
The extension objects now reside on WebGLRenderingContext where they arguably
belong.

[bajones, 2013-09-29 17:45:34]

After doing some more work on a WebGL 2 prototype built on top of this, I think
the code is currently in a good state to commit. I'd like to get this in place
soon to ensure that any upcoming changes conform to the new structure and don't
impede WebGL 2 development when we reach that point.

Please take a look!

[Zhenyao Mo, 2013-09-30 20:46:55]

On 2013/09/29 17:45:34, bajones wrote:
> After doing some more work on a WebGL 2 prototype built on top of this, I
think
> the code is currently in a good state to commit. I'd like to get this in place
> soon to ensure that any upcoming changes conform to the new structure and
don't
> impede WebGL 2 development when we reach that point.
> 
> Please take a look!

LGTM.  Thank you very much for pushing this forward.  And sorry about the nasty
rebase.

[bajones, 2013-10-01 06:17:15]

Nasty rebase done. :P

kbr@: Please take a look

dcarney@, marja@:  I'd appreciate a review for the bindings. If there's a better
individual to review this change please let me know!

[marja, 2013-10-02 09:05:50]

On 2013/10/01 06:17:15, bajones wrote:
> Nasty rebase done. :P
> 
> kbr@: Please take a look
> 
> dcarney@, marja@:  I'd appreciate a review for the bindings. If there's a
better
> individual to review this change please let me know!

The changes to Source/bindings/v8/custom/V8WebGLRenderingContextBaseCustom.cpp
look mechanical to me, and the other bindings/ change is a deleted custom
bindings file (the less custom bindings the better) - so lgtm.

[bajones, 2013-10-10 23:28:35]

Give today's WebGL call and the decision to use the "webgl2" contextId, this
should be the right code now to build WebGL2 on. A semi-timely review would be
appreciated, since this code is a beast to rebase...

[bajones, 2013-10-29 23:30:25]

Bi-weekly hand waving reminder that this CL would really appreciate some love.
It really is painful to rebase. :P

[bajones, 2013-12-03 00:16:40]

Another visit from the rebase fairy!

[Ken Russell (switch to Gerrit), 2013-12-04 00:23:59]

Again, I apologize for not reviewing this yet.

Per our offline discussion, I would strongly prefer not to land this until some
of the serious outstanding WebGL bugs have been fixed, because after it lands,
merging WebGL fixes back to previous releases will become impossible. There are
a few including Issue 319265, Issue 313779, and Issue 299371.

[bajones, 2014-02-13 19:15:25]

Now that the GraphicsContext3D removal is complete I felt it would be a good
time to revisit this long neglected CL. Enough time has passed and enough
changes made that it was more practical this time around to re-write the changes
by hand instead of rebasing it.

While previous versions of this patch also included a multitude of WebKit style
fixes for the changed files I opted to leave those changes out of this revision,
since the patch is large enough as is and I don't want to make it any harder
than it already is to get this reviewed.

Given the nature and scope of these updates I really would like to see them land
soon so that future CLs will take into account the new structure and not
introduce any changes that may make this required transition more difficult down
the road.

[Ken Russell (switch to Gerrit), 2014-02-21 23:40:19]

I'm sorry for taking so long to review this. LGTM.

https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:4111:
m_extensionEnabled[i] = 0;
Use false instead of 0?

Can this code be shared with that in
WebGLRenderingContextBase::initializeNewContext? Not worth it if it's a big
deal.

https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.idl (right):

https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.idl:3: // found in the LICENSE
file.
I wonder what the copyright policy is in this case when the original file's
being copied over to a new one, but it still largely the same content.

[bajones, 2014-02-22 00:47:08]

https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.idl (right):

https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.idl:3: // found in the LICENSE
file.
On 2014/02/21 23:40:20, Ken Russell wrote:
> I wonder what the copyright policy is in this case when the original file's
> being copied over to a new one, but it still largely the same content.

I wondered about the same thing. Do you have any idea where the best place is to
find out about that?

[Ken Russell (switch to Gerrit), 2014-02-22 00:55:22]

On 2014/02/22 00:47:08, bajones wrote:
>
https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
> File Source/core/html/canvas/WebGLRenderingContextBase.idl (right):
> 
>
https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
> Source/core/html/canvas/WebGLRenderingContextBase.idl:3: // found in the
LICENSE
> file.
> On 2014/02/21 23:40:20, Ken Russell wrote:
> > I wonder what the copyright policy is in this case when the original file's
> > being copied over to a new one, but it still largely the same content.
> 
> I wondered about the same thing. Do you have any idea where the best place is
to
> find out about that?

I suggest asking mal@ .

[bajones, 2014-02-22 00:58:30]

On 2014/02/22 00:55:22, Ken Russell wrote:
> On 2014/02/22 00:47:08, bajones wrote:
> >
>
https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
> > File Source/core/html/canvas/WebGLRenderingContextBase.idl (right):
> > 
> >
>
https://codereview.chromium.org/24096029/diff/141001/Source/core/html/canvas/...
> > Source/core/html/canvas/WebGLRenderingContextBase.idl:3: // found in the
> LICENSE
> > file.
> > On 2014/02/21 23:40:20, Ken Russell wrote:
> > > I wonder what the copyright policy is in this case when the original
file's
> > > being copied over to a new one, but it still largely the same content.
> > 
> > I wondered about the same thing. Do you have any idea where the best place
is
> to
> > find out about that?
> 
> I suggest asking mal@ .

Already asked on blink-dev. eseidel@ Said he felt the license block should be
carried over.

[bajones, 2014-02-22 01:00:38]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-22 01:00:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/24096029/741001

[commit-bot: I haz the power, 2014-02-22 01:44:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-22 01:44:42]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[bajones, 2014-02-23 22:55:53]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-23 22:56:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/24096029/951001

[commit-bot: I haz the power, 2014-02-23 23:44:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-23 23:44:27]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[bajones, 2014-02-24 06:17:18]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-24 06:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/24096029/1101001

[commit-bot: I haz the power, 2014-02-24 06:59:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-24 06:59:45]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[bajones, 2014-02-24 17:49:09]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-24 17:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/24096029/1281001

[commit-bot: I haz the power, 2014-02-24 18:59:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-24 18:59:11]

Retried try job too often on win_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[bajones, 2014-02-24 23:33:06]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-24 23:33:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/24096029/1421001

[bajones, 2014-02-25 00:52:02]

Committed patchset #15 manually as r167715 (presubmit successful).

[arv (Not doing code reviews), 2014-02-27 22:09:59]

https://codereview.chromium.org/24096029/diff/1421001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.idl (right):

https://codereview.chromium.org/24096029/diff/1421001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.idl:44: NoInterfaceObject
Why?

https://code.google.com/p/chromium/issues/detail?id=347714