Centralized event streams

Centralized event streams

R=turnidge@google.com

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

[cbernaschina, 2016-08-03 21:43:49]

cbernaschina@google.com changed reviewers:
+ johnmccutchan@google.com, rmacnak@google.com, turnidge@google.com

[turnidge, 2016-08-04 18:16:37]

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/app/event.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/app/event.dart:14: }
One blank line between each class declaration.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart:27: _last =
value; render();
Our style is to put one statement per line.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart:30: _isolate
= value; render();
One stmt per line.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart:48: if
(_isolate == null || _last == null) return;
Our style is:

if () {
  return;
}

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/nav/vm_menu_wrapper.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/vm_menu_wrapper.dart:28: set last(bool
value) { _last = value; render(); }
One stmt per line, here and elsewhere.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/objects/event.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/objects/event.dart:3: // BSD-style license
that can be found in the LICENSE file
Do you see us eventually pushing this Event class hierarchy into
service/object.dart?

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/objects/event.dart:25: abstract class
IsolateUpdateEvent extends IsolateEvent {}
Blank line between classes please, here and elsewhere.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/objects/vm.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/objects/vm.dart:8: String get name;
Blank line.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/repositories/event.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/repositories/event.dart:29: Stream<NoneEvent>
get onNone;
NoneEvents are never sent.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:1940: List pauseBreakpoints;
List<Breakpoint>

[cbernaschina, 2016-08-04 21:00:21]

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/app/event.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/app/event.dart:14: }
On 2016/08/04 18:16:36, turnidge wrote:
> One blank line between each class declaration.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart:27: _last =
value; render();
On 2016/08/04 18:16:36, turnidge wrote:
> Our style is to put one statement per line.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart:30: _isolate
= value; render();
On 2016/08/04 18:16:36, turnidge wrote:
> One stmt per line.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/isolate_menu_wrapper.dart:48: if
(_isolate == null || _last == null) return;
On 2016/08/04 18:16:36, turnidge wrote:
> Our style is:
> 
> if () {
>   return;
> }

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/nav/vm_menu_wrapper.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/nav/vm_menu_wrapper.dart:28: set last(bool
value) { _last = value; render(); }
On 2016/08/04 18:16:36, turnidge wrote:
> One stmt per line, here and elsewhere.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/objects/event.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/objects/event.dart:3: // BSD-style license
that can be found in the LICENSE file
On 2016/08/04 18:16:36, turnidge wrote:
> Do you see us eventually pushing this Event class hierarchy into
> service/object.dart?

I think that will be possible.
I think, though, that the best solution is to separate it into two CLs.
Once this layer of abstraction is in place integrate it into the service object
will be a non breaking change for the UI.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/objects/event.dart:25: abstract class
IsolateUpdateEvent extends IsolateEvent {}
On 2016/08/04 18:16:36, turnidge wrote:
> Blank line between classes please, here and elsewhere.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/objects/vm.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/objects/vm.dart:8: String get name;
On 2016/08/04 18:16:36, turnidge wrote:
> Blank line.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/repositories/event.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/repositories/event.dart:29: Stream<NoneEvent>
get onNone;
On 2016/08/04 18:16:36, turnidge wrote:
> NoneEvents are never sent.

Done.

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2211603002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:1940: List pauseBreakpoints;
On 2016/08/04 18:16:36, turnidge wrote:
> List<Breakpoint>

Done.

[turnidge, 2016-08-04 22:43:44]

lgtm

[cbernaschina, 2016-08-05 00:11:23]

Description was changed from

==========
Centralized event streams
==========

to

==========
Centralized event streams

R=turnidge@google.com

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

[cbernaschina, 2016-08-05 00:11:25]

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