Bug 99246

Summary: [TestResultServer] Move the resource loading into a dedicated class
Product: WebKit Reporter: Zan Dobersek <zan>
Component: Tools / TestsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: dpranke, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Work-in-progress patch
none
Patch ojan: review+

Description Zan Dobersek 2012-10-13 01:41:58 PDT
This cleans up dashboard_base.js a bit and moves all the resource loading logic into one place.
Comment 1 Zan Dobersek 2012-10-13 01:47:02 PDT
Created attachment 168549 [details]
Work-in-progress patch
Comment 2 Ojan Vafai 2012-10-13 09:49:50 PDT
Comment on attachment 168549 [details]
Work-in-progress patch

I haven't looked closely at this patch since it's not up for review, but at a high-level this looks great! It would be especially great if you could find a way to write tests for the new class (e.g. by stubbing out the bits that do the actual network requests).

A couple thoughts:
1. Could you wrap all the code in resourceLoader.js in an anonymous function, add a namespace and just export that? See http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/results.js as an example. Maybe the namespace could be "loader"? 
2. Name the file whatever the namespace name is, so loader.js if we choose "loader" for the namespace for consistency with garden-o-matic's naming style. I'm trying to get the flakiness dashboard to be more consistent in style with garden-o-matic.
3. Also for consistency with garden-o-matic, lets set the whole prototype to a new object at once. See base.RequestTracker http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/base.js#L192 as an example.

Thanks for working on this! I'm very excited to see this code improved.
Comment 3 Zan Dobersek 2012-10-17 12:00:05 PDT
Created attachment 169230 [details]
Patch
Comment 4 Ojan Vafai 2012-10-20 10:58:10 PDT
Comment on attachment 169230 [details]
Patch

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

<3 this change so much. Thanks for doing this!

> Tools/TestResultServer/static-dashboards/dashboard_base.js:547
> +function resourceLoadingComplete()
>  {
> -    if(!haveJsonFilesLoaded())
> -        return;
> +    g_resourceLoader = null;
> +    handleLocationChange();
> +}
>  
> +function handleLocationChange()
> +{
>      addBuilderLoadErrors();

I think this introduces a race-condition since we also call handleLocationChange on the hashchange event. So, I think you might want something like this at the beginning of handleLocationChange:
if (g_resourceLoader)
    return;
Comment 5 Zan Dobersek 2012-10-22 00:44:18 PDT
Committed r132034: <http://trac.webkit.org/changeset/132034>
Comment 6 Zan Dobersek 2012-10-22 00:52:51 PDT
(In reply to comment #4)
> (From update of attachment 169230 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169230&action=review
> 
> <3 this change so much. Thanks for doing this!
> 
> > Tools/TestResultServer/static-dashboards/dashboard_base.js:547
> > +function resourceLoadingComplete()
> >  {
> > -    if(!haveJsonFilesLoaded())
> > -        return;
> > +    g_resourceLoader = null;
> > +    handleLocationChange();
> > +}
> >  
> > +function handleLocationChange()
> > +{
> >      addBuilderLoadErrors();
> 
> I think this introduces a race-condition since we also call handleLocationChange on the hashchange event. So, I think you might want something like this at the beginning of handleLocationChange:
> if (g_resourceLoader)
>     return;

Forgot about this initially, but added the early return in r132035.
http://trac.webkit.org/changeset/132035

(In reply to comment #5)
> Committed r132034: <http://trac.webkit.org/changeset/132034>

The results files in the initial patch were loaded through XHR with a JSONP callback as I was, for reasons unknown, convinced that this was the only way the data was served with the 'Access-Control-Allow-Origin: *' header. That's incorrect though and no callback is required, so I changed the code to take that into account.