Bug 66245 – garden-o-matic frontend needs a generic way to track updates.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

garden-o-matic frontend needs a generic way to track updates.

[Dimitri Glazkov (Google)]

Created attachment 103941 [details]
Patch

[Dimitri Glazkov (Google)]

As mentioned in https://bugs.webkit.org/show_bug.cgi?id=66144#c8.

[Adam Barth]

Comment on attachment 103941 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:271
> +        thisObject = thisObject || {};

You and MarkM have been talking!

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js: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.js:278
> +        removeCallback = removeCallback || function() {}

Missing ;

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:280
> +            if (!updated) {

I'd use early return.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js: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_unittests.js:279
> +    dict.purge(function() {
> +        removeCount++;
> +    });

I'd add one test with purge that uses the thisObject parameter.

[Dimitri Glazkov (Google)]

Comment on attachment 103941 [details]
Patch

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

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

Oops. I didn't mean to leave the one at the top.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:278
>> +        removeCallback = removeCallback || function() {}
> 
> Missing ;

I blame Python.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:280
>> +            if (!updated) {
> 
> I'd use early return.

Sounds good, will change.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base.js:281
>> +                removeCallback.call(thisObject || item, item);
> 
> I'd make this match forEach, whichever way you decided to make forEach work.

ok.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/base_unittests.js:279
>> +    });
> 
> I'd add one test with purge that uses the thisObject parameter.

Will do.

[Dimitri Glazkov (Google)]

Committed r93061: <http://trac.webkit.org/changeset/93061>