Added new implementations for virtual containers

Added new implementations for virtual containers

R=johnmccutchan@google.com, turnidge@google.com

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

[cbernaschina, 2016-08-01 16:50:06]

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

[Cutch, 2016-08-01 17:02:36]

Some initial comments.

I'd like to see this virtual-container work be used everywhere that we are
displaying a table (allocation profile) or tree (class hiearchy, cpu profiler,
heap profile, etc).

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:67:
super.detached(); _r.disable(notify: true);
new line

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:103:
_update(e..style.display = null, _items[i], i);
don't assign in a function call.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:122:
_shifter.children.length / 4 * _itemHeight) {
please give "4" (here and elsewhere) a symbolic name.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/containers/virtual_tree.dart (right):

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_tree.dart:11: typedef
HtmlElement _CreateElementCallback(toggle({bool forward}));
don't these need to be public?

what is forward?

[turnidge, 2016-08-01 17:32:40]

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:27:
StreamSubscription _onScroollSubscription;
Typo: Scrooll -> Scroll.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:78: if
(e_i < 0) return null;
Use {}'s here and everywhere for if statements in Observatory.

if (e_i < 0) {
  return null;
}

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:79:
final i_i = _top + e_i - (_shifter.children.length / 4).floor();
Can you think of a better name than i_i?  What is this variable mean?

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:80: if
(0 <= i_i && i_i < items.length) return _items[i_i];
Here too, for example.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:97:
children = [_scroller..children = [_shifter]];
This is kind of a complex little line.  What is it doing?  Does it need to be so
tight?

[cbernaschina, 2016-08-01 17:44:45]

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:27:
StreamSubscription _onScroollSubscription;
On 2016/08/01 17:32:39, turnidge wrote:
> Typo: Scrooll -> Scroll.

Done.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:67:
super.detached(); _r.disable(notify: true);
On 2016/08/01 17:02:35, Cutch wrote:
> new line

Done.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:78: if
(e_i < 0) return null;
On 2016/08/01 17:32:39, turnidge wrote:
> Use {}'s here and everywhere for if statements in Observatory.
> 
> if (e_i < 0) {
>   return null;
> }

Done.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:79:
final i_i = _top + e_i - (_shifter.children.length / 4).floor();
On 2016/08/01 17:32:40, turnidge wrote:
> Can you think of a better name than i_i?  What is this variable mean?

item_index

done

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:80: if
(0 <= i_i && i_i < items.length) return _items[i_i];
On 2016/08/01 17:32:39, turnidge wrote:
> Here too, for example.

Done.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:97:
children = [_scroller..children = [_shifter]];
On 2016/08/01 17:32:39, turnidge wrote:
> This is kind of a complex little line.  What is it doing?  Does it need to be
so
> tight?

Changed to:
children = [
  _scroller
    ..children = [_shifter]
];

It defines/describes the DOM structure of the element

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:103:
_update(e..style.display = null, _items[i], i);
On 2016/08/01 17:02:35, Cutch wrote:
> don't assign in a function call.

Done.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:122:
_shifter.children.length / 4 * _itemHeight) {
On 2016/08/01 17:02:35, Cutch wrote:
> please give "4" (here and elsewhere) a symbolic name.

Done.

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/containers/virtual_tree.dart (right):

https://codereview.chromium.org/2203433002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/containers/virtual_tree.dart:11: typedef
HtmlElement _CreateElementCallback(toggle({bool forward}));
On 2016/08/01 17:02:36, Cutch wrote:
> don't these need to be public?
> 
> what is forward?

Made public and fixed collision in elements.dart with "hide"

[turnidge, 2016-08-01 21:13:11]

lgtm w/ 1 comment

https://codereview.chromium.org/2203433002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:83: if
(0 <= item_index && item_index < items.length){
Add space before {

[cbernaschina, 2016-08-01 21:48:12]

https://codereview.chromium.org/2203433002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:83: if
(0 <= item_index && item_index < items.length){
On 2016/08/01 21:13:10, turnidge wrote:
> Add space before {

Done.

[Cutch, 2016-08-01 23:05:54]

Some more comments

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:10:
typedef HtmlElement CreateElementCallback();
I'm worried about the typedefs in this file and the virtual tree file
conflicting. Can you prefix the callback types somehow?

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:19:
Stream<RenderedEvent<VirtualCollectionElement>> get onRendered => _r.onRendered;
long line

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:82:
final item_index = _top + el_index - (_shifter.children.length / 4).floor();
what is this "4"?

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_tree.dart (right):

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_tree.dart:11: typedef
HtmlElement CreateElementCallback(toggle({bool forward}));
s/forward/autoExpandChilNodesWithOnlyOneChild

[cbernaschina, 2016-08-01 23:17:34]

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:10:
typedef HtmlElement CreateElementCallback();
On 2016/08/01 23:05:54, Cutch wrote:
> I'm worried about the typedefs in this file and the virtual tree file
> conflicting. Can you prefix the callback types somehow?

Done.

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:19:
Stream<RenderedEvent<VirtualCollectionElement>> get onRendered => _r.onRendered;
On 2016/08/01 23:05:54, Cutch wrote:
> long line

Done.

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:82:
final item_index = _top + el_index - (_shifter.children.length / 4).floor();
On 2016/08/01 23:05:54, Cutch wrote:
> what is this "4"?

Done.

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_tree.dart (right):

https://codereview.chromium.org/2203433002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_tree.dart:11: typedef
HtmlElement CreateElementCallback(toggle({bool forward}));
On 2016/08/01 23:05:54, Cutch wrote:
> s/forward/autoExpandChilNodesWithOnlyOneChild

Done.

[Cutch, 2016-08-01 23:45:11]

LGTM w/c

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:11:
typedef void VirtualCollectionUpdateCallback(HtmlElement el, dynamic item, int
index);
long line

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/containers/virtual_tree.dart (right):

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
runtime/observatory/lib/src/elements/containers/virtual_tree.dart:12:
toggle({bool toggleSingleChildNodes, bool toggleWholeTree}));
prefix both of these with "auto"

[cbernaschina, 2016-08-01 23:50:41]

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:11:
typedef void VirtualCollectionUpdateCallback(HtmlElement el, dynamic item, int
index);
On 2016/08/01 23:45:11, Cutch wrote:
> long line

Done.

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/containers/virtual_tree.dart (right):

https://codereview.chromium.org/2203433002/diff/100001/runtime/observatory/li...
runtime/observatory/lib/src/elements/containers/virtual_tree.dart:12:
toggle({bool toggleSingleChildNodes, bool toggleWholeTree}));
On 2016/08/01 23:45:11, Cutch wrote:
> prefix both of these with "auto"

Done.

[cbernaschina, 2016-08-01 23:52:33]

Description was changed from

==========
Added new implementations for virtual containers
==========

to

==========
Added new implementations for virtual containers

R=johnmccutchan@google.com, turnidge@google.com

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

[cbernaschina, 2016-08-01 23:52:35]

Committed patchset #7 (id:120001) manually as
647d6f36033699ffda06d58685b0a0286e72628e (presubmit successful).