Bug 99246 – [TestResultServer] Move the resource loading into a dedicated class

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

This cleans up dashboard_base.js a bit and moves all the resource loading logic into one place.

[Zan Dobersek]

Created attachment 168549 [details]
Work-in-progress patch

[Ojan Vafai]

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.

[Zan Dobersek]

Created attachment 169230 [details]
Patch

[Ojan Vafai]

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;

[Zan Dobersek]

Committed r132034: <http://trac.webkit.org/changeset/132034>

[Zan Dobersek]

(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.