390556 - Adds a graph option to cvsqueryform.cgi

Status: RESOLVED WONTFIX

(Webtools Graveyard :: Bonsai, enhancement)

Description

[Ben Hsieh]

Attachment: Adds a graph option to cvsqueryform.cgi

Adds a graph option to cvsqueryform.cgi

Comment 1

[Mike Taylor [:bear]]

Comment on attachment 274852 [details] [diff] [review]
Adds a graph option to cvsqueryform.cgi

this is running on landfill and the perl code looks ok.  I don't know javascript so cannot comment on it :)

I'm going to r+ it and ask cls to do a review as a second set of eye.

Comment 2

[Ben Hsieh]

Attachment: as above, but adds back-button functionality and automatically drills to requested directory

No significant change to the perl, so I hope bear would be okay with me directing this review to cls again. Just thought I'd save you guys the trouble of reviewing twice.

Comment 3

[Ben Hsieh]

Attachment: included checkinsbargraph.js this time

didn't include a new file in above patch.

Comment 4

[Ben Hsieh]

Comment on attachment 277413 [details] [diff] [review]
included checkinsbargraph.js this time

We'd like another set of eyes on the perl-side of the code, so requesting an additional review.

Comment 5

[Ben Hsieh]

Attachment: identical to above, except with license block

requesting additional review from timeless.

Comment 6

[timeless]

Comment on attachment 277948 [details] [diff] [review]
identical to above, except with license block

>+    $headerinfo .= '<style type="text/css"> a {color: blue; text-decoration: underline } </style>';

we can't use a class with an external css file? :(

>+    # this part sets up the js variables checkinsbargraph.js expects

"the js variables" => "the variables"

>+    $headerinfo .= '<script type="text/javascript">';
...
>+        for (my $f = 0; $f <= $#ci_files; $f++) {
>+            $headerinfo .= "\"" . &html_log($ci_files[$f]) . "\"";

Personally, I'd rather '"'

>+    $headerinfo .= "] \n";
>+    
please don't add trailing whitespace



>+++ webtools/bonsai/checkinsbargraph.js
>@@ -0,0 +1,318 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>

line endings seem strange for this file. please be sure whomever commits it ensures that the line endings are sane.

>+ * The Original Code is the Mozilla Community QA Extension

really?

>+function handleLoad(parentName, firstDirectory) {
>
>+    if (parentName == undefined) parentName = rootName;

personally i prefer splitting if (cond) from statement;
debuggers/pretty printing is a risk.

>+    

please don't include trailing whitespace.

>+    if (window.location.hash.length > 0) handleHistory();      // if the user got here from a refresh or bookmark, take him back where he was

stick the comment on its own line before the if (cond), and the statement() on its own line after.

>+    else if (firstDirectory) graphContents(firstDirectory, dirCompareByName);
>
>+    else if (dir != "") graphContents(rootName + "/" + dir, dirCompareByName);
>
>+    else graphContents(parentName, dirCompareByName);

Personally, I'd stick each statement into a { block(); }.

>+var factor = 1/20;

0.05; ? Asking the compiler or something to do math here seem silly.

>+function graphContents(parentName, sortFunction) {
>+    makeHistory(parentName, sortFunction);
>+	changeDirectory(parentName.split('/'));

bad indentation

>+	var outputText = makeBreadCrumbs() + "<br /><br /><table BORDER=0 style='font-size:11px'><th style='font-size:14px'><a style = 'color: blue; text-decoration: underline;'"

inline style is evil, how about a class? also, don't stick spaces around =. But yes, a class would be a very good thing.

Sure bonsai/tinderbox are old and ugly, but that doesn't mean you have to make them worse.

>+		+ "onclick='graphContents(\"" + parentName + "\", dirCompareByName);'>Module</a></th>"

tabs. bad. no. begone.

don't recalculate 500/(upperEdge*2). make a local and use it twice:
>+	var hScaling = (500 / (upperEdge * 2) > 20) ? 20 : 500 / (upperEdge * 2);

>+        if (navigator.appName == "Netscape") {
>+            barMaxWidth = window.innerWidth - 110;
>+        } else if (navigator.appName.indexOf("Microsoft")!=-1) {
>+            barMaxWidth = document.body.offsetWidth - 110;

What happens for Opera/Safari? (this isn't a show stopper, I just want to know).

Stick comments before code, not way to the right:
>+	curRoot = root;                                  // start from the top
>+	//fullPath.shift();                                // get rid of top-level

>+				alert("File not found: " + flattenArray(fullPath));

please don't.

>+function makeModuleLink(file) {
>+	var RV = "<a onclick='resetGraphContents(\"" + file.fullPath + file.name + "\", dirCompareByName)'>"

don't use 'RV', call it "link" or something.
>+	return RV;

>+function resetGraphContents(parentName, sortFunction) {
>+	nameSort = countSort = -1;

it'd be kinda nice if these were gNameSort/gCountSort to make it clear that they're globals.

>+function makeBar(count, maxWidth, hScaling, lowerEdge, upperEdge, colorScaling) {

hScaling doesn't match conventions:
1. I can't figure out what it means
2. single letter prefix has a specific style it usually indicates a type (konstant, global, argument)

>+function makeBreadCrumbs() {
>+		+ curpath + "\", dirCompareByName);'>" + levels[i] + "</a><span style='font-size:11px'> > </span>";

please use &gt; when you want a bare > to appear in content.


>+function makeBonsaiLink(file) {
>
>+    var RV = "<a href = \"";

var link = '<a href="';

that's the last time i repeat a complaint (or two).

>+        var dirindex = fileURLtemplate.indexOf("subdir=");
>+        var fileindex = fileURLtemplate.indexOf("files=");
>+        RV += fileURLtemplate.substring(0, dirindex + 7) + realPath
>+                + fileURLtemplate.substring(dirindex + 7, fileindex + 6) + file.name
>+                + fileURLtemplate.substring(fileindex + 6);

this looks like a regexp would serve nicely.

>+// note: is this how you do OOP in jscript? this is the devmo model.

yes. and remove the note.

>+// @params: name of directory, full string path to file (not including self)

to "file", isn't this a directory?

>+function Directory(name, stringFullPath) {

name your functions:
>+Directory.prototype.categorizeArray = function(strArray)


>+	this.count++;

personally, ++this.count; but...

>+    if (this.subdirs[folder] == null) {

personally, if (!this.subdirs[folder]) {

>+Directory.prototype.isFile = function()
open and close braces on their own lines:
>+{	return this.sortedSubdirs.length == 0; }

>+// shoves the objects in subdirs into sortedSubdirs in random (but indexed) order.

boy you make it sound so crude.

>+Directory.prototype.recPopulateSortedSubdirs = function()

>+/** Sorting functions for Directories **/

we try to encourage java doc style comments:
/**
 * Sorting functions for Directories
 */

>+var nameSort = -1;  // invert each time

comment doesn't make sense.
>
>+function dirCompareByName(obj1, obj2) {
>+function dirCompareByCount(obj1, obj2) {

one could be evil and use a function constructor to share this code, but it doesn't seem worth it :(.

>+function flattenArray(array, length) {
>+    var max = (length == undefined) ? array.length : length;
>+    for (var i = 0; i < max; i++) {
>+        RV += array[i] + "/";

this seems unfortunate.
I wonder if:
 x=[].concat(array);if (length != undefined) x.length = length; return x.join ('/');
would be better. ask neil?

>+function colorSwitch(size, lowerEdge, upperEdge, scaling) {

local variable for 255 / scaling?
stick a space before ?
>+	distFromLower = (size < lowerEdge)? 0 : parseInt((size - lowerEdge) / scaling * 255);
>+	distFromUpper = (size > upperEdge)? 0 : parseInt((upperEdge - size) / scaling * 255);

>+	return "#"  + parseHex(distFromLower) + parseHex(distFromUpper) + "00";

>+function parseHex(d) {

I don't like the name. You aren't really parsing a hex, you're making one.

>+    var string = d.toString(16);
>
>+    if (string.length < 2) string = "0" + string;

neil has a cleverer way of dealing w/ this.

I think it's something like:
var string = ("0" + d.toString).substr(-2, 2);

>+            var hashParts = expectedHash.substring(1).split(delim);

please explain.

If you can address my comments to another reviewer's satisfaction (e.g. reed), I'm fine with this.

I expect to be busy next week preparing for my month long vacation.

Comment 7

[Ben Hsieh]

Attachment: v2

Reed, please note Timeless's comments above.

In some places I left \" instead of '"', in particular whenever both types of quotes are being used anyways, for something like

> element.innerHTML = "onclick=\"doThing('stringparameter')\""

Otherwise, hopefully I caught everything.

Comment 8

[Reed Loden [:reed]]

Comment on attachment 278888 [details] [diff] [review]
v2

timeless is still around, so if he has time, I'd rather him do it. :)

Comment 9

[timeless]

Comment on attachment 278888 [details] [diff] [review]
v2

yeah, I wasn't expecting you to change it for the mixed cases.

really stupid nit:
-if( !$show_xml ) {
+if( !$show_xml && 
!$show_graph) {

this is now if_nospace_(_space_condition_nospace_)

which is ungood. I'd rather you make it match the one below:

-if( !$show_raw && !$show_xml ) {
+if ($show_graph) {

which is obviously:
if_space_(_nospace_condition_nospace_)

(so yes, add the space before the paren and remove the one after it)

+        my $j = $k+1;

I think I use spaces around +

+    if (sortFunction == dirCompareByName) gNameSort *= -1;
+    else if (sortFunction == dirCompareByCount) gCountSort *= -1;

you didn't deal w/ my if comment here. my comments are style not local.

everything else looks much better (except the line endings for new files, and reed promises to fix that)

Comment 10

[Reed Loden [:reed]]

Comment on attachment 278888 [details] [diff] [review]
v2

Ben, please address the last of timeless's comments and submit a new patch (with r+ carried over), and I'll be glad to check it in for you.

Comment 11

[Ben Hsieh]

Attachment: v3

Comment 12

[Ben Hsieh]

Attachment: v4

fixed license block to be MPL-only

Comment 13

[Reed Loden [:reed]]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bonsai/CGI.pl,v  <--  CGI.pl
new revision: 1.25; previous revision: 1.24
done
RCS file: /cvsroot/mozilla/webtools/bonsai/bargraph.css,v
done
Checking in bargraph.css;
/cvsroot/mozilla/webtools/bonsai/bargraph.css,v  <--  bargraph.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bonsai/checkinsbargraph.js,v
done
Checking in checkinsbargraph.js;
/cvsroot/mozilla/webtools/bonsai/checkinsbargraph.js,v  <--  checkinsbargraph.js
initial revision: 1.1
done
Checking in cvsquery.cgi;
/cvsroot/mozilla/webtools/bonsai/cvsquery.cgi,v  <--  cvsquery.cgi
new revision: 1.66; previous revision: 1.65
done
Checking in cvsqueryform.cgi;
/cvsroot/mozilla/webtools/bonsai/cvsqueryform.cgi,v  <--  cvsqueryform.cgi
new revision: 1.29; previous revision: 1.28
done

Comment 14

[Rob Campbell [:rc] (:robcee)]

sweet! when's this likely to make it over to the main bonsai page?

Comment 15

[Reed Loden [:reed]]

(In reply to comment #14)
> sweet! when's this likely to make it over to the main bonsai page?

That depends on justdave getting bonsai-stage updated, making sure it works fine, and then updating production. Time to completion for upgrades is usually weeks to months if there isn't some extremely compelling reason to upgrade. However, I have been poking him lately to do a Bonsai upgrade, as I have some stuff I want to see pushed live, so maybe it will be sooner rather than later.

Comment 16

[Ben Hsieh]

On bonsai-stage, the Graph output option was added, but it doesn't work (produces empty output except for header), probably because the script that's supposed to produce the graph, checkinsbargraph.js, isn't on the server so far as I can tell.

Seems relevant that http://bonsai-stage.mozilla.org/checkinsbargraph.js returns a 404.

I was wondering if one of you could get that onto stage. It looks like it's in the bonsai source, but somehow didn't get pushed?

Comment 17

[Reed Loden [:reed]]

This bug is fixed. You can file an IT request to have bonsai-stage updated, or just  poke justdave on IRC to do it.

Comment 18

[Dave Miller [:justdave]]

bonsai-stage is running cvs tip.  if it's not there, it's not fixed in cvs yet.

Comment 19

[Dave Miller [:justdave]]

(I just ran |cvs -q up -AdP| and it picked up nothing)

Comment 20

[Reed Loden [:reed]]

You need to add the files to the Makefile to be copied over to the install directory. Submit a patch for that, please.

Comment 21

[Ben Hsieh]

Attachment: Makefile patch

Patched Makefile.

Comment 22

[Reed Loden [:reed]]

Comment on attachment 282741 [details] [diff] [review]
Makefile patch

>-# Contributor(s): 
>+# Contributor(s):

Why this change?

>@@ -42,6 +42,7 @@
> 		adminmail.pl		\
> 		bonsai.gif		\
> 		bonsai.ico		\
>+		checkinsbargraph.js	\

You forgot bargraph.css, too.

Comment 23

[Ben Hsieh]

Attachment: Makefile patch v2

D'oh!

Comment 24

[Reed Loden [:reed]]

Checking in Makefile.in;
/cvsroot/mozilla/webtools/bonsai/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done

You can ask justdave to update bonsai-stage now. :)

Comment 25

[Dave Miller [:justdave]]

(In reply to comment #24)
> You can ask justdave to update bonsai-stage now. :)

Done.

Comment 26

[Reed Loden [:reed]]

I backed this out because it caused bug 455111 and because timeless and I can't seem to even get this to work at all. If you can address the issue raised in bug 455111 and show that this actually works, we'll be happy to take a look at this again.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bonsai/CGI.pl,v  <--  CGI.pl
new revision: 1.29; previous revision: 1.28
done
Checking in Makefile.in;
/cvsroot/mozilla/webtools/bonsai/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done
Removing bargraph.css;
/cvsroot/mozilla/webtools/bonsai/bargraph.css,v  <--  bargraph.css
new revision: delete; previous revision: 1.1
done
Removing checkinsbargraph.js;
/cvsroot/mozilla/webtools/bonsai/checkinsbargraph.js,v  <--  checkinsbargraph.js
new revision: delete; previous revision: 1.1
done
Checking in cvsquery.cgi;
/cvsroot/mozilla/webtools/bonsai/cvsquery.cgi,v  <--  cvsquery.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in cvsqueryform.cgi;
/cvsroot/mozilla/webtools/bonsai/cvsqueryform.cgi,v  <--  cvsqueryform.cgi
new revision: 1.31; previous revision: 1.30
done

Comment 27

[Adam Muntner [:adamm] (use NEEDINFO)]

Bonsai was decommissioned, closing all remaining bugs "wontfix"