Miscellaneous Observatory UI improvements

Miscellaneous Observatory UI improvements

Update cursor over expander in profiler

Ressurect json-view

Show arrow in profiler tree

Clicking on <code-ref> link in profiler doesn't expand tree

<code-ref> only shows a link for Dart code

Port script-view to use script-inset

Add function source to function view

Add counters to isolate view

Add counters to isolate summary

R=turnidge@google.com

Committed: https://code.google.com/p/dart/source/detail?r=34661

[turnidge, 2014-04-01 19:42:48]

lgtm, with comments.

Nice changes.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/app/view_model.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/app/view_model.dart:14: @observable String
expander = arrowRight;
I recommend deleting expander here and instead using a template-if on "expanded"
in the html file.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/code_ref.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/code_ref.dart:17: @observable bool
get isDartCode {
Could this predicate be put in the model object (Code) instead?

You could add:

    @observable Code code => ref;

and use "code.isDartCode" in the html file.  (Also would need to notify property
"code" when "ref" changes?).

Second choice: if for some reason using the model object directly doesn't work,
this getter could be renamed something ui-ish like "shouldShowLink" .

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart:236:
@observable void toggleExpanded(Event e, var detail, Element target) {
A comment here would make this more clear.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:84: <span
id="expand" style="cursor: pointer;">{{ row.expander }}</span>
Redundant comment from me: drop row.expander here and instead have a template-if
on "expanded".  It will keep your ui choices in the html file.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_summary.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_summary.html:50: <div
class="col-md-8">
We should stop introducing new bootstrap usage in our html files if we are
planning on dropping it soon.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_view.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_view.html:207: <div
class="col-md-8">
Ditto bootstrap usage.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/json_view.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:50: _depth--;
The downside to using a member for _depth is that it needs to be restored before
exit from the function.  I recommend making depth a function parameter instead. 
Then your recursive _printMap call looks like this:

  _printMap(v, depth + 1);

And no set/restore is required.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:86: String _indent;
I would drop the _indent member and just hard-code the tab-stop at 2 until you
need to change it.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:87: int _depth;
Delete _depth.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:88: String get
_indentForDepth => _indent * _depth;
Consider replacing with a function (_indent or _writeIndent?) that does the
indentation:

void _indent(int level) {
    const tab = '  ';  // 2 spaces.
    _buffer.write(level * tab);
}

This will change this:

  _buffer.write(_indentForDepth);

To this:

  _indent(depth);

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/script_inset.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/script_inset.dart:38: @observable
String hitsStyle(ScriptLine line) {
This function is called "hitsStyle" (hits, plural), the constants are named
"hitStyle..." (hit, singular).  Is this intentional?

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/script_inset.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/script_inset.html:25: <td
style="{{ hitsStyle(line) }}">&nbsp;</td>
For a future change: instead of injecting the styling from the .dart file, we
could use classes in the <style> section and switch between them with <template
if>.  No change needed for this review.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/script_view.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/script_view.html:27: }
Do we use any of these styles on this page?  If not, consider deleting them.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/service/object.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/service/object.dart:845: @observable int
lastTokenPos;
I see where these are initialized, but I missed where they get used.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/service/object.dart:888: // Mark first
token position.
This assumes that the line number table is provided in sorted order by lines. 
It also assumes that tokens within a line are sorted by column.

Safer would be to keep track of highest and lowest seen by (row,col) or perhaps
simpler might be to keep track of the lowest and highest tokenPos, which assumes
tokens are generated in monotonic order as we parse the program.
 
Not sure I want you to change this, just pointing this out.  Maybe a
comment/todo?

[Cutch, 2014-04-02 18:26:51]

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/app/view_model.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/app/view_model.dart:14: @observable String
expander = arrowRight;
On 2014/04/01 19:42:48, turnidge wrote:
> I recommend deleting expander here and instead using a template-if on
"expanded"
> in the html file.

I agree with you in spirit. But putting this into the template triggers voodoo
mode and I want to avoid that. I'm adding a TODO.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/code_ref.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/code_ref.dart:17: @observable bool
get isDartCode {
On 2014/04/01 19:42:48, turnidge wrote:
> Could this predicate be put in the model object (Code) instead?
> 
> You could add:
> 
>     @observable Code code => ref;
> 
> and use "code.isDartCode" in the html file.  (Also would need to notify
property
> "code" when "ref" changes?).
> 
> Second choice: if for some reason using the model object directly doesn't
work,
> this getter could be renamed something ui-ish like "shouldShowLink" .

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart:236:
@observable void toggleExpanded(Event e, var detail, Element target) {
On 2014/04/01 19:42:48, turnidge wrote:
> A comment here would make this more clear.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_summary.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_summary.html:50: <div
class="col-md-8">
On 2014/04/01 19:42:48, turnidge wrote:
> We should stop introducing new bootstrap usage in our html files if we are
> planning on dropping it soon.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/isolate_view.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/isolate_view.html:207: <div
class="col-md-8">
On 2014/04/01 19:42:48, turnidge wrote:
> Ditto bootstrap usage.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/json_view.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:50: _depth--;
On 2014/04/01 19:42:48, turnidge wrote:
> The downside to using a member for _depth is that it needs to be restored
before
> exit from the function.  I recommend making depth a function parameter
instead. 
> Then your recursive _printMap call looks like this:
> 
>   _printMap(v, depth + 1);
> 
> And no set/restore is required.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:86: String _indent;
On 2014/04/01 19:42:48, turnidge wrote:
> I would drop the _indent member and just hard-code the tab-stop at 2 until you
> need to change it.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:87: int _depth;
On 2014/04/01 19:42:48, turnidge wrote:
> Delete _depth.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/json_view.dart:88: String get
_indentForDepth => _indent * _depth;
On 2014/04/01 19:42:48, turnidge wrote:
> Consider replacing with a function (_indent or _writeIndent?) that does the
> indentation:
> 
> void _indent(int level) {
>     const tab = '  ';  // 2 spaces.
>     _buffer.write(level * tab);
> }
> 
> This will change this:
> 
>   _buffer.write(_indentForDepth);
> 
> To this:
> 
>   _indent(depth);

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/script_inset.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/script_inset.dart:38: @observable
String hitsStyle(ScriptLine line) {
On 2014/04/01 19:42:48, turnidge wrote:
> This function is called "hitsStyle" (hits, plural), the constants are named
> "hitStyle..." (hit, singular).  Is this intentional?

Not intentional and fixed.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/elements/script_view.html (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/elements/script_view.html:27: }
On 2014/04/01 19:42:48, turnidge wrote:
> Do we use any of these styles on this page?  If not, consider deleting them.

Done.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
File runtime/bin/vmservice/client/lib/src/service/object.dart (right):

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/service/object.dart:845: @observable int
lastTokenPos;
On 2014/04/01 19:42:48, turnidge wrote:
> I see where these are initialized, but I missed where they get used.

Now that <script-view> uses <script-inset> we pass these bounds to
<script-inset> an alternative would be to pass -1 or some other sentinel value.

https://codereview.chromium.org/221263002/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/service/object.dart:888: // Mark first
token position.
On 2014/04/01 19:42:48, turnidge wrote:
> This assumes that the line number table is provided in sorted order by lines. 
> It also assumes that tokens within a line are sorted by column.
> 
> Safer would be to keep track of highest and lowest seen by (row,col) or
perhaps
> simpler might be to keep track of the lowest and highest tokenPos, which
assumes
> tokens are generated in monotonic order as we parse the program.
>  
> Not sure I want you to change this, just pointing this out.  Maybe a
> comment/todo?

I've modified the code to track the minimum and maximum token positions.

[Cutch, 2014-04-02 18:30:14]

Committed patchset #2 manually as r34661 (presubmit successful).