Added support for "bin-ified" vmstats web app source files

Added support for "bin-ified" resources, used to link the vmstats 
web app files into the VM binary. create_resources.py takes a list
of resources and writes each as a byte array, then builds a resource
table. resources.h defines a simple lookup for these arrays, which
vmstats uses.
Committed: https://code.google.com/p/dart/source/detail?r=19569

[Tom Ball, 2013-03-05 00:26:16]

This CL isn't actually that big, as there are only two interesting changes:
resources.h and vmstats_impl.cc.

[sethladd1, 2013-03-05 15:59:37]

https://codereview.chromium.org/12377099/diff/8002/runtime/bin/vmstats/server...
File runtime/bin/vmstats/server_access.dart (right):

https://codereview.chromium.org/12377099/diff/8002/runtime/bin/vmstats/server...
runtime/bin/vmstats/server_access.dart:4: HttpRequest request = new
HttpRequest();
have you seen
http://api.dartlang.org/docs/releases/latest/dart_html/HttpRequest.html#getSt...
?

[Tom Ball, 2013-03-05 18:53:30]

https://codereview.chromium.org/12377099/diff/8002/runtime/bin/vmstats/server...
File runtime/bin/vmstats/server_access.dart (right):

https://codereview.chromium.org/12377099/diff/8002/runtime/bin/vmstats/server...
runtime/bin/vmstats/server_access.dart:4: HttpRequest request = new
HttpRequest();
On 2013/03/05 15:59:37, sethladd1 wrote:
> have you seen
>
http://api.dartlang.org/docs/releases/latest/dart_html/HttpRequest.html#getSt...
> ?

Looks very useful, but I can't find any way to get the HTTP error code on
failure, though; since getString is static, the HttpRequest it uses looks to be
hidden. Any suggestions?

[Tom Ball, 2013-03-05 21:46:39]

Used HttpRequest.getString(), which eliminated the need for server_access.dart.

[siva, 2013-03-05 22:38:33]

+srdjan

[siva, 2013-03-05 23:12:43]

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h
File runtime/bin/resources.h (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:19: *resource = entry->resource_;
ASSERT(entry->length_ > 0);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:35: static const size_t builtin_resources_count_;
Wouldn't this just be 
sizeof(builtin_resources_) / sizeof(resource_map_entry);

Maybe you can assert that to with the value put in by the generator to make sure
there were no issues during generation.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:37: static resource_map_entry* get_resource(int i) {
ASSERT(i >= 0 && i < builtin_resources_count_);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:41: static size_t get_resource_count() { return
builtin_resources_count_; }

Since all methods in here are static, you could add

  DISALLOW_ALLOCATION();
  DISALLOW_IMPLICIT_CONSTRUCTORS(Resources);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
File runtime/bin/vmstats/bargraph.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:1: part of vmstats;
Need some copyright info on top?

part of dart:vmstats; or some such qualifier?

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:22: int maxElements =
var maxElements

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:48: int y = LEGEND_Y;
var x ...
var y ...

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:50: for (int i = 0; i < _elements.length; i++)
{
for (var i = 0....

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:59: void drawGraph(var unused) {
Why do we have an unused argument here, can it be added later if there is a
need.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:62: int height = _canvas.clientHeight;
Ditto comment about use of types for local variables here and in code below.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:159: }
GraphModel(this._maxSize);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:211: }
I think this class can be written as:
class Element {
  Element(this.name, this.color);

  String name;
  String color;  // Any description ....
}

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:221: }
Sample(this._segments);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
File runtime/bin/vmstats/isolate_list.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:1: part of vmstats;
Copyright and ditto comment about adding a qualifier to vmstats, similar to how
the core library does.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:11: }
IsolateList(this._listArea);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:24: Iterator<Isolate> iterator =
model.iterator;
type not needed for local vars.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:53: parent.children.add(stack);
Ditto comment about using types for local variables.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
File runtime/bin/vmstats/models.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:1: part of vmstats;
Ditto comment.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:38: }
ModelListener(this._onUpdate, this._onFailure);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:107: Space get oldSpace => _oldSpace;
why have the fields as _ and then a set of getters,
why not just declare the fields without the _ and you get the getter
automatically,

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/vmsta...
File runtime/bin/vmstats/vmstats.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/vmsta...
runtime/bin/vmstats/vmstats.dart:1: library vmstats;
Copyright.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/vmsta...
runtime/bin/vmstats/vmstats.dart:32: _updater = new
Timer.repeating(_POLL_INTERVAL, (timer) => _isolates.update());
Ditto comment regarding types for local variables.

[srdjan, 2013-03-05 23:17:30]

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h
File runtime/bin/resources.h (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:16: for (unsigned i = 0; i < get_resource_count(); i++)
{
for (int i = 0 ...

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:18: if (!strcmp(path, entry->path_)) {
strcmp(..) == 0

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:27: typedef struct {
struct resource_map_entry {
 ...
};

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:35: static const size_t builtin_resources_count_;
s/size_t/intptr_t/ ? (also below).

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
File runtime/bin/vmstats/bargraph.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:1: part of vmstats;
Missing copyright

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:7: num scaleHeight = 0;
s/num/double/

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:9: final int SAMPLE_WIDTH = 5;
static const int (otherwise they get initialized with every constructor call).
Also below.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:19: BarGraph(CanvasElement canvas,
List<Element> elements) {
BarGraph(this._canvas, this._elements) {
....

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:55: y += 30;
Optional: name constants.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:67: scaleHeight = ((scaleHeight / 100).ceil()
* 100).toInt();
Why to toInt? Do you want to truncate or round?

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:69: num scale = height / scaleHeight;
double scale =

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:74: void drawChart(int maxHeight, num scale) {
double scale

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:83: height -= (dividerHeight ~/100) * 100;
space between / and 100.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:155: final int _MAX_LENGTH = 999999999;
static const.  Is this MAX_LENGTH or just LARGE_LENGTH?

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:157: GraphModel(int maxSize) {
GraphModel (this._maxSize);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:201: String _name;
final String _name;
final String _ color_

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:204: Element(String name, String color) {
const Element(this._name, this._color);

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:219: Sample(List<int> segments) {
this._segments

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:228: int get total {
Should this be a method instead of a getter? The body seems computation
intensive.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
File runtime/bin/vmstats/isolate_list.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:1: part of vmstats;
Add Copyright

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:7: final String CSS_HIDDEN  =
'isolate_details_hidden';
static const

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:9: IsolateList(UListElement area) {
this._area

[Tom Ball, 2013-03-06 00:34:42]

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h
File runtime/bin/resources.h (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:16: for (unsigned i = 0; i < get_resource_count(); i++)
{
On 2013/03/05 23:17:30, srdjan wrote:
> for (int i = 0 ...

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:18: if (!strcmp(path, entry->path_)) {
On 2013/03/05 23:17:30, srdjan wrote:
> strcmp(..) == 0

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:19: *resource = entry->resource_;
On 2013/03/05 23:12:44, siva wrote:
> ASSERT(entry->length_ > 0);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:27: typedef struct {
On 2013/03/05 23:17:30, srdjan wrote:
> struct resource_map_entry {
>  ...
> };

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:35: static const size_t builtin_resources_count_;
On 2013/03/05 23:17:30, srdjan wrote:
> s/size_t/intptr_t/ ? (also below).

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:35: static const size_t builtin_resources_count_;
On 2013/03/05 23:12:44, siva wrote:
> Wouldn't this just be 
> sizeof(builtin_resources_) / sizeof(resource_map_entry);
> 
> Maybe you can assert that to with the value put in by the generator to make
sure
> there were no issues during generation.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:35: static const size_t builtin_resources_count_;
On 2013/03/05 23:12:44, siva wrote:
> Wouldn't this just be 
> sizeof(builtin_resources_) / sizeof(resource_map_entry);
> 
> Maybe you can assert that to with the value put in by the generator to make
sure
> there were no issues during generation.

sizeof(builtin_resources_) won't compile because it's an incomplete type.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:37: static resource_map_entry* get_resource(int i) {
On 2013/03/05 23:12:44, siva wrote:
> ASSERT(i >= 0 && i < builtin_resources_count_);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/resources.h#n...
runtime/bin/resources.h:41: static size_t get_resource_count() { return
builtin_resources_count_; }
On 2013/03/05 23:12:44, siva wrote:
> 
> Since all methods in here are static, you could add
> 
>   DISALLOW_ALLOCATION();
>   DISALLOW_IMPLICIT_CONSTRUCTORS(Resources);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
File runtime/bin/vmstats/bargraph.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:1: part of vmstats;
On 2013/03/05 23:12:44, siva wrote:
> Need some copyright info on top?
> 
> part of dart:vmstats; or some such qualifier?

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:1: part of vmstats;
On 2013/03/05 23:17:30, srdjan wrote:
> Missing copyright

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:7: num scaleHeight = 0;
On 2013/03/05 23:17:30, srdjan wrote:
> s/num/double/

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:9: final int SAMPLE_WIDTH = 5;
On 2013/03/05 23:17:30, srdjan wrote:
> static const int (otherwise they get initialized with every constructor call).
> Also below.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:19: BarGraph(CanvasElement canvas,
List<Element> elements) {
On 2013/03/05 23:17:30, srdjan wrote:
> BarGraph(this._canvas, this._elements) {
> ....

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:22: int maxElements =
On 2013/03/05 23:12:44, siva wrote:
> var maxElements

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:48: int y = LEGEND_Y;
On 2013/03/05 23:12:44, siva wrote:
> var x ...
> var y ...

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:50: for (int i = 0; i < _elements.length; i++)
{
On 2013/03/05 23:12:44, siva wrote:
> for (var i = 0....

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:59: void drawGraph(var unused) {
On 2013/03/05 23:12:44, siva wrote:
> Why do we have an unused argument here, can it be added later if there is a
> need.

It's standard in the Observable pattern to pass the object being observed.
vmstats uses the same API, and needs to know the model it's listening to. Here
it doesn't matter, since the model is an instance variable. I changed the code
to use the parameter instead of the instance variable, so the parameter is now
used.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:62: int height = _canvas.clientHeight;
On 2013/03/05 23:12:44, siva wrote:
> Ditto comment about use of types for local variables here and in code below.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:67: scaleHeight = ((scaleHeight / 100).ceil()
* 100).toInt();
On 2013/03/05 23:17:30, srdjan wrote:
> Why to toInt? Do you want to truncate or round?

Neither, I want to display the number as an int (no decimal). I moved the
toInt() to the actual draw code, to make that more obvious.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:74: void drawChart(int maxHeight, num scale) {
On 2013/03/05 23:17:30, srdjan wrote:
> double scale

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:83: height -= (dividerHeight ~/100) * 100;
On 2013/03/05 23:17:30, srdjan wrote:
> space between / and 100.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:155: final int _MAX_LENGTH = 999999999;
On 2013/03/05 23:17:30, srdjan wrote:
> static const.  Is this MAX_LENGTH or just LARGE_LENGTH?

Changed to LARGE_LENGTH

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:157: GraphModel(int maxSize) {
On 2013/03/05 23:17:30, srdjan wrote:
> GraphModel (this._maxSize);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:159: }
On 2013/03/05 23:12:44, siva wrote:
> GraphModel(this._maxSize);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:201: String _name;
On 2013/03/05 23:17:30, srdjan wrote:
> final String _name;
> final String _ color_

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:204: Element(String name, String color) {
On 2013/03/05 23:17:30, srdjan wrote:
> const Element(this._name, this._color);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:211: }
On 2013/03/05 23:12:44, siva wrote:
> I think this class can be written as:
> class Element {
>   Element(this.name, this.color);
> 
>   String name;
>   String color;  // Any description ....
> }

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:219: Sample(List<int> segments) {
On 2013/03/05 23:17:30, srdjan wrote:
> this._segments

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:221: }
On 2013/03/05 23:12:44, siva wrote:
> Sample(this._segments);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:228: int get total {
On 2013/03/05 23:17:30, srdjan wrote:
> Should this be a method instead of a getter? The body seems computation
> intensive.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:228: int get total {
On 2013/03/05 23:17:30, srdjan wrote:
> Should this be a method instead of a getter? The body seems computation
> intensive.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
File runtime/bin/vmstats/isolate_list.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:1: part of vmstats;
On 2013/03/05 23:12:44, siva wrote:
> Copyright and ditto comment about adding a qualifier to vmstats, similar to
how
> the core library does.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:1: part of vmstats;
On 2013/03/05 23:12:44, siva wrote:
> Copyright and ditto comment about adding a qualifier to vmstats, similar to
how
> the core library does.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:7: final String CSS_HIDDEN  =
'isolate_details_hidden';
On 2013/03/05 23:17:30, srdjan wrote:
> static const

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:9: IsolateList(UListElement area) {
On 2013/03/05 23:17:30, srdjan wrote:
> this._area

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:11: }
On 2013/03/05 23:12:44, siva wrote:
> IsolateList(this._listArea);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:24: Iterator<Isolate> iterator =
model.iterator;
On 2013/03/05 23:12:44, siva wrote:
> type not needed for local vars.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/isola...
runtime/bin/vmstats/isolate_list.dart:53: parent.children.add(stack);
On 2013/03/05 23:12:44, siva wrote:
> Ditto comment about using types for local variables.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
File runtime/bin/vmstats/models.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:1: part of vmstats;
On 2013/03/05 23:12:44, siva wrote:
> Ditto comment.

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:38: }
On 2013/03/05 23:12:44, siva wrote:
> ModelListener(this._onUpdate, this._onFailure);

Done.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:107: Space get oldSpace => _oldSpace;
On 2013/03/05 23:12:44, siva wrote:
> why have the fields as _ and then a set of getters,
> why not just declare the fields without the _ and you get the getter
> automatically,

Okay, but that makes instances mutable.

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/vmsta...
File runtime/bin/vmstats/vmstats.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/vmsta...
runtime/bin/vmstats/vmstats.dart:1: library vmstats;
On 2013/03/05 23:12:44, siva wrote:
> Copyright.

Done.

[srdjan, 2013-03-06 00:43:21]

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
File runtime/bin/vmstats/models.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:107: Space get oldSpace => _oldSpace;
On 2013/03/06 00:34:43, Tom Ball wrote:
> On 2013/03/05 23:12:44, siva wrote:
> > why have the fields as _ and then a set of getters,
> > why not just declare the fields without the _ and you get the getter
> > automatically,
> 
> Okay, but that makes instances mutable.

You can make them final, that makes them immutable.

[Tom Ball, 2013-03-06 01:04:49]

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
File runtime/bin/vmstats/models.dart (right):

https://codereview.chromium.org/12377099/diff/14001/runtime/bin/vmstats/model...
runtime/bin/vmstats/models.dart:107: Space get oldSpace => _oldSpace;
On 2013/03/06 00:43:21, srdjan wrote:
> On 2013/03/06 00:34:43, Tom Ball wrote:
> > On 2013/03/05 23:12:44, siva wrote:
> > > why have the fields as _ and then a set of getters,
> > > why not just declare the fields without the _ and you get the getter
> > > automatically,
> > 
> > Okay, but that makes instances mutable.
> 
> You can make them final, that makes them immutable.

Done.

[srdjan, 2013-03-06 01:14:37]

LGTM

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/resources.h
File runtime/bin/resources.h (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/resources.h#n...
runtime/bin/resources.h:43: return builtin_resources_ + i;
&builtin_resources_[i]

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/vmsta...
File runtime/bin/vmstats/vmstats.dart (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/vmsta...
runtime/bin/vmstats/vmstats.dart:21: final int _POLL_INTERVAL = 1000;
It would be good to include unit in the name, e.g., _POLL_INTERVAL_IN_MS.

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:153: int body_len = snprintf(buffer, BUFSIZE,
s/int/intptr_t/

(also below for len).

[siva, 2013-03-06 06:32:19]

lgtm

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/bargr...
File runtime/bin/vmstats/bargraph.dart (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:61: void drawGraph(var model) {
model parameter doesn't seem to have a type is it supposed to be GraphModel?

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:150: char buffer[BUFSIZE + 1];
Do you really need 8k of stack space to write send error responses back?

https://codereview.chromium.org/12377099/diff/27002/runtime/tools/create_reso...
File runtime/tools/create_resources.py (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/tools/create_reso...
runtime/tools/create_resources.py:1: # Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
2013

[Tom Ball, 2013-03-06 17:47:17]

Thanks.

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/resources.h
File runtime/bin/resources.h (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/resources.h#n...
runtime/bin/resources.h:43: return builtin_resources_ + i;
On 2013/03/06 01:14:37, srdjan wrote:
> &builtin_resources_[i]

Done.

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/bargr...
File runtime/bin/vmstats/bargraph.dart (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/bargr...
runtime/bin/vmstats/bargraph.dart:61: void drawGraph(var model) {
On 2013/03/06 06:32:20, siva wrote:
> model parameter doesn't seem to have a type is it supposed to be GraphModel?

Yes, updated.

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/vmsta...
File runtime/bin/vmstats/vmstats.dart (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats/vmsta...
runtime/bin/vmstats/vmstats.dart:21: final int _POLL_INTERVAL = 1000;
On 2013/03/06 01:14:37, srdjan wrote:
> It would be good to include unit in the name, e.g., _POLL_INTERVAL_IN_MS.

Done.

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl.cc
File runtime/bin/vmstats_impl.cc (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:150: char buffer[BUFSIZE + 1];
On 2013/03/06 06:32:20, siva wrote:
> Do you really need 8k of stack space to write send error responses back?

Added printf function that malloc's the necessary memory.

https://codereview.chromium.org/12377099/diff/27002/runtime/bin/vmstats_impl....
runtime/bin/vmstats_impl.cc:153: int body_len = snprintf(buffer, BUFSIZE,
On 2013/03/06 01:14:37, srdjan wrote:
> s/int/intptr_t/
> 
> (also below for len).

Done.

https://codereview.chromium.org/12377099/diff/27002/runtime/tools/create_reso...
File runtime/tools/create_resources.py (right):

https://codereview.chromium.org/12377099/diff/27002/runtime/tools/create_reso...
runtime/tools/create_resources.py:1: # Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
On 2013/03/06 06:32:20, siva wrote:
> 2013

Done.

[Tom Ball, 2013-03-06 17:49:02]

Committed patchset #8 manually as r19569 (presubmit successful).