Mock up code completion.

Mock up code completion.

Includes:

* Mock up semantic analysis by collecting all identifiers.

* Code for talking to Luke's "Dart Mind" service.

* A JavaScript-based UI mock.

R=kasperl@google.com

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

[ahe, 2014-03-12 18:35:49]

Mostly FYI for now. Code is not ready to commit.

[kasperl, 2014-03-13 07:39:58]

Can't wait to try this out in action...

https://codereview.chromium.org/197923002/diff/1/dart/site/try/code_completio...
File dart/site/try/code_completion_ui_mock/experiment.js (right):

https://codereview.chromium.org/197923002/diff/1/dart/site/try/code_completio...
dart/site/try/code_completion_ui_mock/experiment.js:1: var currentEntry;
Copyright notice?

https://codereview.chromium.org/197923002/diff/1/dart/site/try/code_completio...
dart/site/try/code_completion_ui_mock/experiment.js:2: 
It would be helpful to have a comment that explains what this is.

https://codereview.chromium.org/197923002/diff/1/dart/site/try/code_completio...
dart/site/try/code_completion_ui_mock/experiment.js:71: if (cr.bottom - sr.top >
sr.height) {
One of these comparisons use > and the other >=. Is there a reason for that?

https://codereview.chromium.org/197923002/diff/1/dart/site/try/code_completio...
dart/site/try/code_completion_ui_mock/experiment.js:82: String((cr.bottom -
sr.top)) + "px";
No need to use String() here. If either the left or the right side of a + is a
string, you'll get string conversion.

https://codereview.chromium.org/197923002/diff/1/dart/site/try/code_completio...
dart/site/try/code_completion_ui_mock/experiment.js:90: //
document.querySelector('.dart-static').classList.remove('dart-limited-height');
Long line. Maybe explain why this is commented out?

https://codereview.chromium.org/197923002/diff/1/dart/site/try/src/editor.dart
File dart/site/try/src/editor.dart (right):

https://codereview.chromium.org/197923002/diff/1/dart/site/try/src/editor.dar...
dart/site/try/src/editor.dart:130: (String identifier) => identifier != prefix
&& identifier.startsWith(prefix))
Long line.

https://codereview.chromium.org/197923002/diff/1/dart/site/try/src/editor.dar...
dart/site/try/src/editor.dart:132: ..sort();
I think I'd prefer to not use .. for this. It's a lot harder to understand than
a separate line with response.sort().

https://codereview.chromium.org/197923002/diff/1/dart/site/try/src/editor.dar...
dart/site/try/src/editor.dart:143: observer.observe(inputPre, childList: true,
characterData: true, subtree: true);
Long line.

[ahe, 2014-03-24 09:15:55]

I'd like to commit this version. Code completion is disabled by default, but can
be enabled if you know how (we don't advertise this feature as we want to
perform UX studies on our co-workers).

[kasperl, 2014-03-24 10:32:51]

LGTM with a few comments:

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart
(right):

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:11: class
ProjectCommand {
Maybe add a short comment for these classes to give people an understanding of
what they are supposed to be used for?

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:37: ];
Less indentation?

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:69: if
(problem != null) return;
This seems hacky. How about something like this:

  Iterable<String> problems = rules.keys
      .where((name) => !parameters.containsKey(name))
      .map((name) => "Missing parameter: '$name'.")
  if (problems.isNotEmpty) {
    badRequest(problems.first);
    return false;
  }

  Set extra = ...
  if (extra.isEmpty) return true;

  ...
  badRequest(...);
  return false;

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:94: for
(FileSystemEntity entry in entries) {
List<String> files = entries
    .map((e) => e.path)
    .where((e) => !e.endsWith('~') && e.startsWith(nativeDir))
    .map((e) => e.substring(nativeDir.length))
    .map((e) => new Uri.file(e).path)
    .toList();

Not sure if you'll think that's nicer.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/index.html
File dart/site/try/index.html (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/index.html...
dart/site/try/index.html:83: width: 11px;
Do you want two space or four space indents for these? Looks like you mostly use
two.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/Makefile
File dart/site/try/src/Makefile (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/Makefi...
dart/site/try/src/Makefile:9: $(SDK_DIR)/bin/dartanalyzer -p $(PACKAGE_ROOT)
leap.dart --machine 2>&1 | awk -F'|' '{print $$4 ":" $$5 ":" $$6 ": " $$8}'
Break the long line.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/cachin...
File dart/site/try/src/caching_compiler.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/cachin...
dart/site/try/src/caching_compiler.dart:7: import
'package:compiler/compiler.dart' show
This is "weird" file. Maybe add a comment that explains why that is?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
File dart/site/try/src/compilation.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
dart/site/try/src/compilation.dart:305: // cacheCompiler.compilerFor(null);
?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
File dart/site/try/src/compiler_isolate.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
dart/site/try/src/compiler_isolate.dart:69: } else if ('$uri' ==
'$PRIVATE_SCHEME:/main.dart') {
Factor out the '$PRIVATE_SCHEME:/main.dart' string?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/editor...
File dart/site/try/src/editor.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/editor...
dart/site/try/src/editor.dart:60: case hidden: {
No need for { } in a case.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
File dart/site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:104: print('State change
${previous.runtimeType} -> ${runtimeType}.');
Debug printing (repeated)? Maybe introduce some kind of light weight logging
framework (a log function and a const bool to control it) for these things.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:168: for (String query in
['a.diagnostic>span',
const [...]

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:185: 
Remove newline.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:187: 
Removew newline.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:297: for (;token.kind != EOF_TOKEN;
token = token.next) {
I'd put a space after ;.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:568: if (settings.enableDartMind) {
Maybe factor the DartMind support out in its own class / file?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/leap.dart
File dart/site/try/src/leap.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/leap.d...
dart/site/try/src/leap.dart:52: UserOption.storage = window.localStorage;
Maybe "cache" window.localStorage in a local variable? It's a little bit hard on
the eyes.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/messag...
File dart/site/try/src/messages.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/messag...
dart/site/try/src/messages.dart:9: 'Always run in Worker thread.',
Maybe indent the values with two extra spaces to make the pairing (even) more
obvious?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/settin...
File dart/site/try/src/settings.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/settin...
dart/site/try/src/settings.dart:33: _minified.value = b;
_minified is a const. How can you store a value on it?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dart
File dart/site/try/src/ui.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dar...
dart/site/try/src/ui.dart:232: (String response) {
Maybe factor out the then and the catchError closures into methods so it becomes
easier to understand where we're at.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dar...
dart/site/try/src/ui.dart:339: // codePicker.query('li').classes.add('active');
?

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dar...
dart/site/try/src/ui.dart:381: interaction.onMutation([], observer);
Worried about exceptions here at all? try-finally?

[ahe, 2014-03-24 15:20:23]

Thank you, Kasper!

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart
(right):

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:11: class
ProjectCommand {
On 2014/03/24 10:32:51, kasperl wrote:
> Maybe add a short comment for these classes to give people an understanding of
> what they are supposed to be used for?

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:37: ];
On 2014/03/24 10:32:51, kasperl wrote:
> Less indentation?

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:69: if
(problem != null) return;
On 2014/03/24 10:32:51, kasperl wrote:
> This seems hacky. How about something like this:
> 
>   Iterable<String> problems = rules.keys
>       .where((name) => !parameters.containsKey(name))
>       .map((name) => "Missing parameter: '$name'.")
>   if (problems.isNotEmpty) {
>     badRequest(problems.first);
>     return false;
>   }
> 
>   Set extra = ...
>   if (extra.isEmpty) return true;
> 
>   ...
>   badRequest(...);
>   return false;

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/samples/leap_server/leap_server.dart:94: for
(FileSystemEntity entry in entries) {
On 2014/03/24 10:32:51, kasperl wrote:
> List<String> files = entries
>     .map((e) => e.path)
>     .where((e) => !e.endsWith('~') && e.startsWith(nativeDir))
>     .map((e) => e.substring(nativeDir.length))
>     .map((e) => new Uri.file(e).path)
>     .toList();
> 
> Not sure if you'll think that's nicer.

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/index.html
File dart/site/try/index.html (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/index.html...
dart/site/try/index.html:83: width: 11px;
On 2014/03/24 10:32:51, kasperl wrote:
> Do you want two space or four space indents for these? Looks like you mostly
use
> two.

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/Makefile
File dart/site/try/src/Makefile (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/Makefi...
dart/site/try/src/Makefile:9: $(SDK_DIR)/bin/dartanalyzer -p $(PACKAGE_ROOT)
leap.dart --machine 2>&1 | awk -F'|' '{print $$4 ":" $$5 ":" $$6 ": " $$8}'
On 2014/03/24 10:32:51, kasperl wrote:
> Break the long line.

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/cachin...
File dart/site/try/src/caching_compiler.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/cachin...
dart/site/try/src/caching_compiler.dart:7: import
'package:compiler/compiler.dart' show
On 2014/03/24 10:32:51, kasperl wrote:
> This is "weird" file. Maybe add a comment that explains why that is?

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
File dart/site/try/src/compilation.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
dart/site/try/src/compilation.dart:305: // cacheCompiler.compilerFor(null);
On 2014/03/24 10:32:51, kasperl wrote:
> ?

This code will be cleaned up in a follow-up CL.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
File dart/site/try/src/compiler_isolate.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/compil...
dart/site/try/src/compiler_isolate.dart:69: } else if ('$uri' ==
'$PRIVATE_SCHEME:/main.dart') {
On 2014/03/24 10:32:51, kasperl wrote:
> Factor out the '$PRIVATE_SCHEME:/main.dart' string?

I'm working on getting rid of this code.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/editor...
File dart/site/try/src/editor.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/editor...
dart/site/try/src/editor.dart:60: case hidden: {
On 2014/03/24 10:32:51, kasperl wrote:
> No need for { } in a case.

I like {} in this case.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
File dart/site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:104: print('State change
${previous.runtimeType} -> ${runtimeType}.');
On 2014/03/24 10:32:51, kasperl wrote:
> Debug printing (repeated)? Maybe introduce some kind of light weight logging
> framework (a log function and a const bool to control it) for these things.

There isn't much output from these print statements. But I'll think about a
light framework.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:168: for (String query in
['a.diagnostic>span',
On 2014/03/24 10:32:51, kasperl wrote:
> const [...]

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:185: 
On 2014/03/24 10:32:51, kasperl wrote:
> Remove newline.

I've added it to aid navigation in Emacs.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:187: 
On 2014/03/24 10:32:51, kasperl wrote:
> Removew newline.

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:297: for (;token.kind != EOF_TOKEN;
token = token.next) {
On 2014/03/24 10:32:51, kasperl wrote:
> I'd put a space after ;.

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:568: if (settings.enableDartMind) {
On 2014/03/24 10:32:51, kasperl wrote:
> Maybe factor the DartMind support out in its own class / file?

Added TODO.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/leap.dart
File dart/site/try/src/leap.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/leap.d...
dart/site/try/src/leap.dart:52: UserOption.storage = window.localStorage;
On 2014/03/24 10:32:51, kasperl wrote:
> Maybe "cache" window.localStorage in a local variable? It's a little bit hard
on
> the eyes.

Eventually this should be the only place I access window.localStorage directly.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/messag...
File dart/site/try/src/messages.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/messag...
dart/site/try/src/messages.dart:9: 'Always run in Worker thread.',
On 2014/03/24 10:32:51, kasperl wrote:
> Maybe indent the values with two extra spaces to make the pairing (even) more
> obvious?

Done.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/settin...
File dart/site/try/src/settings.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/settin...
dart/site/try/src/settings.dart:33: _minified.value = b;
On 2014/03/24 10:32:51, kasperl wrote:
> _minified is a const. How can you store a value on it?

Through localStorage.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dart
File dart/site/try/src/ui.dart (right):

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dar...
dart/site/try/src/ui.dart:232: (String response) {
On 2014/03/24 10:32:51, kasperl wrote:
> Maybe factor out the then and the catchError closures into methods so it
becomes
> easier to understand where we're at.

Yes. I'll rewrite this. I plan to get rid of the catchError part, and serve
samples through this mechanism as well.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dar...
dart/site/try/src/ui.dart:339: // codePicker.query('li').classes.add('active');
On 2014/03/24 10:32:51, kasperl wrote:
> ?

Dead code.

https://codereview.chromium.org/197923002/diff/60001/dart/site/try/src/ui.dar...
dart/site/try/src/ui.dart:381: interaction.onMutation([], observer);
On 2014/03/24 10:32:51, kasperl wrote:
> Worried about exceptions here at all? try-finally?

onMutation has to be robust. When it throws an exception, things go really
wrong.

[ahe, 2014-03-24 15:31:21]

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