Allow replacementOffset and Length overriding

Allow replacementOffset and replacementLength overriding from plugin autocompletion. In cases where a plugin overrides the original Dart autocompletion offset/length or another plugin's offset/length - the suggestion for the plugin is dropped and an error notification is logged.

BUG=
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/82490f0d2f82b2ec98dfa35593289d2c51aeab63

[maxkim, 2017-05-30 18:05:55]

Patchset #2 (id:20001) has been deleted

[maxkim, 2017-05-30 18:09:17]

Description was changed from

==========
Allow replacementOffset and Length overriding

BUG=
==========

to

==========
Allow replacementOffset and Length overriding

BUG=
==========

[maxkim, 2017-05-30 18:09:17]

maxkim@google.com changed reviewers:
+ brianwilkerson@google.com, konstantin.scheglov@gmail.com

[maxkim, 2017-05-30 18:10:13]

Description was changed from

==========
Allow replacementOffset and Length overriding

BUG=
==========

to

==========
Allow replacementOffset and replacementLength overriding from plugin
autocompletion. In cases where a plugin overrides the original Dart
autocompletion offset/length or another plugin's offset/length - the suggestion
for the plugin is dropped and an error notification is logged.

BUG=
==========

[Brian Wilkerson, 2017-05-30 18:30:21]

brianwilkerson@google.com changed reviewers:
+ scheglov@google.com
- konstantin.scheglov@gmail.com

[Brian Wilkerson, 2017-05-30 18:30:22]

lgtm after comment is addressed.

https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/domain_completion.dart (right):

https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_completion.dart:126:
request.replacementLength = result.replacementLength;
I think we only want to overwrite the current values if there are no current
values. I suspect that you're assuming that if there are no suggestions that
server won't have set these values into the request, but I'm not sure that's
always true. We could make it safe by replacing "=" with "??=".

[maxkim, 2017-05-30 19:10:33]

https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/domain_completion.dart (right):

https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_completion.dart:126:
request.replacementLength = result.replacementLength;
On 2017/05/30 18:30:22, Brian Wilkerson wrote:
> I think we only want to overwrite the current values if there are no current
> values. I suspect that you're assuming that if there are no suggestions that
> server won't have set these values into the request, but I'm not sure that's
> always true. We could make it safe by replacing "=" with "??=".

Adding the null-aware assignment is making the replacementOffset not be properly
override by the plugin's result.replacementOffset; I believe the length is
working fine since the value is null. 

I don't fully understand why the original `request.replacementOffset` and
`request.replacementLength` being null matters if suggestions is already empty.
By this point, the primary Dart autocompletion has had time to finish its
autocompletion calculations and if suggestions is empty, it means there is no
danger of overriding these two values. 

However, I do think I should add another check to make sure that the plugin has
suggestions in the first place before even entering this block.

[Brian Wilkerson, 2017-05-30 19:18:49]

https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/domain_completion.dart (right):

https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_completion.dart:126:
request.replacementLength = result.replacementLength;
> I don't fully understand why the original `request.replacementOffset` and
> `request.replacementLength` being null matters if suggestions is already
empty.

I'm probably just being too cautious. I'm willing to leave it as is.

> However, I do think I should add another check to make sure that the plugin
has
> suggestions in the first place before even entering this block. 

That sounds like a good idea.

[maxkim, 2017-05-30 20:42:24]

On 2017/05/30 19:18:49, Brian Wilkerson wrote:
>
https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
> File pkg/analysis_server/lib/src/domain_completion.dart (right):
> 
>
https://codereview.chromium.org/2913833002/diff/40001/pkg/analysis_server/lib...
> pkg/analysis_server/lib/src/domain_completion.dart:126:
> request.replacementLength = result.replacementLength;
> > I don't fully understand why the original `request.replacementOffset` and
> > `request.replacementLength` being null matters if suggestions is already
> empty.
> 
> I'm probably just being too cautious. I'm willing to leave it as is.
> 
> > However, I do think I should add another check to make sure that the plugin
> has
> > suggestions in the first place before even entering this block. 
> 
> That sounds like a good idea.

Added the changes. Let me know if the changes still look good for submission.

[Brian Wilkerson, 2017-05-30 20:53:45]

Yep, lgtm.

[maxkim, 2017-05-30 20:54:18]

Description was changed from

==========
Allow replacementOffset and replacementLength overriding from plugin
autocompletion. In cases where a plugin overrides the original Dart
autocompletion offset/length or another plugin's offset/length - the suggestion
for the plugin is dropped and an error notification is logged.

BUG=
==========

to

==========
Allow replacementOffset and replacementLength overriding from plugin
autocompletion. In cases where a plugin overrides the original Dart
autocompletion offset/length or another plugin's offset/length - the suggestion
for the plugin is dropped and an error notification is logged.

BUG=
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/82490f0d2f82b2ec98dfa35593289d2c51aeab63
==========

[maxkim, 2017-05-30 20:54:20]

Committed patchset #3 (id:60001) manually as
82490f0d2f82b2ec98dfa35593289d2c51aeab63 (presubmit successful).