Initial check-in of tryconsole.

Initial check-in of tryconsole.

BUG=None
TEST=None

[M-A Ruel, 2010-01-06 02:58:43]

Rubberstampping the javascript, it's a bit dense for me.

Please add the copyrights to the files.

I wonder if some of this could use the data directly from
http://build.chromium.org/buildbot/try-server/json?as_text=1 ?See
http://build.chromium.org/buildbot/try-server/json/help for more info. It would
be much easier that manually parsing html pages...

http://codereview.chromium.org/517046/diff/1/2
File tryconsole/app.yaml (right):

http://codereview.chromium.org/517046/diff/1/2#newcode2
tryconsole/app.yaml:2: version: 2
version 2 ? :)

http://codereview.chromium.org/517046/diff/1/2#newcode19
tryconsole/app.yaml:19: expiration: "1s"
1s for css??

http://codereview.chromium.org/517046/diff/1/12
File tryconsole/css/tryconsole.css (right):

http://codereview.chromium.org/517046/diff/1/12#newcode38
tryconsole/css/tryconsole.css:38: 
make the spacing slightly more consistent.

http://codereview.chromium.org/517046/diff/1/13
File tryconsole/index.yaml (right):

http://codereview.chromium.org/517046/diff/1/13#newcode56
tryconsole/index.yaml:56: 
remove line

http://codereview.chromium.org/517046/diff/1/17
File tryconsole/js/builds.js (right):

http://codereview.chromium.org/517046/diff/1/17#newcode10
tryconsole/js/builds.js:10: 'failure': 'red',
weird alignment

http://codereview.chromium.org/517046/diff/1/15
File tryconsole/js/http.js (right):

http://codereview.chromium.org/517046/diff/1/15#newcode5
tryconsole/js/http.js:5: function() { return new
ActiveXObject('Msxml2.XMLHTTP'); },
Really want to support IE5.5?

http://codereview.chromium.org/517046/diff/1/14
File tryconsole/tryconsole.py (right):

http://codereview.chromium.org/517046/diff/1/14#newcode1
tryconsole/tryconsole.py:1: #!/usr/bin/python2.4
GAE doesn't use 2.4 :)

[bradn, 2010-01-11 06:56:05]

Definitely good point about the json. I've added some TODO's. I had known about
the json at the time, but found some fields that were only available in the
html. However, I should use it for the things that I can, like list of builders.
I'll handle that in a follow on review. Do you think it would make sense to
alter buildbot to emit the missing fields?
Oh well, the ideal way to handle all of this is to have the master push changes
to the dashboard anyway, as you mentioned elsewhere.

I've cleaned up the javascript to match the style guide, somehow completely
forgot there was one. I'll seek out someone with js readability tomorrow to add
to the review.

http://codereview.chromium.org/517046/diff/1/2
File tryconsole/app.yaml (right):

http://codereview.chromium.org/517046/diff/1/2#newcode2
tryconsole/app.yaml:2: version: 2
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> version 2 ? :)

Had been working with more than one version deployed.
Switched back to 1.

http://codereview.chromium.org/517046/diff/1/2#newcode19
tryconsole/app.yaml:19: expiration: "1s"
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> 1s for css??

Oops debugging leftover.
Done.

http://codereview.chromium.org/517046/diff/1/12
File tryconsole/css/tryconsole.css (right):

http://codereview.chromium.org/517046/diff/1/12#newcode38
tryconsole/css/tryconsole.css:38: 
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> make the spacing slightly more consistent.

Done.

http://codereview.chromium.org/517046/diff/1/13
File tryconsole/index.yaml (right):

http://codereview.chromium.org/517046/diff/1/13#newcode56
tryconsole/index.yaml:56: 
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> remove line

Done.

http://codereview.chromium.org/517046/diff/1/17
File tryconsole/js/builds.js (right):

http://codereview.chromium.org/517046/diff/1/17#newcode10
tryconsole/js/builds.js:10: 'failure': 'red',
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> weird alignment

Done.

http://codereview.chromium.org/517046/diff/1/15
File tryconsole/js/http.js (right):

http://codereview.chromium.org/517046/diff/1/15#newcode5
tryconsole/js/http.js:5: function() { return new
ActiveXObject('Msxml2.XMLHTTP'); },
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> Really want to support IE5.5?

Fair point. :-)
Done.

http://codereview.chromium.org/517046/diff/1/14
File tryconsole/tryconsole.py (right):

http://codereview.chromium.org/517046/diff/1/14#newcode1
tryconsole/tryconsole.py:1: #!/usr/bin/python2.4
On 2010/01/06 02:58:43, Marc-Antoine Ruel wrote:
> GAE doesn't use 2.4 :)

Done.

[M-A Ruel, 2010-01-11 15:28:55]

On 2010/01/11 06:56:05, bradn wrote:
> Definitely good point about the json. I've added some TODO's. I had known
about
> the json at the time, but found some fields that were only available in the
> html. However, I should use it for the things that I can, like list of
builders.
> I'll handle that in a follow on review. Do you think it would make sense to
> alter buildbot to emit the missing fields?

Yes, the only reason there's missing values is because I didn't take the time to
write the code yet. :)


I have a few nit but lgtm, you can check-in as-is too.

http://codereview.chromium.org/517046/diff/12001/12002
File tryconsole/app.yaml (right):

http://codereview.chromium.org/517046/diff/12001/12002#newcode13
tryconsole/app.yaml:13: - url: /images
nit: Personally, I prefer to have all the static files in static/ since it's
simpler than spreading files around but I don't mind.

http://codereview.chromium.org/517046/diff/12001/12002#newcode15
tryconsole/app.yaml:15: - url: /favicon.ico
nit: I think the yaml files would be more readable if you would put a vertical
space between each entries.

http://codereview.chromium.org/517046/diff/12001/12014
File tryconsole/tryconsole.py (right):

http://codereview.chromium.org/517046/diff/12001/12014#newcode31
tryconsole/tryconsole.py:31: master_url = db.StringProperty()
nit: some of these data models members could be required=True. That would force
you to change the way you construct the objects though.

http://codereview.chromium.org/517046/diff/12001/12014#newcode46
tryconsole/tryconsole.py:46: changed_at = db.DateTimeProperty()
nit: auto_now_add=True or auto_now=True, depending on each DateTimeProperty.

http://codereview.chromium.org/517046/diff/12001/12014#newcode198
tryconsole/tryconsole.py:198: q = db.GqlQuery('SELECT * FROM BuildResult '
nit: you could use shorthand:
q= BuildResult.gql('WHERE master_url=:1 and '
                   'builder_name=:2 and '
                   'build_number=:3', master_url, builder_name, build_number)

and equivalent for all you db.GqlQuery(...)

http://codereview.chromium.org/517046/diff/12001/12014#newcode684
tryconsole/tryconsole.py:684: application = webapp.WSGIApplication([
Move APPLICATION outside the main(), it'll speed up the app.

[bradn, 2010-01-12 00:20:00]

http://codereview.chromium.org/517046/diff/12001/12002
File tryconsole/app.yaml (right):

http://codereview.chromium.org/517046/diff/12001/12002#newcode13
tryconsole/app.yaml:13: - url: /images
On 2010/01/11 15:28:55, Marc-Antoine Ruel wrote:
> nit: Personally, I prefer to have all the static files in static/ since it's
> simpler than spreading files around but I don't mind.

Done.

http://codereview.chromium.org/517046/diff/12001/12002#newcode15
tryconsole/app.yaml:15: - url: /favicon.ico
On 2010/01/11 15:28:55, Marc-Antoine Ruel wrote:
> nit: I think the yaml files would be more readable if you would put a vertical
> space between each entries.

Done.

http://codereview.chromium.org/517046/diff/12001/12014
File tryconsole/tryconsole.py (right):

http://codereview.chromium.org/517046/diff/12001/12014#newcode31
tryconsole/tryconsole.py:31: master_url = db.StringProperty()
On 2010/01/11 15:28:55, Marc-Antoine Ruel wrote:
> nit: some of these data models members could be required=True. That would
force
> you to change the way you construct the objects though.

Fair point. Though currently that would just be master_url, builder_name,
build_number. I'll add a TODO for now.

http://codereview.chromium.org/517046/diff/12001/12014#newcode46
tryconsole/tryconsole.py:46: changed_at = db.DateTimeProperty()
On 2010/01/11 15:28:55, Marc-Antoine Ruel wrote:
> nit: auto_now_add=True or auto_now=True, depending on each DateTimeProperty.

I did that originally. But to my distress, I found that datatime.datetime.now()
seems to return a different time value than what the datastore uses.

http://codereview.chromium.org/517046/diff/12001/12014#newcode198
tryconsole/tryconsole.py:198: q = db.GqlQuery('SELECT * FROM BuildResult '
On 2010/01/11 15:28:55, Marc-Antoine Ruel wrote:
> nit: you could use shorthand:
> q= BuildResult.gql('WHERE master_url=:1 and '
>                    'builder_name=:2 and '
>                    'build_number=:3', master_url, builder_name, build_number)
> 
> and equivalent for all you db.GqlQuery(...)

Ok Done, except for all the __key__ queries, can't seem to find the gql
equivalent syntax for those.

http://codereview.chromium.org/517046/diff/12001/12014#newcode684
tryconsole/tryconsole.py:684: application = webapp.WSGIApplication([
On 2010/01/11 15:28:55, Marc-Antoine Ruel wrote:
> Move APPLICATION outside the main(), it'll speed up the app.

Done.

[bradn, 2010-01-12 00:20:33]

Adding arv for javascript review.

[arv (Not doing code reviews), 2010-01-14 04:32:47]

http://codereview.chromium.org/517046/diff/14001/14015
File tryconsole/static/builds.js (right):

http://codereview.chromium.org/517046/diff/14001/14015#newcode12
tryconsole/static/builds.js:12: var Builds = {};
Only class names and enums names should be capitalized.

http://codereview.chromium.org/517046/diff/14001/14015#newcode26
tryconsole/static/builds.js:26: }
missing semicolon

http://codereview.chromium.org/517046/diff/14001/14015#newcode44
tryconsole/static/builds.js:44: * @return {list of dicts} A list of build names
and links.
Use Array.<Object> or more strictly

!Array.<!Object>

!T means non nullable T. 
T? means nullable T

string, number, boolean, undefined are non nullable by default. Everything else
(Object and derived classes) are nullable by default.

http://codereview.chromium.org/517046/diff/14001/14015#newcode53
tryconsole/static/builds.js:53: 'link': mr[1],
No trailing commas in objects and arrays.

http://codereview.chromium.org/517046/diff/14001/14015#newcode75
tryconsole/static/builds.js:75: var color = Builds.BUILD_COLOR_MAP_[m[1]];
It is valid to put the var here but it is a bit confusing. It is usually better
to move it up to the common block.

for (var i...) is the exception where it is more common to redeclare the var.

http://codereview.chromium.org/517046/diff/14001/14015#newcode82
tryconsole/static/builds.js:82: 'time': parseInt(m[5]),
Always provide the radix for parseInt since a leading 0 makes it parse as octal.

http://codereview.chromium.org/517046/diff/14001/14015#newcode100
tryconsole/static/builds.js:100: for (var s in steps) {
Do not use for in loops for arrays. See style guide why it is bad.

http://codereview.chromium.org/517046/diff/14001/14015#newcode111
tryconsole/static/builds.js:111: Builds.redrawUpdate = function(id, limitId,
data) {
Missing jsdoc for data

http://codereview.chromium.org/517046/diff/14001/14015#newcode184
tryconsole/static/builds.js:184: b['status'] != 'running') return -1;
I find this confusing. Consider using {} when the if expression spans multiple
lines

http://codereview.chromium.org/517046/diff/14001/14015#newcode196
tryconsole/static/builds.js:196: html += '<dl class="job">';
move up to previous line

http://codereview.chromium.org/517046/diff/14001/14015#newcode214
tryconsole/static/builds.js:214: html += '</dt>';
html += ... +
    ... +
    ...;

http://codereview.chromium.org/517046/diff/14001/14015#newcode215
tryconsole/static/builds.js:215: html += ('<dd class="' + buildi['status'] +
'">');
useless parentheses

http://codereview.chromium.org/517046/diff/14001/14015#newcode267
tryconsole/static/builds.js:267: * @param {dict} data Dictionary containing
information on all builds.
Invalid type

http://codereview.chromium.org/517046/diff/14001/14013
File tryconsole/static/http.js (right):

http://codereview.chromium.org/517046/diff/14001/14013#newcode20
tryconsole/static/http.js:20: function() { return new XMLHttpRequest(); },
Do you really need to support IE6 for the try console. I think you are better of
always using XMLHttpRequest.

http://codereview.chromium.org/517046/diff/14001/14007
File tryconsole/templates/status.html (right):

http://codereview.chromium.org/517046/diff/14001/14007#newcode1
tryconsole/templates/status.html:1: <html>
Missing doctype

<!DOCTYPE html>

http://codereview.chromium.org/517046/diff/14001/14007#newcode6
tryconsole/templates/status.html:6: <body onload="document.f.limit1.focus()">
yuk, DOM0. 

How about using the autofocus attribute on limit1

<input autofocus>

http://codereview.chromium.org/517046/diff/14001/14007#newcode7
tryconsole/templates/status.html:7: <script type="text/javascript"
src="/static/http.js"></script>
useless attribute. type is optional in html5

http://codereview.chromium.org/517046/diff/14001/14007#newcode11
tryconsole/templates/status.html:11: <img id="perf1" width="400" height="150">
Use CSS

http://codereview.chromium.org/517046/diff/14001/14007#newcode15
tryconsole/templates/status.html:15: <form id="f" name="f">
Do you really need both id and name?

http://codereview.chromium.org/517046/diff/14001/14007#newcode16
tryconsole/templates/status.html:16: Limit to: <input id="limit1" name="limit1"
width="30">
Use style width

http://codereview.chromium.org/517046/diff/14001/14007#newcode16
tryconsole/templates/status.html:16: Limit to: <input id="limit1" name="limit1"
width="30">
Do you really need both id and name?

[bradn, 2010-01-22 03:02:55]

Ok have another look.

http://codereview.chromium.org/517046/diff/14001/14015
File tryconsole/static/builds.js (right):

http://codereview.chromium.org/517046/diff/14001/14015#newcode12
tryconsole/static/builds.js:12: var Builds = {};
On 2010/01/14 04:32:47, arv wrote:
> Only class names and enums names should be capitalized.

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode26
tryconsole/static/builds.js:26: }
On 2010/01/14 04:32:47, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode44
tryconsole/static/builds.js:44: * @return {list of dicts} A list of build names
and links.
On 2010/01/14 04:32:47, arv wrote:
> Use Array.<Object> or more strictly
> 
> !Array.<!Object>
> 
> !T means non nullable T. 
> T? means nullable T
> 
> string, number, boolean, undefined are non nullable by default. Everything
else
> (Object and derived classes) are nullable by default.

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode44
tryconsole/static/builds.js:44: * @return {list of dicts} A list of build names
and links.
On 2010/01/14 04:32:47, arv wrote:
> Use Array.<Object> or more strictly
> 
> !Array.<!Object>
> 
> !T means non nullable T. 
> T? means nullable T
> 
> string, number, boolean, undefined are non nullable by default. Everything
else
> (Object and derived classes) are nullable by default.

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode53
tryconsole/static/builds.js:53: 'link': mr[1],
On 2010/01/14 04:32:47, arv wrote:
> No trailing commas in objects and arrays.

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode75
tryconsole/static/builds.js:75: var color = Builds.BUILD_COLOR_MAP_[m[1]];
On 2010/01/14 04:32:47, arv wrote:
> It is valid to put the var here but it is a bit confusing. It is usually
better
> to move it up to the common block.
> 
> for (var i...) is the exception where it is more common to redeclare the var.

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode82
tryconsole/static/builds.js:82: 'time': parseInt(m[5]),
On 2010/01/14 04:32:47, arv wrote:
> Always provide the radix for parseInt since a leading 0 makes it parse as
octal.

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode100
tryconsole/static/builds.js:100: for (var s in steps) {
On 2010/01/14 04:32:47, arv wrote:
> Do not use for in loops for arrays. See style guide why it is bad.

Ah good point!
Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode111
tryconsole/static/builds.js:111: Builds.redrawUpdate = function(id, limitId,
data) {
On 2010/01/14 04:32:47, arv wrote:
> Missing jsdoc for data

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode184
tryconsole/static/builds.js:184: b['status'] != 'running') return -1;
On 2010/01/14 04:32:47, arv wrote:
> I find this confusing. Consider using {} when the if expression spans multiple
> lines

Clarified with variables... I hope.

http://codereview.chromium.org/517046/diff/14001/14015#newcode196
tryconsole/static/builds.js:196: html += '<dl class="job">';
On 2010/01/14 04:32:47, arv wrote:
> move up to previous line

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode214
tryconsole/static/builds.js:214: html += '</dt>';
On 2010/01/14 04:32:47, arv wrote:
> html += ... +
>     ... +
>     ...;
>   

Done.

http://codereview.chromium.org/517046/diff/14001/14015#newcode215
tryconsole/static/builds.js:215: html += ('<dd class="' + buildi['status'] +
'">');
On 2010/01/14 04:32:47, arv wrote:
> useless parentheses

Done.

http://codereview.chromium.org/517046/diff/14001/14013
File tryconsole/static/http.js (right):

http://codereview.chromium.org/517046/diff/14001/14013#newcode20
tryconsole/static/http.js:20: function() { return new XMLHttpRequest(); },
On 2010/01/14 04:32:47, arv wrote:
> Do you really need to support IE6 for the try console. I think you are better
of
> always using XMLHttpRequest.

Done.

http://codereview.chromium.org/517046/diff/14001/14007
File tryconsole/templates/status.html (right):

http://codereview.chromium.org/517046/diff/14001/14007#newcode1
tryconsole/templates/status.html:1: <html>
On 2010/01/14 04:32:47, arv wrote:
> Missing doctype
> 
> <!DOCTYPE html>

Done.

http://codereview.chromium.org/517046/diff/14001/14007#newcode7
tryconsole/templates/status.html:7: <script type="text/javascript"
src="/static/http.js"></script>
On 2010/01/14 04:32:47, arv wrote:
> useless attribute. type is optional in html5

Done.

http://codereview.chromium.org/517046/diff/14001/14007#newcode11
tryconsole/templates/status.html:11: <img id="perf1" width="400" height="150">
On 2010/01/14 04:32:47, arv wrote:
> Use CSS
> 

Done.

http://codereview.chromium.org/517046/diff/14001/14007#newcode15
tryconsole/templates/status.html:15: <form id="f" name="f">
On 2010/01/14 04:32:47, arv wrote:
> Do you really need both id and name?

Done.

[bradn, 2010-01-22 17:03:29]

Actually hold off, I realized I forgot to fix one item.

[bradn, 2011-04-12 21:54:49]

Wow, this is long stale.
I'm closing the issue, as this had some issue scaling anyhow.
I'll like re-do at some point as a chrome extension or some such.