[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]

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.