Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12094074:
Support Chrome Endure graphs in perf dashboard. (Closed)
|
Created:
7 years, 10 months ago by Dai Mikurube (NOT FULLTIME) Modified:
7 years, 10 months ago CC:
Mike Stip (use stip instead) Base URL:
https://git.chromium.org/git/chromium/tools/perf.git@master Visibility:
Public. |
DescriptionSupport Chrome Endure graphs in perf dashboard.
It also imports Chrome Endure's graphing code from
src/chrome/test/functional/perf/endure_graphs/
to
tools/perf/dashboard/ui/
with renaming some files.
BUG=chromium-os:32302
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=180837
Patch Set 1 #Patch Set 2 : updated #
Total comments: 50
Patch Set 3 : addressed the comments. #
Total comments: 2
Patch Set 4 : addressed the comments #Patch Set 5 : fixed eval #
Messages
Total messages: 17 (0 generated)
Hi Mike, Chase, Dennis and Yuki, Could you take a look? It enables Chrome Endure graphs in perf dashboard. (overview.html is not changed in this patch. It'll be changed in another patch.) It imports Yuki's graphing JavaScript code from src/chrome/test/functional/perf/endure_graphs/ almost as is. It should be landed after https://codereview.chromium.org/12087097/.
lgtm w/ nit Probably a good idea to get cmp's approval on the generate_perf.py changes as well. https://codereview.chromium.org/12094074/diff/3001/generate_perf.py File generate_perf.py (right): https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode377 generate_perf.py:377: print system_dir I think this was left in for testing, do you mind taking it out? https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode386 generate_perf.py:386: print ' ' + test_dir same
https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... File dashboard/ui/endure_js/common.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:8: * @fileoverview Common methods for performance-plotting Javascript. I'd recommend a different file name for this file. 'common' doesn't make much sense, and the content code is not that common (URL and fetch specific). I'd recommend url_util(s).js or anything better than 'common'. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:15: * @param {Function(string, string)} callback The function to invoke when the {Function(string, ?string)} since |error| can be null. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:28: var error; Better to initialize |error| with null, otherwise error becomes undefined, which is slightly different from null. null == undefined, but null !== undefined. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:43: * @return {Object} An object with properties given by the parameters specified {Object.<string, (string|number)>} https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:47: var result = new Object(); var result = {}; is the same in effect, and more often used especially for a dict. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:49: var query = window.location.search.substring(1) missing semi-colon. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:51: query = query.substring(0, query.length - 1) // Strip trailing slash. if (query.slice(-1) == '/') { query = query.slice(0, -1); } 1. string.slice() supports a negative index. 2. Always use braces {} in JavaScript. 3. Put a semi colon at the end of statement. By the way, why do you need to strip the last slash? In general, it's possible that a CGI parameter has a value ending with a slash. query is not URL path. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:55: var v = s[i].split('='); This code does not handle '=' in the value correctly. 'a=b=c'.split('=') produces ['a', 'b', 'c']. In general, values may include '=' in itself. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:57: var value = unescape(v[1]); escape/unescape are obsolete. Use encodeURIComponent/decodeURIComponent instead. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:67: result['rev'] = Math.max(result['rev'], -1); Why -1? Can rev be negative? By the way, if 'rev=abc', then parseInt('abc') => NaN, and Math.max(NaN, -1) => NaN. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:76: * @param {Object} An object containing parameters for a URL query string. {Object.<string, (string|number)>} https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:76: * @param {Object} An object containing parameters for a URL query string. Missing parameter name. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:83: if (!p) Shouldn't reach here. For "for (p in params)", p must not be null or undefined. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:85: url += sep + p + '=' + params[p]; You'd better use encodeURIComponent() for values. By the way, I usually do var key_values = []; for (key in params) { // better to sanitize key here. key_values.push(key + '=' + encodeURIComponent(params[key])); } return path + '?' + key_values.join('&'); If we had very many keys, array + join is more efficient than string concatenation, and no need to care about '?' and '&'. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... File dashboard/ui/endure_js/coordinates.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... dashboard/ui/endure_js/coordinates.js:14: * Usually people don't separate annotations (@constructor and @param) with an empty line. For example, Closure library say, /** * Create an instance of a DOM helper with a new document object. * @param {Document=} opt_document Document object to associate with this * DOM helper. * @constructor */ https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... dashboard/ui/endure_js/coordinates.js:15: * @param {Array} plotData Data that will be plotted. It is an array of lines, {Array.<Array.<Array.<number>>>>} or better use @typedef /** @typedef {Array.<number>} */ var PlotDataPoint; /** @typedef {Array.<PlotDataPoint>} */ var PlotDataLine; then, @param {Array.<PlotDataLine>} plotData ...
Dai, is the plan to remove these files from src/chrome/test/functional/perf/endure_graphs as soon as this CL lands?
On 2013/01/31 17:57:33, dennis_jeffrey wrote: > Dai, is the plan to remove these files from > src/chrome/test/functional/perf/endure_graphs as soon as this CL lands? I'd like to move the updated endure_graphs to /tools/perf/dashboard/ui finally. Maybe no need to remove soon, but duplication is not so good? We may have to remove the files here in future.
On 2013/02/01 01:25:30, Dai Mikurube wrote: > On 2013/01/31 17:57:33, dennis_jeffrey wrote: > > Dai, is the plan to remove these files from > > src/chrome/test/functional/perf/endure_graphs as soon as this CL lands? > > I'd like to move the updated endure_graphs to /tools/perf/dashboard/ui finally. > Maybe no need to remove soon, but duplication is not so good? We may have to > remove the files here in future. I agree that having duplicate files in the codebase isn't so good. I'd recommend deleting the files from src/chrome/test/functional/perf/endure_graphs as soon as they're added to the /tools/perf/dashboard folder.
On 2013/02/01 01:28:10, dennis_jeffrey wrote: > On 2013/02/01 01:25:30, Dai Mikurube wrote: > > On 2013/01/31 17:57:33, dennis_jeffrey wrote: > > > Dai, is the plan to remove these files from > > > src/chrome/test/functional/perf/endure_graphs as soon as this CL lands? > > > > I'd like to move the updated endure_graphs to /tools/perf/dashboard/ui > finally. > > Maybe no need to remove soon, but duplication is not so good? We may have to > > remove the files here in future. > > I agree that having duplicate files in the codebase isn't so good. I'd > recommend deleting the files from src/chrome/test/functional/perf/endure_graphs > as soon as they're added to the /tools/perf/dashboard folder. Do you use them in /chrome/test/functional/perf ? If so, please change the path to use them. btw, I wonder if we could apply Yuki's patch in the new place (https://codereview.chromium.org/11293043/). What do you think about the patch?
Chase, I wonder if you could take a look at generate_perf.py as Mike says. It decides the directory structure of perf dashboard. Mike, Thanks for looking! Fixed it. Yuki, Thanks for the detailed comments. I fixed some of them, but didn't fix others. I'm not sure of some unfixed parts (Dennis may know). The others are to keep it identical with the original (in dashboard/ui/js/). I think we can land it soon since it's just a copying patch from the existing code. Then, I or Dennis will start another refactoring change. What do you think? We don't merge it into the original since the perf dashboard is planned to be renewed in a few months. Because of the renewal, we're not so positive to change the code more than requisite minimum. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... File dashboard/ui/endure_js/common.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:8: * @fileoverview Common methods for performance-plotting Javascript. On 2013/01/31 10:57:11, Yuki wrote: > I'd recommend a different file name for this file. > 'common' doesn't make much sense, and the content code is not that common (URL > and fetch specific). > > I'd recommend url_util(s).js or anything better than 'common'. Because it's a fork of dashboard/ui/js/common.js, I think keeping it is better. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:15: * @param {Function(string, string)} callback The function to invoke when the On 2013/01/31 10:57:11, Yuki wrote: > {Function(string, ?string)} > since |error| can be null. Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:28: var error; On 2013/01/31 10:57:11, Yuki wrote: > Better to initialize |error| with null, otherwise error becomes undefined, which > is slightly different from null. > null == undefined, but null !== undefined. Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:43: * @return {Object} An object with properties given by the parameters specified On 2013/01/31 10:57:11, Yuki wrote: > {Object.<string, (string|number)>} Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:47: var result = new Object(); On 2013/01/31 10:57:11, Yuki wrote: > var result = {}; > is the same in effect, and more often used especially for a dict. Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:49: var query = window.location.search.substring(1) On 2013/01/31 10:57:11, Yuki wrote: > missing semi-colon. Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:51: query = query.substring(0, query.length - 1) // Strip trailing slash. On 2013/01/31 10:57:11, Yuki wrote: > if (query.slice(-1) == '/') { > query = query.slice(0, -1); > } > > 1. string.slice() supports a negative index. > 2. Always use braces {} in JavaScript. > 3. Put a semi colon at the end of statement. > Done. > By the way, why do you need to strip the last slash? > In general, it's possible that a CGI parameter has a value ending with a slash. > query is not URL path. I'm not sure. What do you think, Dennis? https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:55: var v = s[i].split('='); On 2013/01/31 10:57:11, Yuki wrote: > This code does not handle '=' in the value correctly. > 'a=b=c'.split('=') produces ['a', 'b', 'c']. > > In general, values may include '=' in itself. Hmm, I thought to fix it, but stopped it not to have more difference from the original dashboard/ui/js/common.js. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:57: var value = unescape(v[1]); On 2013/01/31 10:57:11, Yuki wrote: > escape/unescape are obsolete. > Use encodeURIComponent/decodeURIComponent instead. Stopped to fix it in the same reason above. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:67: result['rev'] = Math.max(result['rev'], -1); On 2013/01/31 10:57:11, Yuki wrote: > Why -1? Can rev be negative? > > By the way, if 'rev=abc', then parseInt('abc') => NaN, and Math.max(NaN, -1) => > NaN. I'm not sure. What do you think, Dennis? https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:76: * @param {Object} An object containing parameters for a URL query string. On 2013/01/31 10:57:11, Yuki wrote: > {Object.<string, (string|number)>} Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:76: * @param {Object} An object containing parameters for a URL query string. On 2013/01/31 10:57:11, Yuki wrote: > Missing parameter name. Done. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:83: if (!p) On 2013/01/31 10:57:11, Yuki wrote: > Shouldn't reach here. > For "for (p in params)", p must not be null or undefined. We may want to remove it. What do you think, Dennis? https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:85: url += sep + p + '=' + params[p]; On 2013/01/31 10:57:11, Yuki wrote: > You'd better use encodeURIComponent() for values. > > By the way, I usually do > > var key_values = []; > for (key in params) { > // better to sanitize key here. > key_values.push(key + '=' + encodeURIComponent(params[key])); > } > return path + '?' + key_values.join('&'); > > If we had very many keys, array + join is more efficient than string > concatenation, and no need to care about '?' and '&'. What do you think, Dennis? https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... File dashboard/ui/endure_js/coordinates.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... dashboard/ui/endure_js/coordinates.js:14: * On 2013/01/31 10:57:11, Yuki wrote: > Usually people don't separate annotations (@constructor and @param) with an > empty line. > For example, Closure library say, > > /** > * Create an instance of a DOM helper with a new document object. > * @param {Document=} opt_document Document object to associate with this > * DOM helper. > * @constructor > */ WDYT, Dennis? https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... dashboard/ui/endure_js/coordinates.js:15: * @param {Array} plotData Data that will be plotted. It is an array of lines, On 2013/01/31 10:57:11, Yuki wrote: > {Array.<Array.<Array.<number>>>>} > > or better use @typedef > > /** @typedef {Array.<number>} */ > var PlotDataPoint; > /** @typedef {Array.<PlotDataPoint>} */ > var PlotDataLine; > > then, @param {Array.<PlotDataLine>} plotData ... Temporarily changed it to {Array.<Array.<Array.<number>>>>}. https://codereview.chromium.org/12094074/diff/3001/generate_perf.py File generate_perf.py (right): https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode377 generate_perf.py:377: print system_dir On 2013/01/31 09:39:22, Mike Stipicevic wrote: > I think this was left in for testing, do you mind taking it out? Ugh, thanks for the good catch. removed. https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode386 generate_perf.py:386: print ' ' + test_dir On 2013/01/31 09:39:22, Mike Stipicevic wrote: > same Done.
LGTM. I'm okay to go with just copying the files with no changes. Having said that, the current code seems using unescape() but not using escape(), which seems inconsistent. Also encode/decodeURIComponent and escape/unescape have much difference and encode/decodeURIComponent should be safer. It wouldn't be a serious security hole for Endure graphing but it's very hard to prove it. It's much easier and costless to fix them than to prove it's absolutely safe. So I'd recommend you guys fix them in near future. No need to work on other nitpicks.
LGTM, but may I ask a favor to fix this issue? https://codereview.chromium.org/12094074/diff/4004/dashboard/ui/endure_js/end... File dashboard/ui/endure_js/endure_plotter.js (right): https://codereview.chromium.org/12094074/diff/4004/dashboard/ui/endure_js/end... dashboard/ui/endure_js/endure_plotter.js:77: return eval('(' + data + ')') FYI, eval() is seriously dangerous. Remember you need to fix this issue. Please use a pair of JSON.stringify() and JSON.parse().
I wonder if I could have your l g t m for confirmation, Dennis. :) (Also for the other two patchsets.)
Replies to earlier comments, and a few new comments. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... File dashboard/ui/endure_js/common.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:67: result['rev'] = Math.max(result['rev'], -1); On 2013/02/01 06:14:13, Dai Mikurube wrote: > On 2013/01/31 10:57:11, Yuki wrote: > > Why -1? Can rev be negative? > > > > By the way, if 'rev=abc', then parseInt('abc') => NaN, and Math.max(NaN, -1) > => > > NaN. > > I'm not sure. What do you think, Dennis? I don't think "rev" should ever be negative from the perspective of the endurance tests, so I'd say that we can go ahead and remove this statement. Also, "rev" should always be something that can parse into an int. If it isn't, Yuki indicates that trying to parse it into an int will be NaN, and I think that's fine here. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:83: if (!p) On 2013/02/01 06:14:13, Dai Mikurube wrote: > On 2013/01/31 10:57:11, Yuki wrote: > > Shouldn't reach here. > > For "for (p in params)", p must not be null or undefined. > > We may want to remove it. What do you think, Dennis? Yes, if it's true that p will never be null or undefined in this kind of loop, then it's safe to remove this check. It does make sense that if we loop through an object's properties, there should never be a null or undefined property (though I guess its corresponding value may be null or undefined). https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:85: url += sep + p + '=' + params[p]; On 2013/02/01 06:14:13, Dai Mikurube wrote: > On 2013/01/31 10:57:11, Yuki wrote: > > You'd better use encodeURIComponent() for values. > > > > By the way, I usually do > > > > var key_values = []; > > for (key in params) { > > // better to sanitize key here. > > key_values.push(key + '=' + encodeURIComponent(params[key])); > > } > > return path + '?' + key_values.join('&'); > > > > If we had very many keys, array + join is more efficient than string > > concatenation, and no need to care about '?' and '&'. > > What do you think, Dennis? I think this sounds good, as long as we make sure to decode the values on the other end where they're consumed. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... File dashboard/ui/endure_js/coordinates.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... dashboard/ui/endure_js/coordinates.js:14: * On 2013/02/01 06:14:13, Dai Mikurube wrote: > On 2013/01/31 10:57:11, Yuki wrote: > > Usually people don't separate annotations (@constructor and @param) with an > > empty line. > > For example, Closure library say, > > > > /** > > * Create an instance of a DOM helper with a new document object. > > * @param {Document=} opt_document Document object to associate with this > > * DOM helper. > > * @constructor > > */ > > WDYT, Dennis? I'm ok with making this change if it's closer to what's typically done. https://codereview.chromium.org/12094074/diff/3001/generate_perf.py File generate_perf.py (right): https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode266 generate_perf.py:266: # Add detail tabs to config nit: add period at end of sentence https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode279 generate_perf.py:279: open(os.path.join(perf_dir, 'config.js'), 'w').write(contents) should we use the with/open syntax here to ensure that the file descriptor is closed immediately when we're done with it? with open(os.path.join(perf_dir, 'config.js'), 'w') as fp: fp.write(contents) https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode309 generate_perf.py:309: open(os.path.join(perf_dir, 'config.js'), 'w').write(contents) should we use the with/open syntax here too?
Thanks, Dennis. I've fixed them, and I'll set up another change for refactor the code later if required. What do you think? https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... File dashboard/ui/endure_js/common.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:67: result['rev'] = Math.max(result['rev'], -1); On 2013/02/05 02:48:28, dennis_jeffrey wrote: > On 2013/02/01 06:14:13, Dai Mikurube wrote: > > On 2013/01/31 10:57:11, Yuki wrote: > > > Why -1? Can rev be negative? > > > > > > By the way, if 'rev=abc', then parseInt('abc') => NaN, and Math.max(NaN, -1) > > => > > > NaN. > > > > I'm not sure. What do you think, Dennis? > > I don't think "rev" should ever be negative from the perspective of the > endurance tests, so I'd say that we can go ahead and remove this statement. > Also, "rev" should always be something that can parse into an int. If it isn't, > Yuki indicates that trying to parse it into an int will be NaN, and I think > that's fine here. Removed the statement. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:83: if (!p) On 2013/02/05 02:48:28, dennis_jeffrey wrote: > On 2013/02/01 06:14:13, Dai Mikurube wrote: > > On 2013/01/31 10:57:11, Yuki wrote: > > > Shouldn't reach here. > > > For "for (p in params)", p must not be null or undefined. > > > > We may want to remove it. What do you think, Dennis? > > Yes, if it's true that p will never be null or undefined in this kind of loop, > then it's safe to remove this check. It does make sense that if we loop through > an object's properties, there should never be a null or undefined property > (though I guess its corresponding value may be null or undefined). Applied the change below. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/com... dashboard/ui/endure_js/common.js:85: url += sep + p + '=' + params[p]; On 2013/02/05 02:48:28, dennis_jeffrey wrote: > On 2013/02/01 06:14:13, Dai Mikurube wrote: > > On 2013/01/31 10:57:11, Yuki wrote: > > > You'd better use encodeURIComponent() for values. > > > > > > By the way, I usually do > > > > > > var key_values = []; > > > for (key in params) { > > > // better to sanitize key here. > > > key_values.push(key + '=' + encodeURIComponent(params[key])); > > > } > > > return path + '?' + key_values.join('&'); > > > > > > If we had very many keys, array + join is more efficient than string > > > concatenation, and no need to care about '?' and '&'. > > > > What do you think, Dennis? > > I think this sounds good, as long as we make sure to decode the values on the > other end where they're consumed. Replaced with the code by Yuki. https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... File dashboard/ui/endure_js/coordinates.js (right): https://codereview.chromium.org/12094074/diff/3001/dashboard/ui/endure_js/coo... dashboard/ui/endure_js/coordinates.js:14: * On 2013/02/05 02:48:28, dennis_jeffrey wrote: > On 2013/02/01 06:14:13, Dai Mikurube wrote: > > On 2013/01/31 10:57:11, Yuki wrote: > > > Usually people don't separate annotations (@constructor and @param) with an > > > empty line. > > > For example, Closure library say, > > > > > > /** > > > * Create an instance of a DOM helper with a new document object. > > > * @param {Document=} opt_document Document object to associate with this > > > * DOM helper. > > > * @constructor > > > */ > > > > WDYT, Dennis? > > I'm ok with making this change if it's closer to what's typically done. Removed empty lines from all annotations in this file. https://codereview.chromium.org/12094074/diff/3001/generate_perf.py File generate_perf.py (right): https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode266 generate_perf.py:266: # Add detail tabs to config On 2013/02/05 02:48:28, dennis_jeffrey wrote: > nit: add period at end of sentence Done. https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode279 generate_perf.py:279: open(os.path.join(perf_dir, 'config.js'), 'w').write(contents) On 2013/02/05 02:48:28, dennis_jeffrey wrote: > should we use the with/open syntax here to ensure that the file descriptor is > closed immediately when we're done with it? > > with open(os.path.join(perf_dir, 'config.js'), 'w') as fp: > fp.write(contents) Done. (I'm not sure why the existing code does it.) https://codereview.chromium.org/12094074/diff/3001/generate_perf.py#newcode309 generate_perf.py:309: open(os.path.join(perf_dir, 'config.js'), 'w').write(contents) On 2013/02/05 02:48:28, dennis_jeffrey wrote: > should we use the with/open syntax here too? Done. https://codereview.chromium.org/12094074/diff/4004/dashboard/ui/endure_js/end... File dashboard/ui/endure_js/endure_plotter.js (right): https://codereview.chromium.org/12094074/diff/4004/dashboard/ui/endure_js/end... dashboard/ui/endure_js/endure_plotter.js:77: return eval('(' + data + ')') On 2013/02/01 07:31:03, Yuki wrote: > FYI, > > eval() is seriously dangerous. > Remember you need to fix this issue. > Please use a pair of JSON.stringify() and JSON.parse(). Fixed the eval to JSON.parse().
LGTM
On 2013/02/06 00:47:17, dennis_jeffrey wrote: > LGTM Thanks. I'm committing it. I'll set up a refactoring issue if required, or please do it in your merge of https://codereview.chromium.org/12087097/.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/12094074/18009
Message was sent while issue was closed.
Change committed as 180837 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
