Bug 127924 - WebKit Bot Watcher's Dashboard: Teach JSON.load() to take optional failure callback function
Summary: WebKit Bot Watcher's Dashboard: Teach JSON.load() to take optional failure ca...
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: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-30 11:06 PST by Daniel Bates
Modified: 2014-01-31 13:51 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.19 KB, patch)
2014-01-30 11:11 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2014-01-30 12:40 PST, Daniel Bates
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-01-30 11:06:46 PST
We should have JSON.load() take an optional failure callback function that is invoked whenever a load error or JSON parsing error occurs.

One use case of the optional failure callback can up in bug #127784  to check whether a load failed due to an authentication error toward avoiding excessively prompting a person for credentials.
Comment 1 Daniel Bates 2014-01-30 11:11:15 PST
Created attachment 222690 [details]
Patch
Comment 2 Alexey Proskuryakov 2014-01-30 11:33:10 PST
Comment on attachment 222690 [details]
Patch

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/BuildbotIteration.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/Utilities.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/Utilities.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/Utilities.js:63
> +            failureCallback(data);

This will fail if there is no failureCallback, unlike above code that checks for its type.
Comment 3 Alexey Proskuryakov 2014-01-30 11:33:54 PST
You could also utilize the trick you did before, and assign failureCallback to a no-op function if it's not what we expect.
Comment 4 Daniel Bates 2014-01-30 12:40:16 PST
Created attachment 222713 [details]
Patch

Updated patch according to Alexey Proskuryakov's remarks.
Comment 5 Daniel Bates 2014-01-31 13:51:53 PST
Committed r163196: <http://trac.webkit.org/changeset/163196>