[webkit-reviews] review denied: [Bug 127924] WebKit Bot Watcher's Dashboard: Teach JSON.load() to take optional failure callback function : [Attachment 222690] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 30 11:35:45 PST 2014
Alexey Proskuryakov <ap at webkit.org> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 127924: WebKit Bot Watcher's Dashboard: Teach JSON.load() to take optional
failure callback function
https://bugs.webkit.org/show_bug.cgi?id=127924
Attachment 222690: Patch
https://bugs.webkit.org/attachment.cgi?id=222690&action=review
------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=222690&action=review
Looks good to me overall. I'm asking for a number of changes though, so it
would be beneficial to have another patch posted before landing.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:354
> + }.bind(this), function(data) {
Could you please split this into two lines? This looks somewhat confusing as it
is, with two distinct large functions that get to share one line of code.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/U
tilities.js:27
> +JSON.LoadError = 1 << 0;
> +JSON.ParseError = 1 << 1;
Other code in the Dashboard uses string names for everything, I don't think
that a bitmask fits the purpose well.
It's not used as a bitmask either.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/U
tilities.js:59
> + parseErrorOccurred = true;
Why not just invoke failureCallback here, and have an early return? That should
save us the need to track a state variable.
>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/U
tilities.js:63
> + failureCallback(data);
This will fail if there is no failureCallback, unlike above code that checks
for its type.
More information about the webkit-reviews
mailing list