Bug 127784 - WebKit Bot Watcher's Dashboard: Add support for using HTTP authentication with Buildbot
Summary: WebKit Bot Watcher's Dashboard: Add support for using HTTP authentication wit...
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-28 09:25 PST by Daniel Bates
Modified: 2014-01-29 13:17 PST (History)
3 users (show)

See Also:


Attachments
Patch (8.30 KB, patch)
2014-01-28 09:34 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (6.74 KB, patch)
2014-01-29 12:02 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-28 09:25:52 PST
Implement support for querying a Buildbot that requires HTTP authentication.
Comment 1 Daniel Bates 2014-01-28 09:34:49 PST
Created attachment 222461 [details]
Patch
Comment 2 Alexey Proskuryakov 2014-01-28 09:58:11 PST
Comment on attachment 222461 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:44
> +        if (this.status && this.status !== 200 && options.hasOwnProperty("callbackOnLoadError") && options.callbackOnLoadError === true) {
> +            callback({error: this.statusText, errorHTTPCode: this.status});
> +            return;
> +        }

I think that we should always notify the caller of errors. This is actually needed in other code paths too, as we could implement some optimizations if we could track whether iteration data is currently loading, or the load already failed.

An additional callback argument would work. Alternatively, we could pass an object with multiple optional callback functions, e.g. JSON.load(url, { onsuccess = function(data) {...}, onparseerror=..., onloaderror=... }).
Comment 3 Daniel Bates 2014-01-29 12:02:33 PST
Created attachment 222588 [details]
Patch
Comment 4 Alexey Proskuryakov 2014-01-29 12:12:01 PST
Comment on attachment 222588 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:65
> +    get needsAuthentication()
> +    {
> +        return this.buildbot.needsAuthentication;
> +    },

I'd just put this check inline - we don't need to create a concept of a queue that needs authentication, it's the server that does.
Comment 5 Daniel Bates 2014-01-29 12:13:07 PST
(In reply to comment #2)
> (From update of attachment 222461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222461&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:44
> > +        if (this.status && this.status !== 200 && options.hasOwnProperty("callbackOnLoadError") && options.callbackOnLoadError === true) {
> > +            callback({error: this.statusText, errorHTTPCode: this.status});
> > +            return;
> > +        }
> 
> I think that we should always notify the caller of errors. This is actually needed in other code paths too, as we could implement some optimizations if we could track whether iteration data is currently loading, or the load already failed.
> 
> An additional callback argument would work. Alternatively, we could pass an object with multiple optional callback functions, e.g. JSON.load(url, { onsuccess = function(data) {...}, onparseerror=..., onloaderror=... }).

I chose to separate into different bugs the change to teach JSON.load() to optionally send an XMLHTTPRequest with credentials from the change to avoid excessive HTTP authentication prompts. This bug is for the former and bug #127849 is for the latter, which includes adding support for a failure callback function to JSON.load().
Comment 6 Daniel Bates 2014-01-29 12:13:38 PST
(In reply to comment #4)
> (From update of attachment 222588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222588&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:65
> > +    get needsAuthentication()
> > +    {
> > +        return this.buildbot.needsAuthentication;
> > +    },
> 
> I'd just put this check inline - we don't need to create a concept of a queue that needs authentication, it's the server that does.

Will inline before landing.
Comment 7 Daniel Bates 2014-01-29 12:18:29 PST
Committed r163034: <http://trac.webkit.org/changeset/163034>
Comment 8 Daniel Bates 2014-01-29 13:17:46 PST
Fixed syntax issue when using JSONP requests and committed the fix in <http://trac.webkit.org/changeset/163040>.