[webkit-reviews] review granted: [Bug 64477] bring flakiness_dashboard.html closer to webkit style : [Attachment 100702] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 64477: bring flakiness_dashboard.html closer to webkit style
https://bugs.webkit.org/show_bug.cgi?id=64477

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

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


This looks great.  Below are a bunch of nits that I saw when scrolling through.
 I only made it halfway though.  (I'm pretty sure this app is full of XSS, but
I'm not sure if you're going to worry about that at this point.)

> Tools/TestResultServer/static-dashboards/dashboard_base.js:288
> -	 console.log('Invalid query parameter: ' + params[i]);
> +	 console.log('Invalid query parameter: ' + paramsList[i]);

parameterList ?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:343
> +var BUILD_TYPES = {'DEBUG': 'DBG', 'RELEASE': 'RELEASE'};
> +var MIN_SECONDS_FOR_SLOW_TEST = 4;
> +var MIN_SECONDS_FOR_SLOW_TEST_DEBUG = 2 * MIN_SECONDS_FOR_SLOW_TEST;
> +var FAIL_RESULTS = ['IMAGE', 'IMAGE+TEXT', 'TEXT', 'SIMPLIFIED', 'MISSING'];

> +var CHUNK_SIZE = 25;
> +var MAX_RESULTS = 1500;

Do we use all-caps or kMaxResults ?  I'm not sure we have a very clearly
defined style guide for JavaScript.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:401
>	   validateParameter(currentState, key, value,
>	       function() {

I would have merged these lines.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:473
> +var perBuilderPlatformAndBuildType = {};
> +var perBuilderFailures = {};

Should we prefix globals with g_ ?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:549
> +/**
> + * Returns the expectation string for the given single character result.
> + * This string should match the expectations that are put into
> + * test_expectations.py.
> + *
> + * For example, if we start explicitly listing IMAGE result failures,
> + * this function should start returning 'IMAGE'.
> + */

WebKit favors C++ style comments in C++ code.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:556
> -	 return 'TEXT';
> +	   return 'TEXT';

I thought you defined constants for these things so you didn't need to have
string literals?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:730
>      if (html) {

prefer early return.

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:818
> +		   resultsObj.bugs = bugs;

resultsObj => resultsObject

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:835
> +function addFallbacks(addFn, candidates, validValues)

addFn => ??

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1007
> +	   // TODO: Switch to getTestResultsByBuild

TODO => FIXME

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1110
> +		   // TODO(ojan): Once all the FAIL lines are removed from

TODO => FIXME

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1111
> +		   // test_expectations.txt, delete all the
legacyExpectationsSemantics

Has this happened?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1198
> +var bugUrlPrefix = '<a href="http://';
> +var bugUrlPostfix = '/$2">$2</a> ';
> +var webkitBugUrlPostfix = '/$2">WK $2</a> ';
> +var internalBugReplaceValue = bugUrlPrefix + 'b' + bugUrlPostfix;
> +var externalBugReplaceValue = bugUrlPrefix + 'crbug.com' + bugUrlPostfix;
> +var webkitBugReplaceValue = bugUrlPrefix + 'webkit.org/b' +
webkitBugUrlPostfix;

This are constants, right?

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1218
> +function getLinkHTMLToOpenWindow(url, text)
> +{
> +    return '<div class=link onclick="window.open(\'' + url + '\')">' + text
+ '</div>';
> +}

Generally, I try to avoid building HTML by string concatenation, but that's
more than just a stylistic change.  In this case, it's especially dangerous
because you're concating strings into a JavaScript context!

> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1275
> +function getPathToFailureLog(testName)

WebKit avoids get prefixes.