[webkit-reviews] review granted: [Bug 98561] Perf test pesults page takes forever to load on a machine with a slow Internet connection : [Attachment 167394] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 98561: Perf test pesults page takes forever to load on a machine with a
slow Internet connection
https://bugs.webkit.org/show_bug.cgi?id=98561

Attachment 167394: Patch
https://bugs.webkit.org/attachment.cgi?id=167394&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167394&action=review


> PerformanceTests/resources/results-template.html:514
> +(function () {

You probably want to move this whole block close to the top of the file so that
we start doing the network request ASAP.

> PerformanceTests/resources/results-template.html:538
> +	       loadScript(prefix + '/' + plugins[i], function () {
> +		   numberOfLoadedPlugins++;
> +		   if (numberOfLoadedPlugins == plugins.length)
> +		       init();
> +	       }, (function (plugin) {
> +		   return function () { alert("Failed to load " + plugin); }
> +	       })(prefixes[i]));

These nested function calls are really hard to read. Can you make a local named
helper function?

> PerformanceTests/resources/results-template.html:542
> +    var prefixes = ['%AbsolutePathToWebKitTrunk%',
'https://svn.webkit.org/repository/webkit/trunk'];

Instead of trying both, could you try the local one first and only do the
network one if that fails? The local once should error out pretty quickly when
it's not available.

> PerformanceTests/resources/results-template.html:556
> +	   loadScript(prefixes[i] + '/' + jQuery, (function (prefix) {
> +	       return function () {
> +		   if (!startedLoadingPlugins)
> +		       loadPlugins(prefix);
> +		   startedLoadingPlugins = true;		
> +	       }
> +	   })(prefixes[i]), function () {
> +	       numberOfFailures++;
> +	       if (numberOfFailures == prefixes.length)
> +		   alert("Failed to load jQuery.");
> +	   });

Ditto. Hard to read. It would also help if you put the second argument to
loadScript on a newline after the comma.