[webkit-reviews] review denied: [Bug 66144] garden-o-matic needs a summary view with actions for each problem. : [Attachment 103784] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 66144: garden-o-matic needs a summary view with actions for each problem.
https://bugs.webkit.org/show_bug.cgi?id=66144

Attachment 103784: Patch
https://bugs.webkit.org/attachment.cgi?id=103784&action=review

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


This looks pretty cool.  It's hard to visualize what the resulting UI looks
like, but I'll see that soon.  R- for CommitIndex and XSS.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/summary.js:47
> +    findCommitByRevision: function(revision) {
> +	   return this._index[revision];
> +    }

What if 92964  isn't in this._index ?  That's common for revisions that affect
other branches.  A better API is probably to provide a range of revisions and
having this object return a list of CommitData objects in that range.

There's no need for this to be an object.  It's better if it's just a free
function in the model package.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/summary.js:50
> +var g_commitIndex = new CommitIndex(model.state);

This is really part of the model, and should be in the model package.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/summary.js:61
> +	       console.info("Could not find commit data for revision",
revision);

This isn't worth logging about.  It's a very common case.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/notifications.js:1
> +var ui = ui || {};

Copyright block?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/notifications.js:20
> +var Base = base.extends('li', {

Base seems like a very general name.  Can use a more specific name?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/notifications.js:23
> +	   this.id = 'n' + g_notificationId++;

Yuck.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/notifications.js:32
> +	   $(this).fadeOut(function()
> +	   {

We've been merging these lines.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui/notifications.js:72
> +	   this._description.innerHTML = '<a href="">' + commitData.revision +
'</a>' + commitData.title + ' ' + commitData.author + ' (' +
commitData.reviewer + ')';

This line of code contains XSS.  Please don't use innerHTML or string
concatenation to create HTML.