Start new design for interceptors and implement String.charCodeAt with it.

Start new design for interceptors and implement String.charCodeAt with it.
Committed: https://code.google.com/p/dart/source/detail?r=14845

[ahe, 2012-11-09 13:45:28]

Could we go through this in person? I'm not sure I understand everything.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:354:
compiler.jsStringClass);
Looks like a good candidate for a small shared method.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:138: ClassElement
jsStringClass;
Backend specific?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:139: ClassElement
objectInterceptorClass;
Ditto.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:140: Element
getInterceptorMethod;
Ditto.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:142: bool
isForeignClass(Element element) {
Ditto.

[floitsch, 2012-11-09 20:23:02]

First impression: I like it. Looking forward to discussing this in person.

[ahe, 2012-11-12 13:24:11]

Really nice.  LGTM.  Lots of review comments, mostly about adding comments to
the code.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:32: final
Map<SourceString, List<Element>> interceptedElements;
Backend specific?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:670: final
Set<Selector> usedInterceptors;
Document this variable.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:47: final
Map<int, String> boundClosureCache;
Document what "bound closure" means.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:58:
boundClosureInterceptorCache = new Map<int, String>(),
The term bound-closure-interceptor is confusing to me.  interceptorClosureCache
would work much better.  Perhaps with a comment that they are also bound.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:632: // The
ObjectIntercetpr does not have any instance methods.
ObjectIntercetpr -> ObjectInterceptor

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1061: String
extraArg,
extraArg -> extraArgument

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1063: if
(extraArg.isEmpty) {
Could be simplified with this:

extraArgument = extraArgument.isEmpty ? '' : ", '$extraArgument'";

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:9: class
JSString implements String {
Consider moving this to its own file.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:9: class
JSString implements String {
Document this class.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:11: int
charCodeAt(index) {
Add a newline between declarations.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:19: class
ObjectInterceptor {
Document this class.  Explain that all its members are synthesized by the
emitter.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:23:
getInterceptor(obj) {
Document as well.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1860:
world.registerSpecialInstantiatedClass(compiler.jsStringClass);
Perhaps the resolver should just say that a String was instantiated and have the
backend figure out that it needs to enqueue this class?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:56:
backend.usedInterceptors.add(selector);
A helper method instead :-)

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:418:
thisInstruction.sourceElement = closureData.thisElement;
Should this be an argument to the constructor?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2295: if (interceptor
== compiler.getInterceptorMethod) {
Add comment explaining this is the new way.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2302: selector, null,
instruction, true);
Document what true means here.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2303:
instruction.inputs.add(receiver);
Comment here please.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2306: } else {
Add comment explaining this is old style, deprecated?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2643:
visit(node.receiver);
What happens if node.receiver is null?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2651: // The first
entry in the inputs list is the receiver.
Is it the last?

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1523: //
TODO(ngeoffray): Once we remove the old interceptors, we can
Perhaps elaborate on what the hack is.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1983: if
(compiler.isForeignClass(work.element.getEnclosingClass())){
How about "interceptor class" instead of "foreign class".  Since the class
really is a Dart class, it is not foreign.

https://codereview.chromium.org/11365170/diff/4001/tests/language/interceptor...
File tests/language/interceptor_test.dart (right):

https://codereview.chromium.org/11365170/diff/4001/tests/language/interceptor...
tests/language/interceptor_test.dart:5: // Test that interceptors (ie methods in
classes implemented as
ie -> i.e., 

However, "i.e." is actually hard for many people whose primary language isn't
English, so it is a good habit to use "that is" when writing documentation or
comments.

[ngeoffray, 2012-11-13 11:45:16]

Thanks Peter

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:354:
compiler.jsStringClass);
On 2012/11/09 13:45:29, ahe wrote:
> Looks like a good candidate for a small shared method.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:138: ClassElement
jsStringClass;
On 2012/11/09 13:45:29, ahe wrote:
> Backend specific?

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:139: ClassElement
objectInterceptorClass;
On 2012/11/09 13:45:29, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:140: Element
getInterceptorMethod;
On 2012/11/09 13:45:29, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:142: bool
isForeignClass(Element element) {
On 2012/11/09 13:45:29, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:32: final
Map<SourceString, List<Element>> interceptedElements;
On 2012/11/12 13:24:11, ahe wrote:
> Backend specific?

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:670: final
Set<Selector> usedInterceptors;
On 2012/11/12 13:24:11, ahe wrote:
> Document this variable.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:47: final
Map<int, String> boundClosureCache;
On 2012/11/12 13:24:11, ahe wrote:
> Document what "bound closure" means.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:58:
boundClosureInterceptorCache = new Map<int, String>(),
On 2012/11/12 13:24:11, ahe wrote:
> The term bound-closure-interceptor is confusing to me. 
interceptorClosureCache
> would work much better.  Perhaps with a comment that they are also bound.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:632: // The
ObjectIntercetpr does not have any instance methods.
On 2012/11/12 13:24:11, ahe wrote:
> ObjectIntercetpr -> ObjectInterceptor

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1061: String
extraArg,
On 2012/11/12 13:24:11, ahe wrote:
> extraArg -> extraArgument

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1063: if
(extraArg.isEmpty) {
On 2012/11/12 13:24:11, ahe wrote:
> Could be simplified with this:
> 
> extraArgument = extraArgument.isEmpty ? '' : ", '$extraArgument'";

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:9: class
JSString implements String {
On 2012/11/12 13:24:11, ahe wrote:
> Consider moving this to its own file.

In the long run, this file will only contain interceptor classes.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:9: class
JSString implements String {
On 2012/11/12 13:24:11, ahe wrote:
> Document this class.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:11: int
charCodeAt(index) {
On 2012/11/12 13:24:11, ahe wrote:
> Add a newline between declarations.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:19: class
ObjectInterceptor {
On 2012/11/12 13:24:11, ahe wrote:
> Document this class.  Explain that all its members are synthesized by the
> emitter.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:23:
getInterceptor(obj) {
On 2012/11/12 13:24:11, ahe wrote:
> Document as well.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1860:
world.registerSpecialInstantiatedClass(compiler.jsStringClass);
On 2012/11/12 13:24:11, ahe wrote:
> Perhaps the resolver should just say that a String was instantiated and have
the
> backend figure out that it needs to enqueue this class?

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:56:
backend.usedInterceptors.add(selector);
On 2012/11/12 13:24:11, ahe wrote:
> A helper method instead :-)

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:418:
thisInstruction.sourceElement = closureData.thisElement;
On 2012/11/12 13:24:11, ahe wrote:
> Should this be an argument to the constructor?

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2295: if (interceptor
== compiler.getInterceptorMethod) {
On 2012/11/12 13:24:11, ahe wrote:
> Add comment explaining this is the new way.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2302: selector, null,
instruction, true);
On 2012/11/12 13:24:11, ahe wrote:
> Document what true means here.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2303:
instruction.inputs.add(receiver);
On 2012/11/12 13:24:11, ahe wrote:
> Comment here please.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2306: } else {
On 2012/11/12 13:24:11, ahe wrote:
> Add comment explaining this is old style, deprecated?

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2643:
visit(node.receiver);
On 2012/11/12 13:24:11, ahe wrote:
> What happens if node.receiver is null?

Cannot happen right now, but I added a comment.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2651: // The first
entry in the inputs list is the receiver.
On 2012/11/12 13:24:11, ahe wrote:
> Is it the last?

No second. Improved the comment.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1523: //
TODO(ngeoffray): Once we remove the old interceptors, we can
On 2012/11/12 13:24:11, ahe wrote:
> Perhaps elaborate on what the hack is.

Done.

https://codereview.chromium.org/11365170/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1983: if
(compiler.isForeignClass(work.element.getEnclosingClass())){
On 2012/11/12 13:24:11, ahe wrote:
> How about "interceptor class" instead of "foreign class".  Since the class
> really is a Dart class, it is not foreign.

Done.

https://codereview.chromium.org/11365170/diff/4001/tests/language/interceptor...
File tests/language/interceptor_test.dart (right):

https://codereview.chromium.org/11365170/diff/4001/tests/language/interceptor...
tests/language/interceptor_test.dart:5: // Test that interceptors (ie methods in
classes implemented as
On 2012/11/12 13:24:11, ahe wrote:
> ie -> i.e., 
> 
> However, "i.e." is actually hard for many people whose primary language isn't
> English, so it is a good habit to use "that is" when writing documentation or
> comments.

Done.

[karlklose, 2012-11-13 12:25:29]

LGTM.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:733: if
(jsStringClass == null) {
This is a general case, move it outside the if?

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:50: * A
closure is dynamically bound to the instance used when closurization.
closurization -> closurized.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:56: * of
interceptors. Such closure is dynmically bound to the
'Such' -> 'Such a', or change to use 'These'.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:906: " return
receiver.$name($parameters); }");
Use two spaces for indentation?

[ahe, 2012-11-13 12:42:41]

LGTM

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:352: void
enqueue(ClassElement element) {
How about renaming this to registerInstantiatedClass?

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:356: void
enqueueStringClass() {
registerStringInstance()?

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:142: if
(cls.isAbstract(compiler)) {
How about turning all this into a method call on the backend?

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:686: final
Map<SourceString, List<Element>> interceptedElements;
What is this mapping from and to?  member names to members?

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:57: *
interceptor instance, and the actuall receiver of the method.
actuall -> actual.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:39:
getInterceptor(obj) {
obj -> object?

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:49: // Check if we
have an interceptor implemented with classes.
How about:

// Check if we have an interceptor method implemented with interceptor classes.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2291: // interceptor
method and then the actuall dynamic call on the
actuall -> actual

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2639: //
TODO(ngeoffray): This is currently the case, but should be
What does "This" mean here?

[ngeoffray, 2012-11-13 12:51:50]

Thanks Karl and Peter.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:733: if
(jsStringClass == null) {
On 2012/11/13 12:25:30, karlklose wrote:
> This is a general case, move it outside the if?

I think I will define objectInterceptorclass and getInterceptorMethod somewhere
else. Let me wait for the next CL to move those around.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:50: * A
closure is dynamically bound to the instance used when closurization.
On 2012/11/13 12:25:30, karlklose wrote:
> closurization -> closurized.

Done.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:56: * of
interceptors. Such closure is dynmically bound to the
On 2012/11/13 12:25:30, karlklose wrote:
> 'Such' -> 'Such a', or change to use 'These'.

Done.

https://codereview.chromium.org/11365170/diff/11001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:906: " return
receiver.$name($parameters); }");
On 2012/11/13 12:25:30, karlklose wrote:
> Use two spaces for indentation?

No, the string is just one line.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:352: void
enqueue(ClassElement element) {
On 2012/11/13 12:42:41, ahe wrote:
> How about renaming this to registerInstantiatedClass?

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:356: void
enqueueStringClass() {
On 2012/11/13 12:42:41, ahe wrote:
> registerStringInstance()?

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:142: if
(cls.isAbstract(compiler)) {
On 2012/11/13 12:42:41, ahe wrote:
> How about turning all this into a method call on the backend?

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:686: final
Map<SourceString, List<Element>> interceptedElements;
On 2012/11/13 12:42:41, ahe wrote:
> What is this mapping from and to?  member names to members?

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:57: *
interceptor instance, and the actuall receiver of the method.
On 2012/11/13 12:42:41, ahe wrote:
> actuall -> actual.

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:39:
getInterceptor(obj) {
On 2012/11/13 12:42:41, ahe wrote:
> obj -> object?

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:49: // Check if we
have an interceptor implemented with classes.
On 2012/11/13 12:42:41, ahe wrote:
> How about:
> 
> // Check if we have an interceptor method implemented with interceptor
classes.

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2291: // interceptor
method and then the actuall dynamic call on the
On 2012/11/13 12:42:41, ahe wrote:
> actuall -> actual

Done.

https://codereview.chromium.org/11365170/diff/8002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2639: //
TODO(ngeoffray): This is currently the case, but should be
On 2012/11/13 12:42:41, ahe wrote:
> What does "This" mean here?

The assertion. Comment added.