Got WebGLRenderingContextBase to work with the "implements" syntax

Got WebGLRenderingContextBase to work with the "implements" syntax

BUG=348033
R=arv@chromium.org, ch.dumez@samsung.com

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

[bajones, 2014-03-21 23:59:43]

Took me a bit to work out the kinks, but I finally got
WebGLRenderingContext(Base) to work using the proper "implements" syntax. Here
it is, in all it's macro-abusing glory. :P If there's anything here that can be
done in a more sensible manner I'm happy to hear about it!

arv@: I'd appreciate your review on this, since you were the one to call out the
problem in the first place.

[arv (Not doing code reviews), 2014-03-22 17:55:12]

This looks good. Can you add some tests?

shouldBe("WebGLRenderingContext.prototype.__proto__", "Object.prototype");

Also, adding Christophe to see if he has some ideas to simplify this further.

[Inactive, 2014-03-23 18:36:31]

Gosh, with such a huge / complicated interface, the result on implementation
side is not pretty... Personally, I would be tempted to reintroduce
[ImplementedInBaseClass]. It did not add that much complexity to the generator
IMHO and it would make the code so much cleaner here.

Haraken, Nils, what do you think?

[haraken, 2014-03-24 01:11:23]

On 2014/03/23 18:36:31, Chris Dumez wrote:
> Gosh, with such a huge / complicated interface, the result on implementation
> side is not pretty... Personally, I would be tempted to reintroduce
> [ImplementedInBaseClass]. It did not add that much complexity to the generator
> IMHO and it would make the code so much cleaner here.
> 
> Haraken, Nils, what do you think?

hmm, [ImplementedInBaseClass] sounds nicer to me.

Why were we able to remove [ImplementedInBaseClass] without such a mess? Is it
just because the number of methods affected by the removal were limited?

[Inactive, 2014-03-24 01:19:31]

On 2014/03/24 01:11:23, haraken wrote:
> On 2014/03/23 18:36:31, Chris Dumez wrote:
> > Gosh, with such a huge / complicated interface, the result on implementation
> > side is not pretty... Personally, I would be tempted to reintroduce
> > [ImplementedInBaseClass]. It did not add that much complexity to the
generator
> > IMHO and it would make the code so much cleaner here.
> > 
> > Haraken, Nils, what do you think?
> 
> hmm, [ImplementedInBaseClass] sounds nicer to me.
> 
> Why were we able to remove [ImplementedInBaseClass] without such a mess? Is it
> just because the number of methods affected by the removal were limited?

Yes, the previously "implemented" interfaces were much smaller. Adding those
static function that proxy calls to methods is boilerplate for non-module cases.

[bajones, 2014-03-24 05:12:11]

On 2014/03/23 18:36:31, Chris Dumez wrote:
> Personally, I would be tempted to reintroduce
> [ImplementedInBaseClass]. It did not add that much complexity to the generator
> IMHO and it would make the code so much cleaner here.

I would welcome anything that reduces the boilerplate required here.
[ImplementedInBaseClass] sounds to my like it addresses the spirit of this
interface dead-on: Allowing multiple iterations of WebGL contexts to use a
common implementation for shared features.

[Nils Barth (inactive), 2014-03-24 05:29:52]

On 2014/03/24 05:12:11, bajones wrote:
> On 2014/03/23 18:36:31, Chris Dumez wrote:
> > Personally, I would be tempted to reintroduce
> > [ImplementedInBaseClass]. It did not add that much complexity to the
generator
> > IMHO and it would make the code so much cleaner here.
> 
> I would welcome anything that reduces the boilerplate required here.
> [ImplementedInBaseClass] sounds to my like it addresses the spirit of this
> interface dead-on: Allowing multiple iterations of WebGL contexts to use a
> common implementation for shared features.

We remove [LegacyImplementedInBaseClass] because it was originally intended
just for a short-term refactoring.
However, since this looks like it's useful long-term, we should re-introduce it.
Will post a CL doing just that shortly.

For reference, the earlier CL was:
Remove support for [LegacyImplementedInBaseClass] IDL extended attribute 
https://codereview.chromium.org/169273002/

[Nils Barth (inactive), 2014-03-24 05:31:27]

On 2014/03/24 05:29:52, Nils Barth wrote:
> For reference, the earlier CL was:
> Remove support for [LegacyImplementedInBaseClass] IDL extended attribute 
> https://codereview.chromium.org/169273002/

...and the earlier bug was:
Kill [LegacyImplementedInBaseClass] IDL attribute
https://code.google.com/p/chromium/issues/detail?id=344224

[Nils Barth (inactive), 2014-03-24 05:50:20]

CL posted adding back the extended attribute:
Add back [ImplementedInBaseClass] IDL extended attribute
https://codereview.chromium.org/209663005/

[Nils Barth (inactive), 2014-03-24 07:22:23]

On 2014/03/24 05:50:20, Nils Barth wrote:
> Add back [ImplementedInBaseClass] IDL extended attribute
> https://codereview.chromium.org/209663005/

It's baaack! (Landed)
Feel free to use [ImplementedInBaseClass] to simplify this CL!

[arv (Not doing code reviews), 2014-03-24 14:28:26]

On 2014/03/24 07:22:23, Nils Barth wrote:
> On 2014/03/24 05:50:20, Nils Barth wrote:
> > Add back [ImplementedInBaseClass] IDL extended attribute
> > https://codereview.chromium.org/209663005/
> 
> It's baaack! (Landed)
> Feel free to use [ImplementedInBaseClass] to simplify this CL!

Thanks Nils.

[bajones, 2014-03-24 21:19:32]

Updated to use [ImplementedInBaseClass]. Simplifies the code considerably!
Thanks!

I had to make a change to compute_interfaces_info.py in order to get this to
compile, though. I've included those modifications in Patch Set 2, but I'm not
fully convinced that it's the right thing to do and I don't think it should be
part of this patch even if it is. The problem is that the unmodified code is
injecting a

#include "None"

into V8WebGLRenderingContextBase.h. I tried modifying the code further down the
line to not inject include statements for None, but then the compile failed on

void WebGLRenderingContextBase* toNative(v8::Handle<v8::Object> object)

Because WebGLRenderingContextBase is never declared. In the end it seems like
taking out the newly re-added statement in compute_interfaces_info.py does the
"right" thing, but I'd like a more informed opinion on that.

Nils?

[arv (Not doing code reviews), 2014-03-24 21:39:49]

https://codereview.chromium.org/205243013/diff/40001/Source/bindings/bindings...
File Source/bindings/bindings.gypi (right):

https://codereview.chromium.org/205243013/diff/40001/Source/bindings/bindings...
Source/bindings/bindings.gypi:232:
'v8/custom/V8WebGLRenderingContextCustom.cpp',
Do you still need the custom bindings?

[bajones, 2014-03-24 21:46:52]

On 2014/03/24 21:39:49, arv wrote:
>
https://codereview.chromium.org/205243013/diff/40001/Source/bindings/bindings...
> File Source/bindings/bindings.gypi (right):
> 
>
https://codereview.chromium.org/205243013/diff/40001/Source/bindings/bindings...
> Source/bindings/bindings.gypi:232:
> 'v8/custom/V8WebGLRenderingContextCustom.cpp',
> Do you still need the custom bindings?

I thought I wouldn't, but alas I still can't compile without that shim. That's a
pretty minor bit of boilerplate compared to the previous patch, though, so I'm
not too worried about it.

[Nils Barth (inactive), 2014-03-25 03:25:27]

On 2014/03/24 21:19:32, bajones wrote:
> Updated to use [ImplementedInBaseClass]. Simplifies the code considerably!
> Thanks!
> 
> I had to make a change to compute_interfaces_info.py in order to get this to
> compile, though. I've included those modifications in Patch Set 2, but I'm not
> fully convinced that it's the right thing to do and I don't think it should be
> part of this patch even if it is. The problem is that the unmodified code is
> injecting a
> 
> #include "None"
> 
> into V8WebGLRenderingContextBase.h. I tried modifying the code further down
the
> line to not inject include statements for None, but then the compile failed on
> 
> void WebGLRenderingContextBase* toNative(v8::Handle<v8::Object> object)
> 
> Because WebGLRenderingContextBase is never declared. In the end it seems like
> taking out the newly re-added statement in compute_interfaces_info.py does the
> "right" thing, but I'd like a more informed opinion on that.
> 
> Nils?

This |implements| usage is a bit weird.
Firstly, since it's an implemented interface, we don't want to generate bindings
for the base interface itself, so we need to move it to the 'dependency' list in
the .gyp file.

This is documented at:
http://www.chromium.org/developers/web-idl-interfaces#TOC-Build

Secondly, sounds like we don't need [ImplementedInBaseClass];
we just want to change the implemented class name.
Could you try using [ImplementedAs=WebGLRenderingContext]
in WebGLRenderingContextBase.idl
...and see how that works?

I'll take a look myself, but we should be able to remove the hacks.

[bajones, 2014-03-25 03:40:58]

On 2014/03/25 03:25:27, Nils Barth wrote:
> This |implements| usage is a bit weird.
> Firstly, since it's an implemented interface, we don't want to generate
bindings
> for the base interface itself, so we need to move it to the 'dependency' list
in
> the .gyp file.
> 
> This is documented at:
> http://www.chromium.org/developers/web-idl-interfaces#TOC-Build

Okay, that makes sense. I'll give it a try!
 
> Secondly, sounds like we don't need [ImplementedInBaseClass];
> we just want to change the implemented class name.
> Could you try using [ImplementedAs=WebGLRenderingContext]
> in WebGLRenderingContextBase.idl
> ...and see how that works?
> 
> I'll take a look myself, but we should be able to remove the hacks.

That sounds like it will conflict with our WebGL2 interface when we get around
to it. (That's the whole reason we're doing this song and dance.)

The full set of interfaces will eventually be:

[NoInterfaceObject]
interface WebGLRenderingContextBase {
 // All the WebGL stuff
}

interface WebGLRenderingContext{};
WebGLRenderingContext implements WebGLRenderingContextBase;

[NoInterfaceObject]
interface WebGL2RenderingContextBase {
 // All the stuff that's new in WebGL2
}
WebGL2RenderingContextBase implements WebGLRenderingContextBase;

interface WebGL2RenderingContext {};
WebGL2RenderingContext implements WebGL2RenderingContextBase;

And of course you can see how that would extend to a WebGL3 interface in time,
so on and so forth. Anything we implement here needs to keep that future pattern
in mind.

[Nils Barth (inactive), 2014-03-25 05:49:42]

Took a look; here are a few changes to make that will reduce the cruft.
(These are due to implements behaving differently from inheritance.)

(The result compiles, but crashes on run; I've suggestion below.)

1. Move the .idl file to the 'dependency' list:
In core.gypi, move
'html/canvas/WebGLRenderingContextBase.idl',
from
'core_idl_files'
to
'core_dependency_idl_files'

This is because it's now an *implemented* interface,
so we only want to create bindings for WebGLRenderingContext,
not for WebGLRenderingContextBase.

2. Update the bindings class in the custom bindings for
WebGLRenderingContextBase
sed -i \
    s/V8WebGLRenderingContextBase/V8WebGLRenderingContext/g \
    Source/bindings/v8/custom/V8WebGLRenderingContextBaseCustom.cpp
...again, b/c we're generating bindings for V8WebGLRenderingContext, not
V8WebGLRenderingContextBase

3. Remove the custom bindings shim for WebGLRenderingContext
...as it's no longer necessary, since we've updated the names.
rm Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp
git reset HEAD Source/bindings/bindings.gypi


The result now compiles, but on running crashes due to symbol lookup error on:
webCoreInitializeScriptWrappableForInterface(WebCore::WebGLRenderingContextBase*)

Specifically:
Release/chrome: symbol lookup error: /w/chr/src/out/Release/lib/libblink_web.so:
undefined symbol:
_Z44webCoreInitializeScriptWrappableForInterfacePN7WebCore25WebGLRenderingContextBaseE
c++filt 
_Z44webCoreInitializeScriptWrappableForInterfacePN7WebCore25WebGLRenderingContextBaseE
webCoreInitializeScriptWrappableForInterface(WebCore::WebGLRenderingContextBase*)

I believe the fix for this is to move V8WebGLRenderingContextBaseCustom.cpp to
V8WebGLRenderingContextCustom.cpp
(...and update bindings.gypi)
...since those are the bindings we're creating, though this may need other
changes.

[bajones, 2014-03-25 05:55:55]

On 2014/03/25 05:49:42, Nils Barth wrote:
> Took a look; here are a few changes to make that will reduce the cruft...

I was just playing with this, actually. I think the solution to the error you
saw is to remove "ScriptWrappable::init()" from the WebGLRenderingContextBase
constructor. That allows me to compile and run cleanly.

I'll put up a new CL first thing in the morning with these changes. Thanks for
the help!

[Nils Barth (inactive), 2014-03-25 05:59:16]

On 2014/03/25 03:40:58, bajones wrote:
> > Secondly, sounds like we don't need [ImplementedInBaseClass];
> > we just want to change the implemented class name.
> > Could you try using [ImplementedAs=WebGLRenderingContext]
> > in WebGLRenderingContextBase.idl
> > ...and see how that works?
> > 
> > I'll take a look myself, but we should be able to remove the hacks.
> 
> That sounds like it will conflict with our WebGL2 interface when we get around
> to it. (That's the whole reason we're doing this song and dance.)

Oops, you're right; [ImplementedAs] means it points to a different class,
while [ImplementedInBaseClass] means to point to the current class.

Thanks for the background, you're right, it does seem like we need
[ImplementedInBaseClass].

[Nils Barth (inactive), 2014-03-25 06:03:43]

On 2014/03/25 05:49:42, Nils Barth wrote:
> The result now compiles, but on running crashes due to symbol lookup error on:
>
webCoreInitializeScriptWrappableForInterface(WebCore::WebGLRenderingContextBase*)

haraken, Chris -- thoughts?

(Looks like this is being called on an instance of the base class, not the
derived class?)

> I believe the fix for this is to move V8WebGLRenderingContextBaseCustom.cpp to
> V8WebGLRenderingContextCustom.cpp

This file renaming, and replacing
WebGLRenderingContextBase with WebGLRenderingContext
in this file seems sane, and it still compiles,
but we still get symbol lookup error.

[bajones, 2014-03-25 17:23:38]

New patch up. Cleanest one yet! Should address all the feedback from Nils about
file structure, etc. PTAL!

[arv (Not doing code reviews), 2014-03-25 17:44:06]

Nice

LGTM

[bajones, 2014-03-25 17:54:35]

On 2014/03/25 17:44:06, arv wrote:
> Nice
> 
> LGTM

Thanks! And I forgot to mention that I'm adding the tests you requested but I'm
doing so in the Khronos WebGL conformance test suite
(https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html)
That's where all of our WebGL tests are handled to ensure cross-browser
conformance. We run those tests on the GPU bots, which will be tree closes
pretty soon.

[bajones, 2014-03-25 18:10:51]

On 2014/03/25 17:54:35, bajones wrote:
> On 2014/03/25 17:44:06, arv wrote:
> > Nice
> > 
> > LGTM
> 
> Thanks! And I forgot to mention that I'm adding the tests you requested but
I'm
> doing so in the Khronos WebGL conformance test suite
>
(https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html)
> That's where all of our WebGL tests are handled to ensure cross-browser
> conformance. We run those tests on the GPU bots, which will be tree closes
> pretty soon.

The test in question: https://github.com/KhronosGroup/WebGL/pull/508

[bajones, 2014-03-25 18:11:04]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:11:09]

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

[commit-bot: I haz the power, 2014-03-25 18:45:48]

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

[commit-bot: I haz the power, 2014-03-25 18:45:48]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[bajones, 2014-03-25 19:42:07]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-25 19:42:13]

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

[commit-bot: I haz the power, 2014-03-25 19:45:09]

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

[commit-bot: I haz the power, 2014-03-25 19:45:10]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[bajones, 2014-03-25 20:13:54]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-03-25 20:13:59]

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

[commit-bot: I haz the power, 2014-03-25 21:27:10]

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

[commit-bot: I haz the power, 2014-03-25 21:27:11]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Inactive, 2014-03-26 01:24:23]

LGTM2

[bajones, 2014-03-26 01:51:43]

Committed patchset #4 manually as r170000 (presubmit successful).