[webkit-reviews] review granted: [Bug 84642] Make garden-o-matic work for the Apple Mac port : [Attachment 147951] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 84642: Make garden-o-matic work for the Apple Mac port
https://bugs.webkit.org/show_bug.cgi?id=84642

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147951&action=review


This looks great!  A bunch of minor comments below.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/LayoutTestResultsLoader.js:67
> +	   window.console.log('fetching NRWT results ',
this._builder.resultsDirectoryURL(buildName) + 'full_results.json')

Should we put these console message behind a debug flag?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/builders.js:81
> +// FIXME: export, called by results.
> +function indexOfMostRecentCompletedBuild(individualBuilderStatus)

Usually the way we export things is with this idiom:

builders.indexOfMostRecentCompletedBuild =
function(indexOfMostRecentCompletedBuild)
...

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/builders.js:140
> +    net.get(builderInfoURL, function(builderInfo) {
> +	   callback(builderInfo);
> +    });

You can just pass callback directly to net.get.  There's no need for the extra
thunk.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/builders.js:152
> +	       selectURL += (first ? '?' : '&') + 'select=' + buildNumber;

A slightly better idiom for doing this is to build up a dictionary and then use
$.param to convert the dictionary into a query string.	That will handle the
escaping correctly in case anything funky is going on.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/builders.js:158
> +	   net.get(selectURL, function(cachedBuildsInfo) {
> +	       callback(cachedBuildsInfo);
> +	   });

Same comment about the extra thunk.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/model.js:174
> +	   // if (testName != 'editing/spelling/grammar-edit-word.html')
> +	   //	  return;

We should probably remove this before landing.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/model.js:184
> -
> +	       

This change seem spurious.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/net.js:61
> +	       // window.console.log('failed to load', url, textStatus,
errorThrown);

We should either remove this line or uncomment it and put it behind a debug
flag.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/results.js:393
> +		   continueWalk();

I always worry that this function is going to overflow its stack, but I think
we can wait for that problem to occur before worrying about it too much.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/results.js:621
> +	   net.jsonp(buildURLs[currentIndex], resultsCallback);

There's probably a way of avoiding repeating this line, but it's probably not
worthwhile.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/results.js:629
> +	   var requestsInFlight = builderNameList.length;

Sorry to have created a merge conflict for you in this file.  You should feel
free to revert my change to this function.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/results.js:643
> +		       ++requestsInFlight;

Interesting.  I'd probably change this around to use RequestTracker, but you or
I can do that in a later patch.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui.js:99
> +	   function getURLParameter(name) {
> +	       return decodeURI(
> +		   (RegExp(name + '=' +
'(.+?)(&|$)').exec(location.search)||[,null])[1]
> +	       );
> +	   }

Can you move this function to base.js?

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui.js:106
> +	   var platformSelect = document.getElementById('platform-picker');

Typically we'd say $(this, "#platform-picker") to scope this selector to this
object.  It's not super important here, though, because this UI element really
is a singleton.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui.js:108
> +	   Object.keys(config.kPlatforms).forEach(function(platformName) {

You should always call sort() after calling Object.keys so that you iterate
over the keys in a predictable order.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/script
s/ui.js:122
> +	       var url = window.location.href;
> +	       var queryIndex = url.indexOf('?');
> +	       if (queryIndex != -1)
> +		   url = url.substring(0, queryIndex);
> +	       window.location.href = url + '?platform=' +
platformSelect.selectedOptions[0]._platform;

Why not just assign to window.location.search?

location.search = '?platform=' + platformSelect.selectedOptions[0]._platform;

That also has the advantage of preserving the fragment, which we use for
remembering which tab is currently open.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/styles
/notifications.css:93
> +	   opacity: 0.2;
> +	   -webkit-transition: opacity 0.5s;

Nice idea.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/styles
/onebar.css:47
> +  float: right;
> +  margin: 8px;
> +  font-size: larger;

We've been using four space indent for CSS.