Give generatored bindings classes a field remembering an index into a cache of handles on the corre…

Give each binding class an id to be used as an index into a cache of handles on the corresponding Dart classes.

BUG=
R=asiva@google.com, vsm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27452

[siva, 2013-09-10 00:22:43]

Adding Vijay to the reviewers list.

https://codereview.chromium.org/23819035/diff/7001/tools/dom/scripts/idlnode.py
File tools/dom/scripts/idlnode.py (right):

https://codereview.chromium.org/23819035/diff/7001/tools/dom/scripts/idlnode....
tools/dom/scripts/idlnode.py:9: class IDLNode(object):
This file seems to have only white space changes maybe you don't need to change
it.

[vsm, 2013-09-10 00:29:39]

Ryan, can you run the go.sh script under tools/dom/scripts and include any
modified files in this CL?

[rmacnak, 2013-09-10 00:35:46]

On 2013/09/10 00:29:39, vsm wrote:
> Ryan, can you run the go.sh script under tools/dom/scripts and include any
> modified files in this CL?

Done.

[vsm, 2013-09-10 04:41:27]

lgtm

https://codereview.chromium.org/23819035/diff/12001/tools/dom/templates/html/...
File tools/dom/templates/html/dartium/cpp_header.template (right):

https://codereview.chromium.org/23819035/diff/12001/tools/dom/templates/html/...
tools/dom/templates/html/dartium/cpp_header.template:19: static intptr_t
kBindingCid;
Use Blink style instead:

static const int dartClassId;

[rmacnak, 2013-09-10 20:01:12]

On 2013/09/10 04:41:27, vsm wrote:
> lgtm
> 
>
https://codereview.chromium.org/23819035/diff/12001/tools/dom/templates/html/...
> File tools/dom/templates/html/dartium/cpp_header.template (right):
> 
>
https://codereview.chromium.org/23819035/diff/12001/tools/dom/templates/html/...
> tools/dom/templates/html/dartium/cpp_header.template:19: static intptr_t
> kBindingCid;
> Use Blink style instead:
> 
> static const int dartClassId;

Done.

[vsm, 2013-09-10 20:17:09]

https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
File tools/dom/templates/html/dartium/cpp_implementation.template (right):

https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
tools/dom/templates/html/dartium/cpp_implementation.template:31: const int
Dart$(INTERFACE)::dartClassId = DartDOMWrapper::NumCustomBindingClassIds + $CID;
I don't think you need NumCustomBindingClassIds.

Note, for types with a custom cpp, you'll still have a generated cpp.  E.g., a
generated DartWindow.cpp and the handwritten custom/DartWindowCustom.cpp.

[rmacnak, 2013-09-10 20:45:01]

On 2013/09/10 20:17:09, vsm wrote:
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> File tools/dom/templates/html/dartium/cpp_implementation.template (right):
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> tools/dom/templates/html/dartium/cpp_implementation.template:31: const int
> Dart$(INTERFACE)::dartClassId = DartDOMWrapper::NumCustomBindingClassIds +
$CID;
> I don't think you need NumCustomBindingClassIds.
> 
> Note, for types with a custom cpp, you'll still have a generated cpp.  E.g., a
> generated DartWindow.cpp and the handwritten custom/DartWindowCustom.cpp.

This is usually true, but

_HistoryCrossFrameClassId,
_LocationCrossFrameClassId,
_DOMWindowCrossFrameClassId,   There are two Dart classes for one CPP

DOMStringMapClassId = 0,       Neither header nor CPP is generated

_NPObjectClassId,              There is no CPP

(Cf DartDOMWrapper.h in https://chromiumcodereview.appspot.com/23895004/)

[vsm, 2013-09-10 20:53:33]

On 2013/09/10 20:45:01, Ryan Macnak wrote:
> On 2013/09/10 20:17:09, vsm wrote:
> >
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> > File tools/dom/templates/html/dartium/cpp_implementation.template (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> > tools/dom/templates/html/dartium/cpp_implementation.template:31: const int
> > Dart$(INTERFACE)::dartClassId = DartDOMWrapper::NumCustomBindingClassIds +
> $CID;
> > I don't think you need NumCustomBindingClassIds.
> > 
> > Note, for types with a custom cpp, you'll still have a generated cpp.  E.g.,
a
> > generated DartWindow.cpp and the handwritten custom/DartWindowCustom.cpp.
> 
> This is usually true, but
> 
> _HistoryCrossFrameClassId,
> _LocationCrossFrameClassId,
> _DOMWindowCrossFrameClassId,   There are two Dart classes for one CPP
> 
> DOMStringMapClassId = 0,       Neither header nor CPP is generated
> 
> _NPObjectClassId,              There is no CPP
> 
> (Cf DartDOMWrapper.h in https://chromiumcodereview.appspot.com/23895004/)

Good point.

One other thing: before we can upstream to blink, we will need to make our code
gen less monolithic.  I.e., instead of one script that reads all the IDL and
then generates all the output, we will need our scripts to take one IDL file
(and its dependencies) and generate a .h, .cpp.  The cid will probably need to
be rethought when we do this.

Otherwise, LGTM.

[siva, 2013-09-10 23:46:49]

On 2013/09/10 20:53:33, vsm wrote:
> On 2013/09/10 20:45:01, Ryan Macnak wrote:
> > On 2013/09/10 20:17:09, vsm wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> > > File tools/dom/templates/html/dartium/cpp_implementation.template (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> > > tools/dom/templates/html/dartium/cpp_implementation.template:31: const int
> > > Dart$(INTERFACE)::dartClassId = DartDOMWrapper::NumCustomBindingClassIds +
> > $CID;
> > > I don't think you need NumCustomBindingClassIds.
> > > 
> > > Note, for types with a custom cpp, you'll still have a generated cpp. 
E.g.,
> a
> > > generated DartWindow.cpp and the handwritten custom/DartWindowCustom.cpp.
> > 
> > This is usually true, but
> > 
> > _HistoryCrossFrameClassId,
> > _LocationCrossFrameClassId,
> > _DOMWindowCrossFrameClassId,   There are two Dart classes for one CPP
> > 
> > DOMStringMapClassId = 0,       Neither header nor CPP is generated
> > 
> > _NPObjectClassId,              There is no CPP
> > 
> > (Cf DartDOMWrapper.h in https://chromiumcodereview.appspot.com/23895004/)
> 
> Good point.
> 
> One other thing: before we can upstream to blink, we will need to make our
code
> gen less monolithic.  I.e., instead of one script that reads all the IDL and
> then generates all the output, we will need our scripts to take one IDL file
> (and its dependencies) and generate a .h, .cpp.  The cid will probably need to
> be rethought when we do this.
> 
> Otherwise, LGTM.

Getting a valid CID is pretty critical for all the optimizations we are
planning. This needs to work even if we have to go from a monolithic script to a
script that reads one IDL at a time.

If we want to make this future proof maybe we should generate constants like
k<classname>Cid instead of the number we are generating currently and maybe
generate a side file that has an enum declaration of these constants (maybe even
hand write this file for now).

[vsm, 2013-09-11 16:53:37]

On 2013/09/10 23:46:49, siva wrote:
> On 2013/09/10 20:53:33, vsm wrote:
> > On 2013/09/10 20:45:01, Ryan Macnak wrote:
> > > On 2013/09/10 20:17:09, vsm wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> > > > File tools/dom/templates/html/dartium/cpp_implementation.template
(right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/23819035/diff/9001/tools/dom/templates...
> > > > tools/dom/templates/html/dartium/cpp_implementation.template:31: const
int
> > > > Dart$(INTERFACE)::dartClassId = DartDOMWrapper::NumCustomBindingClassIds
+
> > > $CID;
> > > > I don't think you need NumCustomBindingClassIds.
> > > > 
> > > > Note, for types with a custom cpp, you'll still have a generated cpp. 
> E.g.,
> > a
> > > > generated DartWindow.cpp and the handwritten
custom/DartWindowCustom.cpp.
> > > 
> > > This is usually true, but
> > > 
> > > _HistoryCrossFrameClassId,
> > > _LocationCrossFrameClassId,
> > > _DOMWindowCrossFrameClassId,   There are two Dart classes for one CPP
> > > 
> > > DOMStringMapClassId = 0,       Neither header nor CPP is generated
> > > 
> > > _NPObjectClassId,              There is no CPP
> > > 
> > > (Cf DartDOMWrapper.h in https://chromiumcodereview.appspot.com/23895004/)
> > 
> > Good point.
> > 
> > One other thing: before we can upstream to blink, we will need to make our
> code
> > gen less monolithic.  I.e., instead of one script that reads all the IDL and
> > then generates all the output, we will need our scripts to take one IDL file
> > (and its dependencies) and generate a .h, .cpp.  The cid will probably need
to
> > be rethought when we do this.
> > 
> > Otherwise, LGTM.
> 
> Getting a valid CID is pretty critical for all the optimizations we are
> planning. This needs to work even if we have to go from a monolithic script to
a
> script that reads one IDL at a time.
> 
> If we want to make this future proof maybe we should generate constants like
> k<classname>Cid instead of the number we are generating currently and maybe
> generate a side file that has an enum declaration of these constants (maybe
even
> hand write this file for now).

I think a separate .h file with Dart class id constants / enums is a good idea. 
This should be pretty easy to code gen as well.  We will almost certainly see
new and removed IDL types on the next roll.

[rmacnak, 2013-09-12 00:33:12]

Now generating an enum in a separate header file.

[vsm, 2013-09-12 01:08:49]

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
File tools/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:141: path = os.path.join(cpp_output_dir,
'class_ids.h')
The generated bindings files all have camel case.  For consistency, can you call
this "DartClassIds.h"?

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:145: e.Emit('#include
"bindings/dart/DartDOMWrapper.h"\n')
You can combine adjacent calls:
e.Emit(
    '#ifndef class_ids_h\n'
    '#define class_ids_h\n'
...

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:148: e.Emit('    UnusedClassId =
DartDOMWrapper::NumCustomBindingClassIds,\n')
Does it make sense to just list out the builtin classes here (e.g.,
WindowCrossFrame)?

[rmacnak, 2013-09-12 17:56:16]

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
File tools/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:141: path = os.path.join(cpp_output_dir,
'class_ids.h')
On 2013/09/12 01:08:50, vsm wrote:
> The generated bindings files all have camel case.  For consistency, can you
call
> this "DartClassIds.h"?

Done.

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:145: e.Emit('#include
"bindings/dart/DartDOMWrapper.h"\n')
On 2013/09/12 01:08:50, vsm wrote:
> You can combine adjacent calls:
> e.Emit(
>     '#ifndef class_ids_h\n'
>     '#define class_ids_h\n'
> ...

Done.

https://chromiumcodereview.appspot.com/23819035/diff/30001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:148: e.Emit('    UnusedClassId =
DartDOMWrapper::NumCustomBindingClassIds,\n')
On 2013/09/12 01:08:50, vsm wrote:
> Does it make sense to just list out the builtin classes here (e.g.,
> WindowCrossFrame)?

Yes, and it allows us to allocate a fixed-size table.

[vsm, 2013-09-12 18:04:06]

lgtm

[siva, 2013-09-12 18:18:20]

LGTM do we need sdk/lib/html/dartium/html_dartium.dart to be included in this
CL?

https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
File tools/dom/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:141: path = os.path.join(cpp_output_dir,
'DartClassIds.h')
Can we call this file DartWebkitClassIds.h instead of DartClassIds.h to make
that more explicit.

https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:147: #define DartClassIds_h
DartWebkitClassIds_h

https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:155: _NPObjectClassId,
We should add a comment here that whenever new types are introduced which are
not in the auto generated list they need to be added here too.

https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:161: NumClassIds
kNumWebKitClassIds maybe for this maybe.

https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
tools/dom/scripts/dartdomgenerator.py:166: #endif // DartClassIds_h
Ditto comment

[rmacnak, 2013-09-12 21:54:07]

On 2013/09/12 18:18:20, siva wrote:
> LGTM do we need sdk/lib/html/dartium/html_dartium.dart to be included in this
> CL?
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
> File tools/dom/scripts/dartdomgenerator.py (right):
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
> tools/dom/scripts/dartdomgenerator.py:141: path = os.path.join(cpp_output_dir,
> 'DartClassIds.h')
> Can we call this file DartWebkitClassIds.h instead of DartClassIds.h to make
> that more explicit.
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
> tools/dom/scripts/dartdomgenerator.py:147: #define DartClassIds_h
> DartWebkitClassIds_h
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
> tools/dom/scripts/dartdomgenerator.py:155: _NPObjectClassId,
> We should add a comment here that whenever new types are introduced which are
> not in the auto generated list they need to be added here too.
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
> tools/dom/scripts/dartdomgenerator.py:161: NumClassIds
> kNumWebKitClassIds maybe for this maybe.
> 
>
https://chromiumcodereview.appspot.com/23819035/diff/42001/tools/dom/scripts/...
> tools/dom/scripts/dartdomgenerator.py:166: #endif // DartClassIds_h
> Ditto comment

Omitting ordering changes to sdk/lib/html/dartium/html_dartium.dart.
Changed kNumWebKitClassIds to NumWebkitClassIds.

[rmacnak, 2013-09-12 21:55:33]

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