Wrapping leaf nodes in non polymer elements

Wrapping leaf nodes in non polymer elements

If plan is accepted I think a first CL should contain just the support classes
while each control should live in its own CL.

[cbernaschina, 2016-06-30 23:52:41]

Description was changed from

==========
Wrapping leaf nodes in non polymer elements

If plan is accepted I think a first CL should contain just the support classes
while each control should live in its own CL.
==========

to

==========
Wrapping leaf nodes in non polymer elements

If plan is accepted I think a first CL should contain just the support classes
while each control should live in its own CL.
==========

[cbernaschina, 2016-06-30 23:52:41]

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

[Cutch, 2016-07-01 17:33:36]

little comment

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

https://codereview.chromium.org/2119733003/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/isolate_ref.dart:51:
'#/inspect?isolateId=${Uri.encodeComponent(isolate.id)}')
We should probably delegate the href building to some class that knows how
create uris for specific pages and objects.

[cbernaschina, 2016-07-07 16:24:05]

Added curly-block tests

[Cutch, 2016-07-07 19:50:57]

More comments

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/elements.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/elements.dart:63: export
'package:observatory/src/elements/curly_block.dart';
Can we group the import and export lines?

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/elements.dart:78: Future initElements() async {
Why is this function marked async? Is anything inside here asynchronous?

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/repositories.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/repositories.dart:1: // Copyright (c) 2014, the Dart
project authors.  Please see the AUTHORS file
copyright year

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/repositories.dart:9: part
'src/repositories/objects/isolate.dart';
no blank lines between parts.

please keep parts sorted by path name.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/curly_block.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/curly_block.dart:9: import
'helpers/tag.dart';
fully qualify this import with "package:..."

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/curly_block_wrapper.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/curly_block_wrapper.dart:7: import
'shims/binding.dart';
fully qualify imports with package:

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/isolate_ref_wrapper.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/isolate_ref_wrapper.dart:45:
_updatesController.add(new IsolateUpdateEvent(ref));
This pattern is going to appear often. Maybe subscribing for updates and
unsubscribing for updates could be factored into utility functions?

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/shims/binding.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/shims/binding.dart:15: const Binding
(attribute, [String property]):
space between ) and :

the following lines should be indented two more spaces

[cbernaschina, 2016-07-07 22:08:13]

Making tests work on firefox

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/elements.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/elements.dart:63: export
'package:observatory/src/elements/curly_block.dart';
On 2016/07/07 19:50:57, Cutch wrote:
> Can we group the import and export lines?

Done.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/elements.dart:78: Future initElements() async {
On 2016/07/07 19:50:57, Cutch wrote:
> Why is this function marked async? Is anything inside here asynchronous?

Even though this function does not invoke any asynchronous operation it is
marked as async to allow future backward compatible changes.

Added as comment

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/repositories.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/repositories.dart:1: // Copyright (c) 2014, the Dart
project authors.  Please see the AUTHORS file
On 2016/07/07 19:50:57, Cutch wrote:
> copyright year

Done.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/repositories.dart:9: part
'src/repositories/objects/isolate.dart';
On 2016/07/07 19:50:57, Cutch wrote:
> no blank lines between parts.
> 
> please keep parts sorted by path name.

Done.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/curly_block.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/curly_block.dart:9: import
'helpers/tag.dart';
On 2016/07/07 19:50:57, Cutch wrote:
> fully qualify this import with "package:..."

Done.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/curly_block_wrapper.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/curly_block_wrapper.dart:7: import
'shims/binding.dart';
On 2016/07/07 19:50:57, Cutch wrote:
> fully qualify imports with package:

Done.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/isolate_ref_wrapper.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/isolate_ref_wrapper.dart:45:
_updatesController.add(new IsolateUpdateEvent(ref));
On 2016/07/07 19:50:57, Cutch wrote:
> This pattern is going to appear often. Maybe subscribing for updates and
> unsubscribing for updates could be factored into utility functions?

Working on it.

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/shims/binding.dart (right):

https://codereview.chromium.org/2119733003/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/shims/binding.dart:15: const Binding
(attribute, [String property]):
On 2016/07/07 19:50:57, Cutch wrote:
> space between ) and :
> 
> the following lines should be indented two more spaces

updated to
https://www.dartlang.org/effective-dart/style/#do-format-constructor-initiali...

[rmacnak, 2016-07-11 17:54:02]

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/elements.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/elements.dart:65: // There is no
view_footer_wrapper.dart because no binding it needed.
no binding is needed

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/service.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/service.dart:17: import
'package:observatory/repositories.dart' as I;
Why 'I'?

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/css/shared.css (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/css/shared.css:359: /* TODO: fix
error-ref-wrapped to error-ref when wrapper removed */
Our TODOs are always either TODO(username) or TODO(issuenumber)

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/curly_block.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/curly_block.dart:57: assert(expanded is
bool);
These asserts are redundant with the type checks of checked mode.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/curly_block.dart:87: ..innerHtml = expanded
?
Can you use ..text = here? Setting html in dart:html goes through a sanitizer.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/curly_block_wrapper.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/curly_block_wrapper.dart:84:
print('expanded ' + getAttribute('expand'));
Slip

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/error_ref_wrapper.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/error_ref_wrapper.dart:36: void render() {
I think this is much nicer than the html templates :)

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/shims/binding.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/shims/binding.dart:20: class Binder<T> {
Assuming I've understood this, add a comment mentioning is this mechanism is
temporary to bridge between the old Polymer components and the new style.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/tests/observatory_ui/isolate_ref_test.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/tests/observatory_ui/isolate_ref_test.dart:12: main() async{
async {

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/web/main.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/web/main.dart:9: main() async{
async {

[Cutch, 2016-07-11 17:55:39]

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/error_ref_wrapper.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/error_ref_wrapper.dart:36: void render() {
On 2016/07/11 17:54:01, rmacnak wrote:
> I think this is much nicer than the html templates :)

Me too!

[cbernaschina, 2016-07-11 18:14:12]

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/elements.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/elements.dart:65: // There is no
view_footer_wrapper.dart because no binding it needed.
On 2016/07/11 17:54:01, rmacnak wrote:
> no binding is needed

Done.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/service.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/service.dart:17: import
'package:observatory/repositories.dart' as I;
On 2016/07/11 17:54:01, rmacnak wrote:
> Why 'I'?

...Interfaces..
Some of the names collide with the ones defined in this file

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/css/shared.css (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/css/shared.css:359: /* TODO: fix
error-ref-wrapped to error-ref when wrapper removed */
On 2016/07/11 17:54:01, rmacnak wrote:
> Our TODOs are always either TODO(username) or TODO(issuenumber)

Done.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/curly_block.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/curly_block.dart:57: assert(expanded is
bool);
On 2016/07/11 17:54:01, rmacnak wrote:
> These asserts are redundant with the type checks of checked mode.

Done.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/curly_block.dart:87: ..innerHtml = expanded
?
On 2016/07/11 17:54:01, rmacnak wrote:
> Can you use ..text = here? Setting html in dart:html goes through a sanitizer.

Required to add the double space before and after. With text they are trimmed to
1.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/curly_block_wrapper.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/curly_block_wrapper.dart:84:
print('expanded ' + getAttribute('expand'));
On 2016/07/11 17:54:01, rmacnak wrote:
> Slip

Done.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/error_ref_wrapper.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/error_ref_wrapper.dart:36: void render() {
On 2016/07/11 17:54:01, rmacnak wrote:
> I think this is much nicer than the html templates :)

Done.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/lib/src/elements/shims/binding.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/lib/src/elements/shims/binding.dart:20: class Binder<T> {
On 2016/07/11 17:54:01, rmacnak wrote:
> Assuming I've understood this, add a comment mentioning is this mechanism is
> temporary to bridge between the old Polymer components and the new style.

Done.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/tests/observatory_ui/isolate_ref_test.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/tests/observatory_ui/isolate_ref_test.dart:12: main() async{
On 2016/07/11 17:54:01, rmacnak wrote:
> async {

This was replaced in the last version. Going to upload it the afternoon.

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
File runtime/observatory/web/main.dart (right):

https://chromiumcodereview.appspot.com/2119733003/diff/120001/runtime/observa...
runtime/observatory/web/main.dart:9: main() async{
On 2016/07/11 17:54:01, rmacnak wrote:
> async {

Done.

[cbernaschina, 2016-07-11 21:05:37]

Forcing rendering phase to be synchronized with the next AnimationFrame.
If a component needs asynchronous computations prior to rendering can implement
AsyncRenderable instead of Renderable and return a Future that solves into a a
RenderingCallback (probably a lambda) that will be invoked at the next animation
frame.

[Cutch, 2016-07-12 16:24:36]

https://codereview.chromium.org/2119733003/diff/200001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/helpers/rendering_scheduler.dart
(right):

https://codereview.chromium.org/2119733003/diff/200001/runtime/observatory/li...
runtime/observatory/lib/src/elements/helpers/rendering_scheduler.dart:58: render
= await element.render();
just have one animationframe callback and render all elements that are dirty

https://codereview.chromium.org/2119733003/diff/200001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_ref.dart (right):

https://codereview.chromium.org/2119733003/diff/200001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_ref.dart:45: ScriptRefElement e =
document.createElement(tag.name);
script.load().then((_) => markAsDirty());

script.load().then((_) async {
  await otherThing.load();
  markAsDirty();
});

[cbernaschina, 2016-07-14 00:42:31]

Ported full nav-notify element to new rendering pipeline.