[webkit-reviews] review granted: [Bug 66245] garden-o-matic frontend needs a generic way to track updates. : [Attachment 103941] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 15 13:24:49 PDT 2011
Adam Barth <abarth at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 66245: garden-o-matic frontend needs a generic way to track updates.
https://bugs.webkit.org/show_bug.cgi?id=66245
Attachment 103941: Patch
https://bugs.webkit.org/attachment.cgi?id=103941&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103941&action=review
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.j
s:271
> + thisObject = thisObject || {};
You and MarkM have been talking!
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.j
s:274
> + callback.call(thisObject || item, item, key,
!!this._updated[key]);
How can thisObject be falsy here? It was falsy, it would be replaced with {}
above.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.j
s:278
> + removeCallback = removeCallback || function() {}
Missing ;
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.j
s:280
> + if (!updated) {
I'd use early return.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.j
s:281
> + removeCallback.call(thisObject || item, item);
I'd make this match forEach, whichever way you decided to make forEach work.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_u
nittests.js:279
> + dict.purge(function() {
> + removeCount++;
> + });
I'd add one test with purge that uses the thisObject parameter.
More information about the webkit-reviews
mailing list