This cleans up dashboard_base.js a bit and moves all the resource loading logic into one place.
Created attachment 168549 [details] Work-in-progress patch
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.
Created attachment 169230 [details] Patch
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;
Committed r132034: <http://trac.webkit.org/changeset/132034>
(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.