Bug 66144 - garden-o-matic needs a summary view with actions for each problem.
Summary: garden-o-matic needs a summary view with actions for each problem.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 66096 66245 66252 66318 66324
Blocks: 64188
  Show dependency treegraph
 
Reported: 2011-08-12 09:41 PDT by Dimitri Glazkov (Google)
Modified: 2011-08-16 13:57 PDT (History)
1 user (show)

See Also:


Attachments
Patch (11.91 KB, patch)
2011-08-12 09:45 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (11.56 KB, patch)
2011-08-12 11:12 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
WIP: Switching computers. (14.56 KB, patch)
2011-08-13 08:30 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (20.04 KB, patch)
2011-08-16 10:22 PDT, Dimitri Glazkov (Google)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-08-12 09:41:55 PDT
garden-o-matic needs a summary view with actions for each problem.
Comment 1 Dimitri Glazkov (Google) 2011-08-12 09:45:01 PDT
Created attachment 103779 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-08-12 11:12:52 PDT
Created attachment 103784 [details]
Patch
Comment 3 Adam Barth 2011-08-12 12:22:46 PDT
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.
Comment 4 Adam Barth 2011-08-12 12:23:03 PDT
Also, the notification package lacks tests.  :)
Comment 5 Dimitri Glazkov (Google) 2011-08-13 08:30:20 PDT
Created attachment 103859 [details]
WIP: Switching computers.
Comment 6 Adam Barth 2011-08-13 16:57:56 PDT
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.
Comment 7 Adam Barth 2011-08-13 17:12:22 PDT
(Sorry for jumping on your work-in-progress patch.)
Comment 8 Dimitri Glazkov (Google) 2011-08-14 16:19:20 PDT
I'll work on the live cache abstraction first.
Comment 9 Dimitri Glazkov (Google) 2011-08-16 10:22:32 PDT
Created attachment 104064 [details]
Patch
Comment 10 Adam Barth 2011-08-16 13:46:06 PDT
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.
Comment 11 Dimitri Glazkov (Google) 2011-08-16 13:57:14 PDT
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.
Comment 12 Dimitri Glazkov (Google) 2011-08-16 13:57:37 PDT
Committed r93152: <http://trac.webkit.org/changeset/93152>