| Summary: | bring flakiness_dashboard.html closer to webkit style | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||
| Component: | New Bugs | Assignee: | Ojan Vafai <ojan> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, dpranke, eric, tony | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Ojan Vafai
2011-07-13 12:07:00 PDT
Created attachment 100702 [details]
Patch
Comment on attachment 100702 [details] Patch 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. Comment on attachment 100702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100702&action=review >> Tools/TestResultServer/static-dashboards/dashboard_base.js:288 >> + console.log('Invalid query parameter: ' + paramsList[i]); > > parameterList ? Yeah. wanted to keep this change minimal as possible. Will be trying to fix up other files' style in followup patches. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:343 >> +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. I don't have a preference. In code-review.js and results.js we've used allcaps. I see the your new code uses k... I'll leave this for now. Happy to change it later if we decide clearly on one or the other. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:473 >> +var perBuilderFailures = {}; > > Should we prefix globals with g_ ? I think so. Done. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:549 >> + */ > > WebKit favors C++ style comments in C++ code. Yeah...was trying not to do too much in a single patch. I'll fix this before committing. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:556 >> + return 'TEXT'; > > I thought you defined constants for these things so you didn't need to have string literals? This code isn't good about it. It's this code that convinced me to insist you do it in all the new code. :) I'll fix this though. But in a followup patch since I'd be more worried about breaking things. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:818 >> + resultsObj.bugs = bugs; > > resultsObj => resultsObject done >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:835 >> +function addFallbacks(addFn, candidates, validValues) > > addFn => ?? Made it callback for now. Could still be better, but that can be a future patch. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1110 >> + // TODO(ojan): Once all the FAIL lines are removed from > > TODO => FIXME Fixed here an elsewhere. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1111 >> + // test_expectations.txt, delete all the legacyExpectationsSemantics > > Has this happened? No, and in fact, it seems likely that we'll return to only allowing FAIL and getting rid of the other failure types. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1198 >> +var webkitBugReplaceValue = bugUrlPrefix + 'webkit.org/b' + webkitBugUrlPostfix; > > This are constants, right? Fixed >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1218 >> +} > > 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! It's true. There's a lot of code/design cleanup to do in this file. I was just focusing on style for now. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.html:1275 >> +function getPathToFailureLog(testName) > > WebKit avoids get prefixes. fixed in all instances except a couple that are a bit harder to cleanup. Committed r90964: <http://trac.webkit.org/changeset/90964> |