Rework "analysis.updateContent" analysis server request.

Rework "analysis.updateContent" analysis server request.

R=brianwilkerson@google.com

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

[Brian Wilkerson, 2014-08-07 22:15:23]

LGTM after comments are addressed

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

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:191: return;
I think we want to return the errorResponse at this point.

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:195: if (errorResponse != null)
{
In which case errorResponse will never be non-null here.

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:262: String content;
Perhaps change 'content' to 'replacement'?

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:264: int oldLength;
Perhaps rename 'oldLength' to 'length'?

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

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:1585: <field name="oldLength">
"oldLength" --> "length"

[Paul Berry, 2014-08-07 23:58:09]

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

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:191: return;
On 2014/08/07 22:15:22, Brian Wilkerson wrote:
> I think we want to return the errorResponse at this point.

Unfortunately, that doesn't work, because we are inside a callback passed to
foreachMap(), so the return won't return all the way out of updateContent().

Would you prefer the following?

  for (String file in filesDatum.keys) {
    RequestDatum changeDatum = filesDatum[file];
    ContentChange change = new ContentChange();
    switch (...) {
      ...
      default:
        return new Response.invalidParameter(...);
    }
    changes[file] = change;
  }

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:262: String content;
On 2014/08/07 22:15:22, Brian Wilkerson wrote:
> Perhaps change 'content' to 'replacement'?

I'm not sure that would be an improvement, because unfortunately this field is
now serving a dual role: if the change is an AddContentOverlay, then it is the
full file contents; if the change is a ChangeContentOverlay, then it is the
replacement text. :(

How about this?

  /**
   * If [offset] and [length] are null, the full content of the file (or null
   * if the file should be read from the filesystem).
   * 
   * If [offset] and [length] are non-null, the replacement text which should
   * take the place of the [length] characters of the file starting at [offset].
   */
  String contentOrReplacement;

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:264: int oldLength;
On 2014/08/07 22:15:22, Brian Wilkerson wrote:
> Perhaps rename 'oldLength' to 'length'?

Done.

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

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:1585: <field name="oldLength">
On 2014/08/07 22:15:22, Brian Wilkerson wrote:
> "oldLength" --> "length"

Done.

[Brian Wilkerson, 2014-08-08 00:14:58]

LGTM

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

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:191: return;
What I want is non-local returns.

What I need is to see when there's a closure so that I don't miss the fact that
we're running in a different context.

I'm not sure whether the suggested code would be better or not. I leave it to
your judgement.

https://codereview.chromium.org/449213002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_analysis.dart:262: String content;
> How about this?

A bit wordy, but I would have understood what the field actually holds. You can
decide.

[Paul Berry, 2014-08-08 13:15:37]

Committed patchset #1 manually as r39034 (presubmit successful).