Add dartExperimentalFixupGetTag for polymer experiments.

Add dartExperimentalFixupGetTag for polymer experiments.

If global JavaScript name dartExperimentalFixupGetTag is a function,
it is used to generate a 'hooked' version of the code that inspects an
object to discover the native class dispatch tag.

R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2013-07-23 22:53:46]

lgtm!

Awesome Stephen, thank you

[sra1, 2013-07-23 22:57:07]

Committed patchset #2 manually as r25379 (presubmit successful).

[ahe, 2013-07-24 08:46:18]

LGTM, but I'd prefer if you avoid making stuff private.

More importantly, consider adding the extra tests and eliminating
_getTagJSFunction using a bound closure.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
File sdk/lib/_internal/lib/native_helper.dart (right):

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:146: Function _getTypeNameOf;
I would prefer this:

final Function getTypeNameOf = getFunctionForTypeNameOf();

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:162: if (JS('bool', 'typeof
dartExperimentalFixupGetTag == "function"')) {
Which is better:

if (JS('bool', 'typeof dartExperimentalFixupGetTag == "function"')) {

or:

if (JS('String', 'typeof dartExperimentalFixupGetTag') == 'function') {

I'm asking because I don't know. If they are roughly the same, I think I would
prefer the latter as it has the least JS code.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:169: Function
_getFunctionForTypeNameOf() {
Please don't make stuff private.  Library privacy is a broken concept, and it
doesn't convey the intent.

In this case, the method is internal and should only be called by
getFunctionTypeNameOf, but it might still be good to test it.  However, by
making it private you say: "this method is private to this library (so any
method in this library might want to call it) and it should never be tested".

I have a tendency to request unit tests of functions that are made private to
help programmers through the thought process of how library privacy simply
doesn't work.

If you need to hide something, you can create a separate library for it. 
However, this is mostly overkill.

In my opinion, this is a much better solution:

/// Do not call this directly, call [getFunctionForTypeNameOf] instead.
Function getFunctionForTypeNameOfImplementation() {

This:

* Clearly documents the intent.

* Helps your co-workers avoid accidentally calling this method from elsewhere in
this library.

* Allows the method to be tested.

I think it is perfectly fine to use private instance members to hide
implementation details in implementation classes when this protects your users
against accidentally using undocumented implementation details that could lead
to compatibility problems.  However, when your users are your co-workers, who
also work on the same library, you need to be clear about intent, and you need
to trust them to do the right thing.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:194: Function
_applyExperimentalFixup(fixupJSFunction,
Please don't make stuff private.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:199: _getTagJSFunction =
originalGetTagFunction;
I would prefer this:

var boundClosure =
    JS('', '#.bind(null, #)', DART_CLOSURE_TO_JS(_callGetTagJSFunction),
       originalGetTagFunction);

Or possibly:

var boundClosure =
    JS('', '(function() { return function(object) { return #(#, object) } })()',
       DART_CLOSURE_TO_JS(callGetTagJSFunction),
       originalGetTagFunction);

...

callGetTagJSFunction(tagFunction, object) => tagFunction(object);

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:210: var _getTagJSFunction;
Please don't make stuff private.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:211: _callGetTagJSFunction(object) =>
_getTagJSFunction(object);
Please don't make stuff private.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:213: 
Extra lines.

https://codereview.chromium.org/19533006/diff/3001/tests/compiler/dart2js_nat...
File tests/compiler/dart2js_native/fixup_get_tag_test.dart (right):

https://codereview.chromium.org/19533006/diff/3001/tests/compiler/dart2js_nat...
tests/compiler/dart2js_native/fixup_get_tag_test.dart:9: class Foo native "A" { 
// There is one native class with dispatch tag 'A'.
Extract space before //.

https://codereview.chromium.org/19533006/diff/3001/tests/compiler/dart2js_nat...
tests/compiler/dart2js_native/fixup_get_tag_test.dart:49: Expect.equals('isB',
b.token());
Would it make sense to add:

Expect.isTrue(a is A);
Expect.isTrue(b is A);

[sra1, 2013-07-25 21:47:09]

code review fixes in https://codereview.chromium.org/20509002/

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
File sdk/lib/_internal/lib/native_helper.dart (right):

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:162: if (JS('bool', 'typeof
dartExperimentalFixupGetTag == "function"')) {
On 2013/07/24 08:46:18, ahe wrote:
> Which is better:
> 
> if (JS('bool', 'typeof dartExperimentalFixupGetTag == "function"')) {
> 
> or:
> 
> if (JS('String', 'typeof dartExperimentalFixupGetTag') == 'function') {
> 
> I'm asking because I don't know. If they are roughly the same, I think I would
> prefer the latter as it has the least JS code.

I thing the first is better since it guarantees the generated code contains 

   typeof dartExperimentalFixupGetTag == "function"

which is compiled by V8 to a special test that is much more efficient than

   typeof dartExperimentalFixupGetTag

There is a small (but probably unrealistic) chance that giving the compiler
flexibility will let it generate code (e.g. via outlining) that is not
recognized  by the V8 as a function check.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:169: Function
_getFunctionForTypeNameOf() {
On 2013/07/24 08:46:18, ahe wrote:
> Please don't make stuff private.  Library privacy is a broken concept, and it
> doesn't convey the intent.
> 
> In this case, the method is internal and should only be called by
> getFunctionTypeNameOf, but it might still be good to test it.  However, by
> making it private you say: "this method is private to this library (so any
> method in this library might want to call it) and it should never be tested".
> 
> I have a tendency to request unit tests of functions that are made private to
> help programmers through the thought process of how library privacy simply
> doesn't work.
> 
> If you need to hide something, you can create a separate library for it. 
> However, this is mostly overkill.
> 
> In my opinion, this is a much better solution:
> 
> /// Do not call this directly, call [getFunctionForTypeNameOf] instead.
> Function getFunctionForTypeNameOfImplementation() {
> 
> This:
> 
> * Clearly documents the intent.
> 
> * Helps your co-workers avoid accidentally calling this method from elsewhere
in
> this library.
> 
> * Allows the method to be tested.
> 
> I think it is perfectly fine to use private instance members to hide
> implementation details in implementation classes when this protects your users
> against accidentally using undocumented implementation details that could lead
> to compatibility problems.  However, when your users are your co-workers, who
> also work on the same library, you need to be clear about intent, and you need
> to trust them to do the right thing.

Done.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:194: Function
_applyExperimentalFixup(fixupJSFunction,
On 2013/07/24 08:46:18, ahe wrote:
> Please don't make stuff private.

Done.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:210: var _getTagJSFunction;
On 2013/07/24 08:46:18, ahe wrote:
> Please don't make stuff private.

Done.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:211: _callGetTagJSFunction(object) =>
_getTagJSFunction(object);
On 2013/07/24 08:46:18, ahe wrote:
> Please don't make stuff private.

Done.

https://codereview.chromium.org/19533006/diff/3001/sdk/lib/_internal/lib/nati...
sdk/lib/_internal/lib/native_helper.dart:213: 
On 2013/07/24 08:46:18, ahe wrote:
> Extra lines.

Done.

https://codereview.chromium.org/19533006/diff/3001/tests/compiler/dart2js_nat...
File tests/compiler/dart2js_native/fixup_get_tag_test.dart (right):

https://codereview.chromium.org/19533006/diff/3001/tests/compiler/dart2js_nat...
tests/compiler/dart2js_native/fixup_get_tag_test.dart:9: class Foo native "A" { 
// There is one native class with dispatch tag 'A'.
On 2013/07/24 08:46:18, ahe wrote:
> Extract space before //.

Hmm.  I recall being told in a code review to put two spaces before the comment
but a quick search didn't find it.

https://codereview.chromium.org/19533006/diff/3001/tests/compiler/dart2js_nat...
tests/compiler/dart2js_native/fixup_get_tag_test.dart:49: Expect.equals('isB',
b.token());
On 2013/07/24 08:46:18, ahe wrote:
> Would it make sense to add:
> 
> Expect.isTrue(a is A);
> Expect.isTrue(b is A);

Should be: Expect.isTrue(b is Foo);
Done.