[webkit-reviews] review granted: [Bug 64300] TestFailures page's new-bug-filing code is a mess! : [Attachment 100373] This time with unit tests!

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Adam Roben (:aroben)
<aroben at apple.com>'s request for review:
Bug 64300: TestFailures page's new-bug-filing code is a mess!
https://bugs.webkit.org/show_bug.cgi?id=64300

Attachment 100373: This time with unit tests!
https://bugs.webkit.org/attachment.cgi?id=100373&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100373&action=review


>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBug
Form.js:35
> +	   var formData = {
> +	       product: 'WebKit',
> +	       version: '528+ (Nightly build)',
> +	   };

This seems look odd data to hard-code here.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBug
Form.js:62
> +	   var form = document.createElement('form');
> +	   form.method = 'POST';
> +	   form.action = this._bugzilla.baseURL + 'enter_bug.cgi';
> +
> +	   for (var key in formData) {
> +	       var input = document.createElement('input');
> +	       input.type = 'hidden';
> +	       input.name = key;
> +	       input.value = formData[key];
> +	       form.appendChild(input);
> +	   }

This whole function is something you can do in like one line of jQuery, for
whatever that's worth.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBug
Form.js:73
> +    set component(x) {
> +	   this._component = x;
> +    },
> +
> +    get component() {
> +	   return this._component;
> +    },

Woah, is this really needed?  Why not just expose a real property?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFa
ilureBugForm.js:35
> +    this.component = 'Tools / Tests';

Ojan would tell you to store all these string as constants and reference them
with a symbol.	That has two benefits:

1) If you typo the string, you get a louder error.
2) If we change the components in bugzilla, it's easier to fix all the code.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFa
ilureBugForm.js:48
> +	   var form = NewBugForm.prototype.domElement.call(this);

Oh man, you're doing JavaScript inheritance manually.  Ok.  That gets really
ugly really fast.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFa
ilureBugForm_unittests.js:44
> +    var mockTrac = {};
> +    mockTrac.changesetURL = function(revisionNumber) {
> +	   return '[CHANGESET URL r' + revisionNumber + ']';
> +    }
> +    mockTrac.logURL = function(path, startRevision, endRevision) {
> +	   return '[LOG URL ' + path + ', r' + startRevision + ', r' +
endRevision + ']';
> +    }

You'll eventually want to put these in a place where more folks can use them,
but you can do that in the future when needed.