Bug 66166 - garden-o-matic's analyzeUnexpectedFailures needs a completion callback.
Summary: garden-o-matic's analyzeUnexpectedFailures needs a completion callback.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 64188
  Show dependency treegraph
 
Reported: 2011-08-12 14:17 PDT by Dimitri Glazkov (Google)
Modified: 2011-08-13 08:39 PDT (History)
1 user (show)

See Also:


Attachments
Patch (3.19 KB, patch)
2011-08-12 14:18 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2011-08-12 15:49 PDT, Dimitri Glazkov (Google)
no flags 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 14:17:48 PDT
garden-o-matic's analyzeUnexpectedFailures needs a completion callback.
Comment 1 Dimitri Glazkov (Google) 2011-08-12 14:18:33 PDT
Created attachment 103814 [details]
Patch
Comment 2 Adam Barth 2011-08-12 14:31:55 PDT
Comment on attachment 103814 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/main.js:292
> +        model.analyzeUnexpectedFailures(showUnexpectedFailure, function() {
> +            dismissButterbar();
> +        });

You can just pass dismissButterbar as the completion callback.  You don't need the thunk.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:126
> +    var queriesInFlight = Object.keys(unexpectedFailures).length;

please use base.RequestTracker.
Comment 3 Dimitri Glazkov (Google) 2011-08-12 15:49:01 PDT
Created attachment 103833 [details]
Patch
Comment 4 Adam Barth 2011-08-12 15:53:11 PDT
Comment on attachment 103833 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:114
> +    this._tryCallback();

Did you check the other call sites to see what they thought of this new behavior?  I seem to remember a bunch of them doing this manually.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:117
> +base.RequestTracker.prototype = {

I prefer the other style of modifying the prototype, but whatever.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:126
> +    var tracker = new base.RequestTracker(Object.keys(unexpectedFailures).length, completionCallback);

Why Object.keys and not base.keys ?  Should base.keys == Object.keys when Object.keys is available?
Comment 5 Dimitri Glazkov (Google) 2011-08-12 15:59:02 PDT
Comment on attachment 103833 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:114
>> +    this._tryCallback();
> 
> Did you check the other call sites to see what they thought of this new behavior?  I seem to remember a bunch of them doing this manually.

Yep. I did. There were only two other callsites.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:117
>> +base.RequestTracker.prototype = {
> 
> I prefer the other style of modifying the prototype, but whatever.

:)
Comment 6 Dimitri Glazkov (Google) 2011-08-13 08:39:17 PDT
Comment on attachment 103833 [details]
Patch

Clearing flags on attachment: 103833

Committed r93016: <http://trac.webkit.org/changeset/93016>
Comment 7 Dimitri Glazkov (Google) 2011-08-13 08:39:22 PDT
All reviewed patches have been landed.  Closing bug.