dart2js: Run prepareNativeClasses in the program builder.

dart2js: Run prepareNativeClasses in the program builder.

R=herhut@google.com

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

[floitsch, 2015-01-26 13:17:33]

floitsch@google.com changed reviewers:
+ herhut@google.com

[floitsch, 2015-01-26 13:17:33]

The additionalProperties map will go away in a future CL. So I'm not too worried
about how it is initialized right now.

[herhut, 2015-01-26 13:50:15]

lgtm w/ comments

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/model.dart:29:
{this.outputContainsNativeClasses,
I am not very keen on this pattern. I assume this is supposed to increase
readability at call sites and I buy that. However, in non-checked mode, this
will fail with strange errors (as null gets propagated).

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:55: Map<Class,
Map<String, jsAst.Expression>> additionalProperties;
Have you considered not making this a field but passing it into the emitXXX
functions directly?

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/program_builder.dart (right):

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/program_builder.dart:107: List<Class>
nativeNeededClasses =
This reads strange. Why not neededNativeClasses?

[floitsch, 2015-01-26 17:50:41]

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/model.dart:29:
{this.outputContainsNativeClasses,
On 2015/01/26 13:50:14, herhut wrote:
> I am not very keen on this pattern. I assume this is supposed to increase
> readability at call sites and I buy that. However, in non-checked mode, this
> will fail with strange errors (as null gets propagated). 

I don't think I agree.
Devs should run their code in checked mode at least once. If I add a new call to
this function and I forget an argument it should be detected at first run.
I furthermore believe that the code is read more frequently than it is written.
(In particular calls to this constructor).

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:55: Map<Class,
Map<String, jsAst.Expression>> additionalProperties;
On 2015/01/26 13:50:15, herhut wrote:
> Have you considered not making this a field but passing it into the emitXXX
> functions directly?

As written in my initial message: this field is going away in a future CL.

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/program_builder.dart (right):

https://codereview.chromium.org/868473003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/program_builder.dart:107: List<Class>
nativeNeededClasses =
On 2015/01/26 13:50:15, herhut wrote:
> This reads strange. Why not neededNativeClasses?

Done.

[floitsch, 2015-01-27 12:10:44]

Committed patchset #2 (id:20001) manually as 43177 (presubmit successful).