Compressed/optimized navigation notification.

Compressed/optimized navigation notification.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2014-12-01 21:42:15]

LGTM

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:700: <field name="targetFiles">
How about "files"? The name "targetFiles" is a little more descriptive, but also
more verbose, and I think the shorter name is good enough.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2402: <field name="targetIndexes">
I'd rather leave this as "targets". We didn't add "Indexes" in TypeHierarchyItem
where we also use indexes into an array held by a containing object.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2414: A description of a target to
which the user can navigate to.
Remove second "to".

[Paul Berry, 2014-12-01 21:50:37]

The only blocking issue for me is storing all targets/files in the
NavigationRegion object rather than just the targets/files that are needed. 
Otherwise LGTM.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/computer/computer_navigation.dart (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/computer_navigation.dart:54: index =
targetMap.length;
Nit: how about "targets.length" instead?  This makes it easier to understand why
the computed index will be correct, and it makes this function more consistent
with _addFile().

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol_server.dart (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol_server.dart:230: fileToIndex(String file))
{
For additional type safety, let's make this "int fileToIndex(String file)"

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/codegen_java_types.dart (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/codegen_java_types.dart:458: writeln(' 
this.allTargets = allTargets;');
I would feel more comfortable if, instead of storing all targets in the data
structure and deferring the lookup until getTargets(), we simply did the lookup
immediately and stored just the targets we need.  That way each NavigationRegion
object would only be holding references to a small number of targets.  That
should avoid memory bloat when we start implementing paged notifications.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/codegen_java_types.dart:476: writeln(' 
this.allTargetFiles = allTargetFiles;');
Similar situation here.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2405: The indexes of the targets
(in the enclosing navigation response)
s/indexes/indices/

[scheglov, 2014-12-01 22:20:32]

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/computer/computer_navigation.dart (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/computer/computer_navigation.dart:54: index =
targetMap.length;
On 2014/12/01 21:50:37, Paul Berry wrote:
> Nit: how about "targets.length" instead?  This makes it easier to understand
why
> the computed index will be correct, and it makes this function more consistent
> with _addFile().

Done.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol_server.dart (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol_server.dart:230: fileToIndex(String file))
{
On 2014/12/01 21:50:37, Paul Berry wrote:
> For additional type safety, let's make this "int fileToIndex(String file)"

Done.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/codegen_java_types.dart (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/codegen_java_types.dart:458: writeln(' 
this.allTargets = allTargets;');
On 2014/12/01 21:50:37, Paul Berry wrote:
> I would feel more comfortable if, instead of storing all targets in the data
> structure and deferring the lookup until getTargets(), we simply did the
lookup
> immediately and stored just the targets we need.  That way each
NavigationRegion
> object would only be holding references to a small number of targets.  That
> should avoid memory bloat when we start implementing paged notifications.

Good idea.
Thank you.
Done.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:700: <field name="targetFiles">
On 2014/12/01 21:42:14, Brian Wilkerson wrote:
> How about "files"? The name "targetFiles" is a little more descriptive, but
also
> more verbose, and I think the shorter name is good enough.

Done.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2402: <field name="targetIndexes">
On 2014/12/01 21:42:14, Brian Wilkerson wrote:
> I'd rather leave this as "targets". We didn't add "Indexes" in
TypeHierarchyItem
> where we also use indexes into an array held by a containing object.

Done.

https://codereview.chromium.org/766323002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2414: A description of a target to
which the user can navigate to.
On 2014/12/01 21:42:14, Brian Wilkerson wrote:
> Remove second "to".

Done.

[scheglov, 2014-12-01 22:21:46]

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