Summary: | WebKit Bot Watcher's Dashboard: Teach JSON.load() to take optional failure callback function | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | Tools / Tests | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, timothy | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Daniel Bates
2014-01-30 11:06:46 PST
Created attachment 222690 [details]
Patch
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. You could also utilize the trick you did before, and assign failureCallback to a no-op function if it's not what we expect. Created attachment 222713 [details]
Patch
Updated patch according to Alexey Proskuryakov's remarks.
Committed r163196: <http://trac.webkit.org/changeset/163196> |