Fix some observatory analysis issues.

Fix some observatory analysis issues.

BUG=
R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/60a8ed5d471da5676590eb6ae409bb65ae419876

[devoncarew, 2017-05-31 23:06:10]

devoncarew@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com

[devoncarew, 2017-05-31 23:06:11]

Fix a few analysis issues in runtime/observatory and update the analysis
excludes. 

Aside from unused imports, here are the remaining issues:

info • The getter 'buffer' isn't defined for the class 'Object' at
lib/service_html.dart:45:18 • (undefined_getter)
info • The getter 'offsetInBytes' isn't defined for the class 'Object' at
lib/service_html.dart:45:33 • (undefined_getter)
info • The getter 'target' isn't defined for the class 'ServiceObjectOwner' at
lib/src/elements/debugger.dart:1691:32 • (undefined_getter)
info • Declaring a class named 'Function' is deprecated at
lib/src/models/objects/function.dart:77:16 •
(deprecated_function_class_declaration)
info • The getter 'isList' isn't defined for the class 'ObjectRef' at
tests/service/get_object_store_rpc_test.dart:40:74 • (undefined_getter)

[siva, 2017-06-01 19:26:33]

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/app/page.dart (right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/app/page.dart:520: _retainingPathRepository,
why remove _objectRepository? The factory constructor of ObjectViewElement is :

  factory ObjectPoolViewElement(
      M.VM vm,
      M.IsolateRef isolate,
      M.ObjectPool pool,
      M.EventRepository events,
      M.NotificationRepository notifications,
      M.ObjectPoolRepository pools,
      M.RetainedSizeRepository retainedSizes,
      M.ReachableSizeRepository reachableSizes,
      M.InboundReferencesRepository references,
      M.RetainingPathRepository retainingPaths,
      M.ObjectRepository objects,
      {RenderingQueue queue}) {

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/native_memory_profiler.dart (right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:149: await
(isolate as S.Isolate).invokeRpc("_collectAllGarbage", {});
Not sure about this cast, according to Ryan the right fix is to have
_collectAllGarbage added to the Isolate interface.

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
File
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart
(right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart:18:
// ignore: mixin_inherits_from_not_object
Not sure what this comment here means?

[rmacnak, 2017-06-01 19:30:46]

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/instance_ref.dart (left):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/instance_ref.dart:102: await
_objects.get(_isolate, _instance.id, count: count * 2);
This parameter should instead be copied from InstanceRepository to
ObjectRepository

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/native_memory_profiler.dart (right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:147: // TODO:
Is this casting correct?
This casting will work in practice in Observatory, but it isn't correct.
collectAllGarbage should be added to M.Isolate.

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
File
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart
(right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart:18:
// ignore: mixin_inherits_from_not_object
On 2017/06/01 19:26:33, siva wrote:
> Not sure what this comment here means?

If the analyzer produces an error here, that is a bug in the analyzer.

[devoncarew, 2017-06-01 20:11:54]

Thanks for the feedback! Some responses in-line.

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/app/page.dart (right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/app/page.dart:520: _retainingPathRepository,
On 2017/06/01 19:26:32, siva wrote:
> why remove _objectRepository? The factory constructor of ObjectViewElement is
:
> 
>   factory ObjectPoolViewElement(
>       M.VM vm,
>       M.IsolateRef isolate,
>       M.ObjectPool pool,
>       M.EventRepository events,
>       M.NotificationRepository notifications,
>       M.ObjectPoolRepository pools,
>       M.RetainedSizeRepository retainedSizes,
>       M.ReachableSizeRepository reachableSizes,
>       M.InboundReferencesRepository references,
>       M.RetainingPathRepository retainingPaths,
>       M.ObjectRepository objects,
>       {RenderingQueue queue}) {

This is creating a ObjectViewElement (not a ObjectPoolViewElement - created
above). I assume that ObjectViewElement is the object type we want to create
here, but it would be good to have someone familiar with the code to double
check :)

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/instance_ref.dart (left):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/instance_ref.dart:102: await
_objects.get(_isolate, _instance.id, count: count * 2);
On 2017/06/01 19:30:46, rmacnak wrote:
> This parameter should instead be copied from InstanceRepository to
> ObjectRepository

I'm not familiar w/ this code - can you provide more details?
ObjectRepository.get doesn't have a count param, and it's not totally clear to
me what the intention here is.

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/native_memory_profiler.dart (right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:147: // TODO:
Is this casting correct?
On 2017/06/01 19:30:46, rmacnak wrote:
> This casting will work in practice in Observatory, but it isn't correct.
> collectAllGarbage should be added to M.Isolate.

The issue here was more around the isolates getter: "The getter 'isolates' isn't
defined for the class 'VMRef'". It's using the 'vm' getter here - it should
perhaps just use the _vm field instead.

(updated this to use _vm)

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:149: await
(isolate as S.Isolate).invokeRpc("_collectAllGarbage", {});
On 2017/06/01 19:26:32, siva wrote:
> Not sure about this cast, according to Ryan the right fix is to have
> _collectAllGarbage added to the Isolate interface.

The issue reported here was that M.IsolateRef doesn't define invokeRpc() -
that's defined down at the Isolate service object.

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
File
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart
(right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart:18:
// ignore: mixin_inherits_from_not_object
On 2017/06/01 19:30:46, rmacnak wrote:
> On 2017/06/01 19:26:33, siva wrote:
> > Not sure what this comment here means?
> 
> If the analyzer produces an error here, that is a bug in the analyzer.

It does report an issue; I believe it is correct in this case. Super mixins are
not enabled for this project
(https://github.com/flutter/flutter/blob/master/packages/flutter/lib/analysis_...),
which they probably shouldn't be, given that dart2js doesn't support them, and
this is largely a web app project.

[siva, 2017-06-01 21:28:07]

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/app/page.dart (right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/app/page.dart:520: _retainingPathRepository,
On 2017/06/01 20:11:54, devoncarew wrote:
> On 2017/06/01 19:26:32, siva wrote:
> > why remove _objectRepository? The factory constructor of ObjectViewElement
is
> :
> > 
> >   factory ObjectPoolViewElement(
> >       M.VM vm,
> >       M.IsolateRef isolate,
> >       M.ObjectPool pool,
> >       M.EventRepository events,
> >       M.NotificationRepository notifications,
> >       M.ObjectPoolRepository pools,
> >       M.RetainedSizeRepository retainedSizes,
> >       M.ReachableSizeRepository reachableSizes,
> >       M.InboundReferencesRepository references,
> >       M.RetainingPathRepository retainingPaths,
> >       M.ObjectRepository objects,
> >       {RenderingQueue queue}) {
> 
> This is creating a ObjectViewElement (not a ObjectPoolViewElement - created
> above). I assume that ObjectViewElement is the object type we want to create
> here, but it would be good to have someone familiar with the code to double
> check :)

Ah ok, ObjectPoolViewElement was created above.

[siva, 2017-06-02 15:33:00]

Devon, did Ryan cover most of the changes from this CL in the version he
submitted?

[devoncarew, 2017-06-05 15:31:04]

On 2017/06/02 15:33:00, siva wrote:
> Devon, did Ryan cover most of the changes from this CL in the version he
> submitted?

Well, some of them, and many of the ones that would have been issues at runtime.
There are still the updates to the analysis options file, which was out of date,
the unread but assigned locals, and smaller misc changes. Plus, one new analysis
error was introduced w/ the new Isolate method - an issue on IsolateMock; I've
updated this CL so you can see all the diffs.

[rmacnak, 2017-06-05 16:33:50]

lgtm

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
File
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart
(right):

https://codereview.chromium.org/2920603003/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/evaluate_activation_in_method_class_test.dart:18:
// ignore: mixin_inherits_from_not_object
On 2017/06/01 20:11:54, devoncarew wrote:
> On 2017/06/01 19:30:46, rmacnak wrote:
> > On 2017/06/01 19:26:33, siva wrote:
> > > Not sure what this comment here means?
> > 
> > If the analyzer produces an error here, that is a bug in the analyzer.
> 
> It does report an issue; I believe it is correct in this case. Super mixins
are
> not enabled for this project
>
(https://github.com/flutter/flutter/blob/master/packages/flutter/lib/analysis_...),
> which they probably shouldn't be, given that dart2js doesn't support them, and
> this is largely a web app project.

Mixing-in a class that extends not-Object is allowed under the Dart standard.
This unit test runs only on the VM, not dart2js.

[devoncarew, 2017-06-09 06:59:32]

Description was changed from

==========
Fix some observatory analysis issues.

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

to

==========
Fix some observatory analysis issues.

BUG=
R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/60a8ed5d471da5676590eb6ae409bb65ae419876
==========

[devoncarew, 2017-06-09 06:59:35]

Committed patchset #5 (id:80001) manually as
60a8ed5d471da5676590eb6ae409bb65ae419876 (presubmit successful).