Bug 66144 – garden-o-matic needs a summary view with actions for each problem.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

garden-o-matic needs a summary view with actions for each problem.

[Dimitri Glazkov (Google)]

Created attachment 103779 [details]
Patch

[Dimitri Glazkov (Google)]

Created attachment 103784 [details]
Patch

[Adam Barth]

Comment on attachment 103784 [details]
Patch

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/scripts/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/scripts/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/scripts/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/scripts/ui/notifications.js:1
> +var ui = ui || {};

Copyright block?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/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/scripts/ui/notifications.js:23
> +        this.id = 'n' + g_notificationId++;

Yuck.

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

We've been merging these lines.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/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.

[Adam Barth]

Also, the notification package lacks tests.  :)

[Dimitri Glazkov (Google)]

Created attachment 103859 [details]
WIP: Switching computers.

[Adam Barth]

Comment on attachment 103859 [details]
WIP: Switching computers.

View in context: https://bugs.webkit.org/attachment.cgi?id=103859&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:149
> +        var keyPieces = [];
> +        forEachRevision(function(revision) {
> +            keyPieces.push(revision);
> +        });
> +        var key = keyPieces.join('+');

This is overkill, right?  All revision ranges are contiguous, which means they're characterized by newestPassingRevision and oldestFailingRevision.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:150
> +        var failure = g_failureObjects[key];

This g_failureObjects feels like an instance of a more general concept of some kind of live dictionary.  failureAnalysisByTest is another instance of the same concept.  Maybe it's best to abstract that out into base?

Basically, you want to present it with a dictionary of updated keys/values.  It should then initialize slots for the new keys (via a callback you provide) and destruct keys that are no longer present (again via a callback).  In your use case here, you probably want some sort of "update" step where we call some function with the updated keys/values to let them update.  That will move all this machination out of this layer, where it doesn't really belong.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:157
> +            g_failureObjects[key] = createObjectCallback();
> +            forEachRevision(function(revision) {
> +                var commitData = findCommitByRevision(revision);
> +                if (commitData)
> +                    failure.addCommitData(commitData);
> +            });

Rather than calling addCommitData a bunch of times, why not just have this code pass in a commitDataList all-at-once?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:168
> +        for(var key in Object.keys(g_failureObjects)) {
> +            var failureObject = g_failureObjects[key];
> +            if (!failureObjectMap[failureObject]) {
> +                failureObject.dismiss();
> +                delete g_failureObjects[key];
> +            }
> +        }

For example, all this code has nothing to do with "failure" anything.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications.js:27
> +        // FIXME: These fade in/out effects are lame.

You should feel to remove them if you think they're lame!

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications.js:80
> +        // FIXME: Set href.
> +        linkToRevision.href = '';

There's a function in the ui package that knows how to construct a hyperlink to a revision.  You'll also want it to target="_blank".

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications.js:82
> +        this._description.appendChild(document.createTextNode(commitData.title + ' ' + commitData.author + ' (' + commitData.reviewer + ')'));

What if there's no reviewer?  Also, the existing UI grays out the author/reviewer, which is something I'd like to keep.

[Adam Barth]

(Sorry for jumping on your work-in-progress patch.)

[Dimitri Glazkov (Google)]

I'll work on the live cache abstraction first.

[Dimitri Glazkov (Google)]

Created attachment 104064 [details]
Patch

[Adam Barth]

Comment on attachment 104064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104064&action=review

This looks like a great starting point.  There are a bunch if FIXMES, but we can iterate on those.  :)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications_unittests.js:58
> +    equal(suspiciousCommit.innerHTML, '<div class="description"><a href="">1</a>title author (reviewer)</div><ul class="actions"><li><button>Roll out</button></li></ul>');

I usually try to formate these strings using + and line breaks to make them (and diffs) more readable.

[Dimitri Glazkov (Google)]

Comment on attachment 104064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104064&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications_unittests.js:58
>> +    equal(suspiciousCommit.innerHTML, '<div class="description"><a href="">1</a>title author (reviewer)</div><ul class="actions"><li><button>Roll out</button></li></ul>');
> 
> I usually try to formate these strings using + and line breaks to make them (and diffs) more readable.

Yeah, it's going to get pretty ghastly quick once I start filling these in. I'll think of a better method to test/represent these.

[Dimitri Glazkov (Google)]

Committed r93152: <http://trac.webkit.org/changeset/93152>