Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10679009:
Revision to CPM UI using Flot and JQuery (Closed)
|
Created:
8 years, 6 months ago by clintstaley Modified:
8 years, 5 months ago Reviewers:
open-source-third-party-reviews, Steve Block, koz (OOO until 15th September), Dan Beam, clintstaley CC:
chromium-reviews, Evan Stade, Matt Tytel, gone Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionNew version of performance monitor UI, using Flot and JQuery
Image= http://imgur.com/8ifMy
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145626
Patch Set 1 #Patch Set 2 : Version without nonminified Flot code #
Total comments: 60
Patch Set 3 : New version with comments, event changes, etc. #
Total comments: 14
Patch Set 4 : Fixes per review #
Total comments: 71
Patch Set 5 : Fixes per review #Patch Set 6 : Revised constructor patterns per review #
Total comments: 11
Patch Set 7 : Final small fixes #Messages
Total messages: 33 (0 generated)
Koz and Finnur, Thanks for being willing to help us out. Clint
I thought we decided to check in the minified version of flot, since it's findable on the web...?
+estade Evan, I don't want you to waste a bunch of time reviewing this until the extensions guys have gone through it. But here's the CL for context.
Can do. Wasn't sure if you all wanted the whole thing in third_party, or only the minimized stuff. License issues and all that. Will post a version w/o in the nonminified in 10 min. Clint On 6/26/2012 4:32 PM, aa@chromium.org wrote: > I thought we decided to check in the minified version of flot, since it's > findable on the web...? > > http://codereview.chromium.org/10679009/ >
> Koz and Finnur, > Thanks for being willing to help us out. This is the first I hear of this. There's no bug or any context and from looking at the changelist I suspect you want someone with more js experience than me... On 2012/06/26 23:54:10, clintstaley_gmail.com wrote: > Can do. Wasn't sure if you all wanted the whole thing in third_party, > or only the minimized stuff. License issues and all that. Will post a > version w/o in the nonminified in 10 min. > > Clint > > > On 6/26/2012 4:32 PM, mailto:aa@chromium.org wrote: > > I thought we decided to check in the minified version of flot, since it's > > findable on the web...? > > > > http://codereview.chromium.org/10679009/ > >
On Wed, Jun 27, 2012 at 2:55 AM, <finnur@chromium.org> wrote: >> Koz and Finnur, >> Thanks for being willing to help us out. > > This is the first I hear of this. There's no bug or any context and from > looking > at the changelist I suspect you want someone with more js experience than > me... koz has lots of js experience. You have lots of general UI, C++, and Chrome design experience. That was my thinking anyway. - a
let's just get some style nits out of the way first then I can look a little deeper into the code after http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.html:15: src="../../../../third_party/flot/jquery.min.js"> </script> nit: no space before </script> http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.html:34: <div id="labelTemplate" class="event-label"> nit: move </div> to end of same line http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:8: function docGet(criterion) { why did you change the name here to docGet? that seems like it'd return the document itself or the owner document for a node... additionally every single use of docGet() (that I can tell) simply uses '#id-name', so can you simply change this to document.getElementById() or include src/chrome/browser/resources/shared/js/util.js via <script src="chrome://resources/js/util.js"></script> from the HTML file that embeds this .js (preferred) or by: <include "../shared/js/util.js"> right after 'use strict';? http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:39: // All metrics have entries, but those not displayed have empty div list. have an empty div list? http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:40: // If a div list is nonempty, the associated data will be nonull, or null but not empty, non-null http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:41: // about to be filled by webui response. Any metric with nonempty div I think all these object literals would be easier to read if indented like so: this.metricMap = { jankiness: { data: null, description: 'Jankiness', units: 'milliJanks' }, ... http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:45: data: null, description: 'Jankiness', units: 'milliJanks'}, I think it'd be easier to read if indented like so: this.metricMap = { jankiness: { data: null, description: 'Jankiness', units: 'milliJanks' }, ... http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:66: // as a model. as a template. (model implies something else) http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:82: radio.addEventListener('click', function() { instead of creating an event listener for each radio, it'd seem like you could delegate these event handlers higher in the DOM and take advantage of event propagation, e.g.: timeDiv.addEventListener('click', function(e) { if (!e.target.webkitMatchesSelector('input[type="radio"]')) return; controller.setTimeRange(e.target.timeRange); }); when you click anywhere in timeDiv we check to see if it was a click on a radio, and if it was, we use e.target (which is the radio) to set the time range. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:89: // or metrics. Take a div ID |divId| into which to place the checkboxes, comments describing the parameters to this function are more standardized using JSDoc notation, i.e. /** * @param {string} divId The ID of the div to set up. */ (and on). Here's a guide to how to do this - https://developers.google.com/closure/compiler/docs/js-for-compiler - we do it pretty much everywhere else in WebUI code. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:101: checkbox.getElementsByTagName('span')[0].innerText = 'Show ' + you could also just .querySelector('span') here http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:111: }.bind(input, check.bind(this, option), uncheck.bind(this, option))); why are you doing all these binds? http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:115: /* Outdated specialized functions replaced by setupCheckboxes. Included I'm confused, what do you want me to do with this? Generally I only publish for review what I think is ready to be committed right after LG, so making something like this makes you modify the code after somebody reviews or makes somebody re-review again. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:171: } all function expressions (something = function(){};) should end with a ; after the end curly }, so this would be: this.setupMainChart = function() { // ... }; http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:178: this.end = Math.floor(new Date().getTime() / range.resolution) * instead of new Date().getTime() -> Date.now() http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:182: this.end -= new Date().getTimezoneOffset() * 60000; what is this doing? http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:192: {'start': this.start + interval * .1, remove quotes around 'key': names and line things up like this if they need to wrap and you don't want to expand like I mentioned above (putting new lines like in pretty printed JSON): {start: this.start + interval * .1, end: this.start + interval * .2}, http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:201: // Replace with: chrome.send('getIntervals', this.start, this.end, when will you do this? http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:213: if (metricValue.divs.length > 0) { // If we're displaying this metric. nit: no curlies for 1 line if (/* conditional */) and { /* expression */ } (and any number of else if () {} and else {} statements afterward), i.e. if (who) what(); else if (where) when(); else why(); http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:220: if (eventValue.divs.length > 0) { nit: no curlies http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:391: for (var x = 0; x < chartData.metrics.length; x++) nit: curlies around the for loop (as it's more than 1 line) http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:395: for (var x = 0; x < chartData.events.length; x++) nit: see above, also why are you using x as a loop variable? http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:413: markings.push({xaxis: {from: point.time, to: point.time}, I think it'd be cleaner to read if there were line breaks here, i.e.: markings.push({ color: eventValue.color, description: eventValue.description, xaxis: {from: point.time, to: point.time}, }); http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:429: if (!this.isDataReady(chartData)) move this if () to before: var seriesSeq = []; var yAxes = []; http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:435: seriesSeq.push({ I think only 2 spaces indented here (instead of 4) http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:447: var plot = $.plot(chart, seriesSeq, { same here about 2 \s instead of 4 http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:459: var point = plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max, optional nit: may be easier to read if you do this instead: var point = plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max, yaxis: 1}); http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:469: this.setupCheckboxes('#chooseMetrics', this.metricMap, this.addMetric, optional nit: same general readability comment here this.setupCheckboxes( '#chooseEvents', this.eventMap, this.addEventType,this.dropMetric); http://codereview.chromium.org/10679009/diff/4001/third_party/flot/LICENSE.txt File third_party/flot/LICENSE.txt (right): http://codereview.chromium.org/10679009/diff/4001/third_party/flot/LICENSE.tx... third_party/flot/LICENSE.txt:1: Copyright (c) 2007-2009 IOLA and Ole Laursen did you run check licenses on this?
Dan, thanks for your thorough and educational review. I hope this version is closer to the mark. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.html:34: <div id="labelTemplate" class="event-label"> On 2012/06/28 21:39:14, Dan Beam wrote: > nit: move </div> to end of same line Done, though I did this intentionally because the <div> in question will be filled in with stuff when in use. Just seemed natural. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:8: function docGet(criterion) { On 2012/06/28 21:39:14, Dan Beam wrote: > why did you change the name here to docGet? that seems like it'd return the > document itself or the owner document for a node... additionally every single > use of docGet() (that I can tell) simply uses '#id-name', so can you simply > change this to document.getElementById() or include > src/chrome/browser/resources/shared/js/util.js via > > <script src="chrome://resources/js/util.js"></script> > > from the HTML file that embeds this .js (preferred) or by: > > <include "../shared/js/util.js"> > > right after 'use strict';? This was originally a hand-written $ function, cause I didn't know about util.js. But, in any event the I believe the $ in util.js will clash with the $ in JQuery, which latter $ produces not an Element but a wrapper around an Element, or at least a decorated Element IIUC. Given this, can I even use util.js?? And, as for getElementById, aa@ dings me for that and asks for use of querySelector, instead. I can't win. :). At any rate, I'll rename the (badly) named function, and await further comments. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:39: // All metrics have entries, but those not displayed have empty div list. On 2012/06/28 21:39:14, Dan Beam wrote: > have an empty div list? I tend to omit definite articles in phrases qualifying a plural, but I admit it reads a little oddly. Fixed. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:40: // If a div list is nonempty, the associated data will be nonull, or null but On 2012/06/28 21:39:14, Dan Beam wrote: > not empty, non-null Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:41: // about to be filled by webui response. Any metric with nonempty div On 2012/06/28 21:39:14, Dan Beam wrote: > I think all these object literals would be easier to read if indented like so: > > this.metricMap = { > jankiness: { > data: null, > description: 'Jankiness', > units: 'milliJanks' > }, > ... Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:45: data: null, description: 'Jankiness', units: 'milliJanks'}, On 2012/06/28 21:39:14, Dan Beam wrote: > I think it'd be easier to read if indented like so: > > this.metricMap = { > jankiness: { > data: null, > description: 'Jankiness', > units: 'milliJanks' > }, > ... Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:66: // as a model. On 2012/06/28 21:39:14, Dan Beam wrote: > as a template. (model implies something else) Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:82: radio.addEventListener('click', function() { On 2012/06/28 21:39:14, Dan Beam wrote: > instead of creating an event listener for each radio, it'd seem like you could > delegate these event handlers higher in the DOM and take advantage of event > propagation, e.g.: > > timeDiv.addEventListener('click', function(e) { > if (!e.target.webkitMatchesSelector('input[type="radio"]')) > return; > > controller.setTimeRange(e.target.timeRange); > }); > > when you click anywhere in timeDiv we check to see if it was a click on a radio, > and if it was, we use e.target (which is the radio) to set the time range. Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:89: // or metrics. Take a div ID |divId| into which to place the checkboxes, On 2012/06/28 21:39:14, Dan Beam wrote: > comments describing the parameters to this function are more standardized using > JSDoc notation, i.e. > > /** > * @param {string} divId The ID of the div to set up. > */ > > (and on). Here's a guide to how to do this - > https://developers.google.com/closure/compiler/docs/js-for-compiler - we do it > pretty much everywhere else in WebUI code. thanks. I'd been wondering about these, but didn't have the reference. Fixed throughout. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:101: checkbox.getElementsByTagName('span')[0].innerText = 'Show ' + On 2012/06/28 21:39:14, Dan Beam wrote: > you could also just .querySelector('span') here Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:111: }.bind(input, check.bind(this, option), uncheck.bind(this, option))); On 2012/06/28 21:39:14, Dan Beam wrote: > why are you doing all these binds? Cause I'm an idiot :). More specifically, I'm new enough at this that I built from an example where the event handler had no parameters, not realizing there's an alternative where it has an Event parameter, as you show above. Rewriting with that vastly reduces the bind-cuteness. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:115: /* Outdated specialized functions replaced by setupCheckboxes. Included On 2012/06/28 21:39:14, Dan Beam wrote: > I'm confused, what do you want me to do with this? Generally I only publish for > review what I think is ready to be committed right after LG, so making something The function with all the binds could be viewed as too "fancy". I wanted the reviewer to have the chance to say "No, just do this in a somewhat repetitious but straightforward way". I'll drop these now that the other function is less intricate. > like this makes you modify the code after somebody reviews or makes somebody > re-review again. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:171: } On 2012/06/28 21:39:14, Dan Beam wrote: > all function expressions (something = function(){};) should end with a ; after > the end curly }, so this would be: > > this.setupMainChart = function() { > // ... > }; Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:178: this.end = Math.floor(new Date().getTime() / range.resolution) * On 2012/06/28 21:39:14, Dan Beam wrote: > instead of new Date().getTime() -> Date.now() Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:182: this.end -= new Date().getTimezoneOffset() * 60000; On 2012/06/28 21:39:14, Dan Beam wrote: > what is this doing? Commment expanded upon... http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:192: {'start': this.start + interval * .1, On 2012/06/28 21:39:14, Dan Beam wrote: > remove quotes around 'key': names and line things up like this if they need to > wrap and you don't want to expand like I mentioned above (putting new lines like > in pretty printed JSON): > > {start: this.start + interval * .1, > end: this.start + interval * .2}, Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:201: // Replace with: chrome.send('getIntervals', this.start, this.end, On 2012/06/28 21:39:14, Dan Beam wrote: > when will you do this? Next phase, once we get the UI code OK'ed and landed, and once we get the webui code that supports it landed (nearly there now) http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:213: if (metricValue.divs.length > 0) { // If we're displaying this metric. On 2012/06/28 21:39:14, Dan Beam wrote: > nit: no curlies for 1 line if (/* conditional */) and { /* expression */ } (and > any number of else if () {} and else {} statements afterward), i.e. > > if (who) > what(); > else if (where) > when(); > else > why(); Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:220: if (eventValue.divs.length > 0) { On 2012/06/28 21:39:14, Dan Beam wrote: > nit: no curlies Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:391: for (var x = 0; x < chartData.metrics.length; x++) On 2012/06/28 21:39:14, Dan Beam wrote: > nit: curlies around the for loop (as it's more than 1 line) But it's one statement :) Done http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:395: for (var x = 0; x < chartData.events.length; x++) On 2012/06/28 21:39:14, Dan Beam wrote: > nit: see above, also why are you using x as a loop variable? Seen it done other places; never sure what the rules are for naming loop vars. Is the the one-letter (you didn't ding me for "i") or the implied geometry of an "x"? For now, I've changed to "i" where I used x. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:413: markings.push({xaxis: {from: point.time, to: point.time}, On 2012/06/28 21:39:14, Dan Beam wrote: > I think it'd be cleaner to read if there were line breaks here, i.e.: > > markings.push({ > color: eventValue.color, > description: eventValue.description, > xaxis: {from: point.time, to: point.time}, > }); Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:429: if (!this.isDataReady(chartData)) On 2012/06/28 21:39:14, Dan Beam wrote: > move this if () to before: > > var seriesSeq = []; > var yAxes = []; Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:435: seriesSeq.push({ On 2012/06/28 21:39:14, Dan Beam wrote: > I think only 2 spaces indented here (instead of 4) Yeah, wasn't sure if it counted as a line continuation or a level of indentation. Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:447: var plot = $.plot(chart, seriesSeq, { On 2012/06/28 21:39:14, Dan Beam wrote: > same here about 2 \s instead of 4 Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:459: var point = plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max, On 2012/06/28 21:39:14, Dan Beam wrote: > optional nit: may be easier to read if you do this instead: > > var point = > plot.pointOffset({x: mark.xaxis.to, y: yAxes[0].max, yaxis: 1}); Done. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:469: this.setupCheckboxes('#chooseMetrics', this.metricMap, this.addMetric, On 2012/06/28 21:39:14, Dan Beam wrote: > optional nit: same general readability comment here > > this.setupCheckboxes( > '#chooseEvents', this.eventMap, this.addEventType,this.dropMetric); Done. http://codereview.chromium.org/10679009/diff/4001/third_party/flot/LICENSE.txt File third_party/flot/LICENSE.txt (right): http://codereview.chromium.org/10679009/diff/4001/third_party/flot/LICENSE.tx... third_party/flot/LICENSE.txt:1: Copyright (c) 2007-2009 IOLA and Ole Laursen On 2012/06/28 21:39:14, Dan Beam wrote: > did you run check licenses on this? Did now, after adding the needed README.chromium file.
will re-review soon, just publishing thoughts for right now http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:8: function docGet(criterion) { On 2012/06/29 22:34:20, clintstaley wrote: > On 2012/06/28 21:39:14, Dan Beam wrote: > > why did you change the name here to docGet? that seems like it'd return the > > document itself or the owner document for a node... additionally every single > > use of docGet() (that I can tell) simply uses '#id-name', so can you simply > > change this to document.getElementById() or include > > src/chrome/browser/resources/shared/js/util.js via > > > > <script src="chrome://resources/js/util.js"></script> > > > > from the HTML file that embeds this .js (preferred) or by: > > > > <include "../shared/js/util.js"> > > > > right after 'use strict';? > > This was originally a hand-written $ function, cause I didn't know about > util.js. But, in any event the I believe the $ in util.js will clash with the $ > in JQuery, which latter $ produces not an Element but a wrapper around an > Element, or at least a decorated Element IIUC. Given this, can I even use > util.js?? You *could* call jQuery.noConflict() <http://api.jquery.com/jQuery.noConflict/> which returns $ to the original reference, and then do something along the lines of: (function($) { // use $ as much as you'd like }(jQuery)); if you want to leave the existing code using $ in both places (though I generally wouldn't recommend using two widely know [semi-]globals interchangeably). I'm also OK with reviewing jQuery-esque code, so if you want to just change to using jQuery/Sizzle as your selector engine that's fine with me, I'll know what you mean. That way you can just say $('#blah') (and add a "[0]" if you want the raw DOM node). > And, as for getElementById, aa@ dings me for that and asks for use of > querySelector, instead. I can't win. :). I would use querySelector() if I thought I'd ever need to change away from just an ID, *but* Chrome's general philosophy is code for the now (as far as I understand) so I'd assume somebody would tell me to only use getElementById() (so I passed this on to you). It's not bad to leave as querySelector() as it allows more flexibility and if aa@ asked for it we can simply leave it be. > At any rate, I'll rename the (badly) named function, and await further comments. We use $ (which is a worse name) everywhere as well, :). http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:201: // Replace with: chrome.send('getIntervals', this.start, this.end, On 2012/06/29 22:34:20, clintstaley wrote: > On 2012/06/28 21:39:14, Dan Beam wrote: > > when will you do this? > > Next phase, once we get the UI code OK'ed and landed, and once we get the webui > code that supports it landed (nearly there now) OK. http://codereview.chromium.org/10679009/diff/4001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:395: for (var x = 0; x < chartData.events.length; x++) On 2012/06/29 22:34:20, clintstaley wrote: > On 2012/06/28 21:39:14, Dan Beam wrote: > > nit: see above, also why are you using x as a loop variable? > > Seen it done other places; never sure what the rules are for naming loop vars. > Is the the one-letter (you didn't ding me for "i") or the implied geometry of an > "x"? For now, I've changed to "i" where I used x. implied geometry, ya (you're also using {x,y}axis in this file a lot)
http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:59: * div list but null data is awaiting a data response from the webui. This last sentence seems to be just a repetition of the previous sentence. Omit? http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:285: * offset to max-offset. (This let us avoid direct overlap of let -> lets http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:300: dataPoints.push({time: time, value: offset + time / This expression could do with some extra parenthesis, and perhaps assign some intermediary expressions into variables? http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:355: metricValue.data.push(series = []); Could you put this on two lines, like: metricValue.data.push(series); series = []; http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:423: // chrome.send("getEvents", eventType, this.range.start, this.range.end); nit: Missing final callback parameter here? http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:466: if (eventValue.divs.length > 0) Add a comment explaining that events appear on all charts? http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:548: for (var run = 0; run < value.data.length; run++) { i seems like a more idiomatic iterator variable here.
Koz and/or Dan, back to you. http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:59: * div list but null data is awaiting a data response from the webui. On 2012/07/02 06:14:48, koz wrote: > This last sentence seems to be just a repetition of the previous sentence. Omit? I was repeating for clarification. (Teacher's instinct, I suppose. :)) I'll add a "thus", if that helps :) http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:285: * offset to max-offset. (This let us avoid direct overlap of On 2012/07/02 06:14:48, koz wrote: > let -> lets Done. http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:300: dataPoints.push({time: time, value: offset + time / On 2012/07/02 06:14:48, koz wrote: > This expression could do with some extra parenthesis, and perhaps assign some > intermediary expressions into variables? Done. http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:355: metricValue.data.push(series = []); On 2012/07/02 06:14:48, koz wrote: > Could you put this on two lines, like: > > metricValue.data.push(series); > series = []; C coder's style; sorry :) Will change. http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:423: // chrome.send("getEvents", eventType, this.range.start, this.range.end); On 2012/07/02 06:14:48, koz wrote: > nit: Missing final callback parameter here? Will add if it proves necessary, but the current design has a fixed-name callback function. http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:466: if (eventValue.divs.length > 0) On 2012/07/02 06:14:48, koz wrote: > Add a comment explaining that events appear on all charts? There's one elsewhere, but it's worth repeating here. http://codereview.chromium.org/10679009/diff/10001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:548: for (var run = 0; run < value.data.length; run++) { On 2012/07/02 06:14:48, koz wrote: > i seems like a more idiomatic iterator variable here. Done.
Cool, the code lgtm though I'm not sure if Dan had any further comments. I should mention too that it was a pleasure to read such a well-written and documented change. Thanks!
the code looks OK to me as well, will re-review one last time after the nits / questions http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:7: function querySelect(criterion) { put this inside controller if it's only used in there to avoid leaking globally http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:12: var controller = new function() { why are you using new function() {}; here? http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:48: /** @type {Object.<string, { move @type down a line past /** http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:55: * nit: we usually put a method/members's description first and then all @type/@param/@return/@private after this with no newline in the middle. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:58: * null but about to be filled by webui response. Thus, any metric with only 1 space between sentences in comments (everywhere) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:62: this.metricMap = { btw, are any of these methods/members only used in this file? if so, they should be named like other @private methods/members in webui (their name endsWithAnUnderscore_). http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:131: } nit: +1 \n http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:155: */ nit: no \n http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:173: doCheck(e.target.option); check.call(this, e.target.option); http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:175: doUncheck(e.target.option); uncheck.call(this, e.target.option); http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:176: }); }.bind(this)); http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:220: { start: this.start + interval * .1, nit: no space after { {start: ... end: ...}, http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:250: if (metricValue.divs.length > 0) // If we're displaying this metric. if (metricValue.divs.length > 0) // If we're displaying this metric. (only 2 spaces) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:291: * @param {!number} offset Adjustment factor * @param {!number} offset Adjustment factor (there was an extra space) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:298: // Rise from low offset to high max-offset in 100 point steps end comments with . http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:300: time += this.range.resolution) { + 1 \s before time += http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:349: var point; remove http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:353: point = points[pointIndex++]; var point = ... (just use var even though it's not truly making a loop local var) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:363: series.push([point.time, point.value]); -2 \s http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:375: this.eventMap[eventType].divs = this.charts; // Events show on all charts 2 spaces between ; and // http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:406: new Date(interval.start) + ' blah, blah blah'}); I assume this'll be real text eventually? http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:425: this.eventMap[eventType].data = null; // Mark eventType as awaiting response 2 spaces in front of comment http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:455: * @param {DOMElement} chart div for which to get relevant items Element, HTMLElement, or HTMLDivElement (there's no DOMElement) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:471: // Events post to all divs, if they post to any . at end of comment http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:490: if (chartData.metrics[i].data == null) is there a reason you're loosely checking if it's null vs. just: if (!chartData.metrics[i].data) ? http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:538: * Redraw the chart in div |chart|, IF all its dependent data is present. s/IF/if/ http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:542: * @param {DOMElement} chart div to redraw same type nit as above (no DOMElement) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:559: yaxis: yAxes.length, // Use just-added Y axis . at end. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:596: } };
Fixes made, Dan http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:7: function querySelect(criterion) { On 2012/07/02 21:33:10, Dan Beam wrote: > put this inside controller if it's only used in there to avoid leaking globally It loses all its brevity, though, if I do that. I instead tried your earlier suggestion of relying on the JQuery $ and using $(criterion)[0]. Works perfectly, and is concise. OK to go with that? http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:12: var controller = new function() { On 2012/07/02 21:33:10, Dan Beam wrote: > why are you using > > new function() {}; > > here? [amusement] Because I had a more conventional pattern, but aa@ likes the one-shot constructor syntax when you're making only one object. I get the impression that object construction styles are a matter of some debate in this language. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:48: /** @type {Object.<string, { On 2012/07/02 21:33:10, Dan Beam wrote: > move @type down a line past /** Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:55: * On 2012/07/02 21:33:10, Dan Beam wrote: > nit: we usually put a method/members's description first and then all > @type/@param/@return/@private after this with no newline in the middle. Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:58: * null but about to be filled by webui response. Thus, any metric with On 2012/07/02 21:33:10, Dan Beam wrote: > only 1 space between sentences in comments (everywhere) Oh all right. But comments are monospace font, and I think the two spaces are needed. (This is a bit of a religious war, though, so I will shaddap and just follow the rules :)) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:62: this.metricMap = { On 2012/07/02 21:33:10, Dan Beam wrote: > btw, are any of these methods/members only used in this file? if so, they should > be named like other @private methods/members in webui (their name > endsWithAnUnderscore_). Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:131: } On 2012/07/02 21:33:10, Dan Beam wrote: > nit: +1 \n Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:155: */ On 2012/07/02 21:33:10, Dan Beam wrote: > nit: no \n Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:173: doCheck(e.target.option); On 2012/07/02 21:33:10, Dan Beam wrote: > check.call(this, e.target.option); Ah. Thanks. That is better. I had earlier been trying to keep the event-source "this" for the listener, but that was only because of my earlier bad design. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:175: doUncheck(e.target.option); On 2012/07/02 21:33:10, Dan Beam wrote: > uncheck.call(this, e.target.option); Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:176: }); On 2012/07/02 21:33:10, Dan Beam wrote: > }.bind(this)); Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:220: { start: this.start + interval * .1, On 2012/07/02 21:33:10, Dan Beam wrote: > nit: no space after { > > {start: ... > end: ...}, Really? Doesn't this clash with 2-space indent rules? Or 4-space continuation rules? I'll change it, but I'd like to understand the rule so I don't trip over it later. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:250: if (metricValue.divs.length > 0) // If we're displaying this metric. On 2012/07/02 21:33:10, Dan Beam wrote: > if (metricValue.divs.length > 0) // If we're displaying this metric. > > (only 2 spaces) Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:291: * @param {!number} offset Adjustment factor On 2012/07/02 21:33:10, Dan Beam wrote: > * @param {!number} offset Adjustment factor > > (there was an extra space) Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:298: // Rise from low offset to high max-offset in 100 point steps On 2012/07/02 21:33:10, Dan Beam wrote: > end comments with . Done, but.. Does this include sentence fragments? e.g. if (max < last) // not yet done var total; // running total http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:300: time += this.range.resolution) { On 2012/07/02 21:33:10, Dan Beam wrote: > + 1 \s before time += Sorry, I'm confused. It's a 4-space indent.. ? http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:349: var point; On 2012/07/02 21:33:10, Dan Beam wrote: > remove Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:353: point = points[pointIndex++]; On 2012/07/02 21:33:10, Dan Beam wrote: > var point = ... > > (just use var even though it's not truly making a loop local var) Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:363: series.push([point.time, point.value]); On 2012/07/02 21:33:10, Dan Beam wrote: > -2 \s Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:375: this.eventMap[eventType].divs = this.charts; // Events show on all charts On 2012/07/02 21:33:10, Dan Beam wrote: > 2 spaces between ; and // Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:406: new Date(interval.start) + ' blah, blah blah'}); On 2012/07/02 21:33:10, Dan Beam wrote: > I assume this'll be real text eventually? Yes, supplied from the C++ side. The getMock... methods are all throwaways. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:425: this.eventMap[eventType].data = null; // Mark eventType as awaiting response On 2012/07/02 21:33:10, Dan Beam wrote: > 2 spaces in front of comment Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:455: * @param {DOMElement} chart div for which to get relevant items On 2012/07/02 21:33:10, Dan Beam wrote: > Element, HTMLElement, or HTMLDivElement (there's no DOMElement) Fixed throughout. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:471: // Events post to all divs, if they post to any On 2012/07/02 21:33:10, Dan Beam wrote: > . at end of comment Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:490: if (chartData.metrics[i].data == null) On 2012/07/02 21:33:10, Dan Beam wrote: > is there a reason you're loosely checking if it's null vs. just: > > if (!chartData.metrics[i].data) > > ? Unfamiliarity with JS :). Wasn't sure what negation of null resulted in. Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:538: * Redraw the chart in div |chart|, IF all its dependent data is present. On 2012/07/02 21:33:10, Dan Beam wrote: > s/IF/if/ That's emphasis, not a misspelling. Is there an accepted way to show such in a doc? *if* perhaps? http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:542: * @param {DOMElement} chart div to redraw On 2012/07/02 21:33:10, Dan Beam wrote: > same type nit as above (no DOMElement) Done. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:559: yaxis: yAxes.length, // Use just-added Y axis On 2012/07/02 21:33:10, Dan Beam wrote: > . at end. Fixed these throughout -- about a dozen cases -- sorry. But, still want to understand whether this is only required for full sentences. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:596: } On 2012/07/02 21:33:10, Dan Beam wrote: > }; lint tool dings me for this, oddly.
FYI http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:7: function querySelect(criterion) { On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > put this inside controller if it's only used in there to avoid leaking > globally > > It loses all its brevity, though, if I do that. I instead tried your earlier > suggestion of relying on the JQuery $ and using $(criterion)[0]. Works > perfectly, and is concise. OK to go with that? I'm fine with using jQuery as a selector engine (just passing though to document.querySelector anyways), otherwise you're including it relatively needlessly... http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:12: var controller = new function() { On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > why are you using > > > > new function() {}; > > > > here? > > [amusement] Because I had a more conventional pattern, but aa@ likes the > one-shot constructor syntax when you're making only one object. I get the > impression that object construction styles are a matter of some debate in this > language. Generally we do stuff like: cr.define('namespace', function() { var localStaticData = 1; function privateStatic() { localStaticData += 1; } function publicStatic() { return localStaticData; } /** @constructor */ function class() { var inst = new class(); inst.__proto__ = class.prototype; return inst; } class.prototype = { __proto__: OptionalSuperClass, publicMethod: function() { this.doSomethingSneaky_(); }, /** @protected */ protectedMethod: function() { // Somebody could change me. }, /** @private */ privateMethod_: function() { this.doSomethingSneakier_(); }, /** @inheritDoc */ inherittedMethod: function() { OptionalSuperClass.prototype.overriddenMethod.call(this); this.doSomethingForMe_(); }, /** @override */ overriddenMethod: function() { this.ignoreSuperClassesMethod_(); }, }; return { class: class, // Published to namespace.class. publicStatic: publicStatic, // Published to namespace.publicStatic. }; }); and then var instance = new namespace.class(); instance.publicMethod(); and namespace.publicStatic(); There are other things you can do we've set up like singletons (using addSingletonGetter, http://code.google.com/p/chromium/source/search?q=cr%5C.addsingletongetter) and other stuff like static classes (http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/shar...) I don't really think construction styles are a matter of debate if you're comfortable with the language (not sure much of Chrome team is). http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:300: time += this.range.resolution) { On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > + 1 \s before time += > > Sorry, I'm confused. It's a 4-space indent.. ? I've generally seen lined up with (, i.e. for (...; ...; ...) { http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:596: } On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > }; > > lint tool dings me for this, oddly. which tool? the js presubmit check?
http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:58: * null but about to be filled by webui response. Thus, any metric with On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > only 1 space between sentences in comments (everywhere) > > Oh all right. But comments are monospace font, and I think the two spaces are > needed. (This is a bit of a religious war, though, so I will shaddap and just > follow the rules :)) This is a convention throughout all of chrome.
http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:220: { start: this.start + interval * .1, On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > nit: no space after { > > > > {start: ... > > end: ...}, > > Really? Doesn't this clash with 2-space indent rules? Or 4-space continuation > rules? I'll change it, but I'd like to understand the rule so I don't trip over > it later. I break into a floating { when the params are too long to fit on a line: var someObjectWithALongName = { someReallyLongKeyNameThatWouldntFitEasilyInEightyChars: 1 }; though if you can fit the names, this way is encouraged: a.b({c: 1, d: 2}; or middle of the road: someClass.doSomething({someKindaLongName: 'a string', someOtherDescriptiveKey: 2}); the "no space in front of an object literal" rule is right outta that google js style guide: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:298: // Rise from low offset to high max-offset in 100 point steps On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > end comments with . > > Done, but.. > > Does this include sentence fragments? > > e.g. if (max < last) // not yet done > var total; // running total yes, but the solution is to just write sentences when you can, :) http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:538: * Redraw the chart in div |chart|, IF all its dependent data is present. On 2012/07/02 23:05:29, clintstaley wrote: > On 2012/07/02 21:33:10, Dan Beam wrote: > > s/IF/if/ > > That's emphasis, not a misspelling. Is there an accepted way to show such in a > doc? *if* perhaps? sure, that works
Dan, Koz, latest revision is in, with the big change in constructor/class design that Dan requested. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:12: var controller = new function() { On 2012/07/03 01:23:46, Dan Beam wrote: > On 2012/07/02 23:05:29, clintstaley wrote: > > On 2012/07/02 21:33:10, Dan Beam wrote: > > > why are you using > > > > > > new function() {}; > > > > > > here? > > > > [amusement] Because I had a more conventional pattern, but aa@ likes the > > one-shot constructor syntax when you're making only one object. I get the > > impression that object construction styles are a matter of some debate in this > > language. > > Generally we do stuff like: > > cr.define('namespace', function() { > var localStaticData = 1; > > function privateStatic() { > localStaticData += 1; > } > > function publicStatic() { > return localStaticData; > } > > /** @constructor */ > function class() { > var inst = new class(); > inst.__proto__ = class.prototype; > return inst; > } > > class.prototype = { > __proto__: OptionalSuperClass, > > publicMethod: function() { > this.doSomethingSneaky_(); > }, > > /** @protected */ > protectedMethod: function() { > // Somebody could change me. > }, > > /** @private */ > privateMethod_: function() { > this.doSomethingSneakier_(); > }, > > /** @inheritDoc */ > inherittedMethod: function() { > OptionalSuperClass.prototype.overriddenMethod.call(this); > this.doSomethingForMe_(); > }, > > /** @override */ > overriddenMethod: function() { > this.ignoreSuperClassesMethod_(); > }, > }; > > return { > class: class, // Published to namespace.class. > publicStatic: publicStatic, // Published to namespace.publicStatic. > }; > }); > > and then > > var instance = new namespace.class(); > instance.publicMethod(); > > and > > namespace.publicStatic(); > > There are other things you can do we've set up like singletons (using > addSingletonGetter, > http://code.google.com/p/chromium/source/search?q=cr%255C.addsingletongetter) and > other stuff like static classes > (http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/shar...) > > I don't really think construction styles are a matter of debate if you're > comfortable with the language (not sure much of Chrome team is). Perhaps you're right. I lack the depth to comment other than to note that I've heard varying opinions on how best to set up classes. The example you gave here is very informative, and I'll revise my code to conform (though I hope it's ok to defer the actual cr.define installation until we have this wired into the full webui). If I understand correctly, this approach holds static state in a closure of the class-creating function, which makes it implicitly private. (?) And one prefers putting instance methods into a __proto__ to avoid repeated references to them in every object (reminiscent of having one vtable object per class in C++?) Why does class() call new class() on itself and return the reference? I'd have thought the new namespace.class() at the bottom would suffice (?) Also (and perhaps this is related) I found I needed this in PerformanceMonitor: this.__proto__ = PerformanceMonitor.prototype; in order to call prototype-defined methods from within the constructor. Is there a way to avoid that, or is it just standard style to do that (redundant, IIUC) assignment early in the constructor if you want to call prototype-defined methods? Is this example something in the Chrome docs, or did you just write it? Do you mind if I use it as a basis for in-class (university course, I mean) examples? I'm planning a JS course at Poly in fall, and this is a good illustration of a lot of OO JS approaches. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:58: * null but about to be filled by webui response. Thus, any metric with On 2012/07/03 01:24:28, Dan Beam wrote: > On 2012/07/02 23:05:29, clintstaley wrote: > > On 2012/07/02 21:33:10, Dan Beam wrote: > > > only 1 space between sentences in comments (everywhere) > > > > Oh all right. But comments are monospace font, and I think the two spaces are > > needed. (This is a bit of a religious war, though, so I will shaddap and just > > follow the rules :)) > > This is a convention throughout all of chrome. And I'm really not objecting so much as joking about it -- it's a reasonable rule. Fixed throughout. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:220: { start: this.start + interval * .1, On 2012/07/03 01:29:14, Dan Beam wrote: > On 2012/07/02 23:05:29, clintstaley wrote: > > On 2012/07/02 21:33:10, Dan Beam wrote: > > > nit: no space after { > > > > > > {start: ... > > > end: ...}, > > > > Really? Doesn't this clash with 2-space indent rules? Or 4-space continuation > > rules? I'll change it, but I'd like to understand the rule so I don't trip > over > > it later. > > I break into a floating { when the params are too long to fit on a line: > > var someObjectWithALongName = { > someReallyLongKeyNameThatWouldntFitEasilyInEightyChars: 1 > }; > > though if you can fit the names, this way is encouraged: > > a.b({c: 1, d: 2}; > > or middle of the road: > > someClass.doSomething({someKindaLongName: 'a string', > someOtherDescriptiveKey: 2}); > > the "no space in front of an object literal" rule is right outta that google js > style guide: > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... OK. Thanks for clarification. Fixed in latest. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:298: // Rise from low offset to high max-offset in 100 point steps On 2012/07/03 01:29:14, Dan Beam wrote: > On 2012/07/02 23:05:29, clintstaley wrote: > > On 2012/07/02 21:33:10, Dan Beam wrote: > > > end comments with . > > > > Done, but.. > > > > Does this include sentence fragments? > > > > e.g. if (max < last) // not yet done > > var total; // running total > > yes, but the solution is to just write sentences when you can, :) OK, will do. http://codereview.chromium.org/10679009/diff/14001/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:300: time += this.range.resolution) { On 2012/07/03 01:23:46, Dan Beam wrote: > On 2012/07/02 23:05:29, clintstaley wrote: > > On 2012/07/02 21:33:10, Dan Beam wrote: > > > + 1 \s before time += > > > > Sorry, I'm confused. It's a 4-space indent.. ? > > I've generally seen lined up with (, i.e. > > for (...; ...; > ...) { Done.
lgtm w/nits http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:10: var Installer = function() { var Installer = (function() { http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:15: * value: !number, technically {string,number,boolean,function} don't need ! because they're not "nullable", but you can leave if you want as it doesn't *hurt*. http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:147: move: var controller = this; to right here or just say: timeDiv.addEventListener('click', function(e) { if (!e.target.webkitMatchesSelector('input[type="radio"]')) return; this.setTimeRange(e.target.timeRange); }.bind(this)); http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:593: }; }()); http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:595: var performanceMonitor = new Installer().PerformanceMonitor(); var performanceMonitor = new Installer.PerformanceMonitor();
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10679009/17004
Presubmit check for 10679009-17004 failed and returned exit status -2001. The presubmit check was hung. It took 362.6 seconds to execute and the time limit is 360.0 seconds. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. See the JavaScript style guide at http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript and if you have any feedback about the JavaScript PRESUBMIT check, contact tbreisacher@chromium.org ** Presubmit Warnings ** Found JavaScript style violations in third_party/flot/jquery.colorhelpers.min.js: line 1: E0002: Missing space before "{" (function(b){b.color={};b.color.make=function(f,e,c,d){var h={};h.r=f||0;h.g=e||0;h.b=c||0;h.a=d!=null?d:1;h.add=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]+=j}return h.normalize()};h.scale=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]*=j}return h.normalize()};h.toString=function(){if(h.a>=1){return"rgb("+[h.r,h.g,h.b].join(",")+")"}else{return"rgba("+[h.r,h.g,h.b,h.a].join(",")+")"}};h.normalize=function(){function g(j,k,i){return k<j?j:(k>i?i:k)}h.r=g(0,parseInt(h.r),255);h.g=g(0,parseInt(h.g),255);h.b=g(0,parseInt(h.b),255);h.a=g(0,h.a,1);return h};h.clone=function(){return b.color.make(h.r,h.b,h.g,h.a)};return h.normalize()};b.color.extract=function(e,d){var f;do{f=e.css(d).toLowerCase();if(f!=""&&f!="transparent"){break}e=e.parent()}while(!b.nodeName(e.get(0),"body"));if(f=="rgba(0, 0, 0, 0)"){f="transparent"}return b.color.parse(f)};b.color.parse=function(f){var e,c=b.color.make;if(e=/rgb\(\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*\)/.exec(f)){return c(parseInt(e[1],10),parseInt(e[2],10),parseInt(e[3],10))}if(e=/rgba\(\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]+(?:\.[0-9]+)?)\s*\)/.exec(f)){return c(parseInt(e[1],10),parseInt(e[2],10),parseInt(e[3],10),parseFloat(e[4]))}if(e=/rgb\(\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*\)/.exec(f)){return c(parseFloat(e[1])*2.55,parseFloat(e[2])*2.55,parseFloat(e[3])*2.55)}if(e=/rgba\(\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\s*\)/.exec(f)){return c(parseFloat(e[1])*2.55,parseFloat(e[2])*2.55,parseFloat(e[3])*2.55,parseFloat(e[4]))}if(e=/#([a-fA-F0-9]{2})([a-fA-F0-9]{2})([a-fA-F0-9]{2})/.exec(f)){return c(parseInt(e[1],16),parseInt(e[2],16),parseInt(e[3],16))}if(e=/#([a-fA-F0-9])([a-fA-F0-9])([a-fA-F0-9])/.exec(f)){return c(parseInt(e[1]+e[1],16),parseInt(e[2]+e[2],16),parseInt(e[3]+e[3],16))}var d=b.trim(f).toLowerCase();if(d=="transparent"){return c(255,255,255,0)}else{e=a[d]||[0,0,0];return c(e[0],e[1],e[2])}};var a={aqua:[0,255,255],azure:[240,255,255],beige:[245,245,220],black:[0,0,0],blue:[0,0,255],brown:[165,42,42],cyan:[0,255,255],darkblue:[0,0,139],darkcyan:[0,139,139],darkgrey:[169,169,169],darkgreen:[0,100,0],darkkhaki:[189,183,107],darkmagenta:[139,0,139],darkolivegreen:[85,107,47],darkorange:[255,140,0],darkorchid:[153,50,204],darkred:[139,0,0],darksalmon:[233,150,122],darkviolet:[148,0,211],fuchsia:[255,0,255],gold:[255,215,0],green:[0,128,0],indigo:[75,0,130],khaki:[240,230,140],lightblue:[173,216,230],lightcyan:[224,255,255],lightgreen:[144,238,144],lightgrey:[211,211,211],lightpink:[255,182,193],lightyellow:[255,255,224],lime:[0,255,0],magenta:[255,0,255],maroon:[128,0,0],navy:[0,0,128],olive:[128,128,0],orange:[255,165,0],pink:[255,192,203],purple:[128,0,128],violet:[128,0,128],red:[255,0,0],silver:[192,192,192],white:[255,255,255],yellow:[255,255,0]}})(jQuery); ^ line 1: E0002: Missing space before "=" (function(b){b.color={};b.color.make=function(f,e,c,d){var h={};h.r=f||0;h.g=e||0;h.b=c||0;h.a=d!=null?d:1;h.add=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]+=j}return h.normalize()};h.scale=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]*=j}return h.normalize()};h.toString=function(){if(h.a>=1){return"rgb("+[h.r,h.g,h.b].join(",")+")"}else{return"rgba("+[h.r,h.g,h.b,h.a].join(",")+")"}};h.normalize=function(){function g(j,k,i){return k<j?j:(k>i?i:k)}h.r=g(0,parseInt(h.r),255);h.g=g(0,parseInt(h.g),255);h.b=g(0,parseInt(h.b),255);h.a=g(0,h.a,1);return h};h.clone=function(){return b.color.make(h.r,h.b,h.g,h.a)};return h.normalize()};b.color.extract=function(e,d){var f;do{f=e.css(d).toLowerCase();if(f!=""&&f!="transparent"){break}e=e.parent()}while(!b.nodeName(e.get(0),"body"));if(f=="rgba(0, 0, 0, 0)"){f="transparent"}return b.color.parse(f)};b.color.parse=function(f){var e,c=b.color.make;if(e=/rgb\(\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*\)/.exec(f)){return c(parseInt(e[1],10),parseInt(e[2],10),parseInt(e[3],10))}if(e=/rgba\(\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]+(?:\.[0-9]+)?)\s*\)/.exec(f)){return c(parseInt(e[1],10),parseInt(e[2],10),parseInt(e[3],10),parseFloat(e[4]))}if(e=/rgb\(\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*\)/.exec(f)){return c(parseFloat(e[1])*2.55,parseFloat(e[2])*2.55,parseFloat(e[3])*2.55)}if(e=/rgba\(\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\s*\)/.exec(f)){return c(parseFloat(e[1])*2.55,parseFloat(e[2])*2.55,parseFloat(e[3])*2.55,parseFloat(e[4]))}if(e=/#([a-fA-F0-9]{2})([a-fA-F0-9]{2})([a-fA-F0-9]{2})/.exec(f)){return c(parseInt(e[1],16),parseInt(e[2],16),parseInt(e[3],16))}if(e=/#([a-fA-F0-9])([a-fA-F0-9])([a-fA-F0-9])/.exec(f)){return c(parseInt(e[1]+e[1],16),parseInt(e[2]+e[2],16),parseInt(e[3]+e[3],16))}var d=b.trim(f).toLowerCase();if(d=="transparent"){return c(255,255,255,0)}else{e=a[d]||[0,0,0];return c(e[0],e[1],e[2])}};var a={aqua:[0,255,255],azure:[240,255,255],beige:[245,245,220],black:[0,0,0],blue:[0,0,255],brown:[165,42,42],cyan:[0,255,255],darkblue:[0,0,139],darkcyan:[0,139,139],darkgrey:[169,169,169],darkgreen:[0,100,0],darkkhaki:[189,183,107],darkmagenta:[139,0,139],darkolivegreen:[85,107,47],darkorange:[255,140,0],darkorchid:[153,50,204],darkred:[139,0,0],darksalmon:[233,150,122],darkviolet:[148,0,211],fuchsia:[255,0,255],gold:[255,215,0],green:[0,128,0],indigo:[75,0,130],khaki:[240,230,140],lightblue:[173,216,230],lightcyan:[224,255,255],lightgreen:[144,238,144],lightgrey:[211,211,211],lightpink:[255,182,193],lightyellow:[255,255,224],lime:[0,255,0],magenta:[255,0,255],maroon:[128,0,0],navy:[0,0,128],olive:[128,128,0],orange:[255,165,0],pink:[255,192,203],purple:[128,0,128],violet:[128,0,128],red:[255,0,0],silver:[192,192,192],white:[255,255,255],yellow:[255,255,0]}})(jQuery); ^ line 1: E0002: Missing space after "=" (function(b){b.color={};b.color.make=function(f,e,c,d){var h={};h.r=f||0;h.g=e||0;h.b=c||0;h.a=d!=null?d:1;h.add=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]+=j}return h.normalize()};h.scale=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]*=j}return h.normalize()};h.toString=function(){if(h.a>=1){return"rgb("+[h.r,h.g,h.b].join(",")+")"}else{return"rgba("+[h.r,h.g,h.b,h.a].join(",")+")"}};h.normalize=function(){function g(j,k,i){return k<j?j:(k>i?i:k)}h.r=g(0,parseInt(h.r),255);h.g=g(0,parseInt(h.g),255);h.b=g(0,parseInt(h.b),255);h.a=g(0,h.a,1);return h};h.clone=function(){return b.color.make(h.r,h.b,h.g,h.a)};return h.normalize()};b.color.extract=function(e,d){var f;do{f=e.css(d).toLowerCase();if(f!=""&&f!="transparent"){break}e=e.parent()}while(!b.nodeName(e.get(0),"body"));if(f=="rgba(0, 0, 0, 0)"){f="transparent"}return b.color.parse(f)};b.color.parse=function(f){var e,c=b.color.make;if(e=/rgb\(\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*\)/.exec(f)){return c(parseInt(e[1],10),parseInt(e[2],10),parseInt(e[3],10))}if(e=/rgba\(\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]{1,3})\s*,\s*([0-9]+(?:\.[0-9]+)?)\s*\)/.exec(f)){return c(parseInt(e[1],10),parseInt(e[2],10),parseInt(e[3],10),parseFloat(e[4]))}if(e=/rgb\(\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*\)/.exec(f)){return c(parseFloat(e[1])*2.55,parseFloat(e[2])*2.55,parseFloat(e[3])*2.55)}if(e=/rgba\(\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\%\s*,\s*([0-9]+(?:\.[0-9]+)?)\s*\)/.exec(f)){return c(parseFloat(e[1])*2.55,parseFloat(e[2])*2.55,parseFloat(e[3])*2.55,parseFloat(e[4]))}if(e=/#([a-fA-F0-9]{2})([a-fA-F0-9]{2})([a-fA-F0-9]{2})/.exec(f)){return c(parseInt(e[1],16),parseInt(e[2],16),parseInt(e[3],16))}if(e=/#([a-fA-F0-9])([a-fA-F0-9])([a-fA-F0-9])/.exec(f)){return c(parseInt(e[1]+e[1],16),parseInt(e[2]+e[2],16),parseInt(e[3]+e[3],16))}var d=b.trim(f).toLowerCase();if(d=="transparent"){return c(255,255,255,0)}else{e=a[d]||[0,0,0];return c(e[0],e[1],e[2])}};var a={aqua:[0,255,255],azure:[240,255,255],beige:[245,245,220],black:[0,0,0],blue:[0,0,255],brown:[165,42,42],cyan:[0,255,255],darkblue:[0,0,139],darkcyan:[0,139,139],darkgrey:[169,169,169],darkgreen:[0,100,0],darkkhaki:[189,183,107],darkmagenta:[139,0,139],darkolivegreen:[85,107,47],darkorange:[255,140,0],darkorchid:[153,50,204],darkred:[139,0,0],darksalmon:[233,150,122],darkviolet:[148,0,211],fuchsia:[255,0,255],gold:[255,215,0],green:[0,128,0],indigo:[75,0,130],khaki:[240,230,140],lightblue:[173,216,230],lightcyan:[224,255,255],lightgreen:[144,238,144],lightgrey:[211,211,211],lightpink:[255,182,193],lightyellow:[255,255,224],lime:[0,255,0],magenta:[255,0,255],maroon:[128,0,0],navy:[0,0,128],olive:[128,128,0],orange:[255,165,0],pink:[255,192,203],purple:[128,0,128],violet:[128,0,128],red:[255,0,0],silver:[192,192,192],white:[255,255,255],yellow:[255,255,0]}})(jQuery); ^ line 1: E0002: Missing space before "=" (function(b){b.color={};b.color.make=function(f,e,c,d){var h={};h.r=f||0;h.g=e||0;h.b=c||0;h.a=d!=null?d:1;h.add=function(k,j){for(var g=0;g<k.length;++g){h[k.charAt(g)]+=j}return h.normal… (message too large)
^ fixing this issue here: https://chromiumcodereview.appspot.com/10692115/
I'm going to click the commit button for you as nothing has changed with this except for the bug in the web dev style checker (which has been fixed, at least in theory).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10679009/17004
Change committed as 145626
Note: When adding third-party code, please make sure to follow the directions at http://www.chromium.org/developers/adding-3rd-party-libraries In particular, you need an LGT.M from open-source-third-party-reviews@google.com , a README.chromium file according to the template, and a confirmation that both src/tools/licenses.py scan is correct and that src/tools/checklicenses/checklicenses.py is happy. The checklicenses.py step is the only bit that should have happened automatically. If you have not already gotten the review, this CL should be reverted.
Realizing open-source-third-party-reviews should have been on the reviewer list, per "Adding 3rd-party libraries". Doing so (belatedly -- sorry!) now. licenses.py and checklicenses.py show no errors for the flot addition made in this CL. Pls let me know if more is needed. http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:10: var Installer = function() { On 2012/07/06 23:49:36, Dan Beam wrote: > var Installer = (function() { Done. http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:15: * value: !number, On 2012/07/06 23:49:36, Dan Beam wrote: > technically {string,number,boolean,function} don't need ! because they're not > "nullable", but you can leave if you want as it doesn't *hurt*. Not sure I understand, here. Can I not write value: null in the enum? http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:147: On 2012/07/06 23:49:36, Dan Beam wrote: > move: > > var controller = this; > > to right here or just say: > > timeDiv.addEventListener('click', function(e) { > if (!e.target.webkitMatchesSelector('input[type="radio"]')) > return; > > this.setTimeRange(e.target.timeRange); > }.bind(this)); Done. http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:593: }; On 2012/07/06 23:49:36, Dan Beam wrote: > }()); Done. http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:595: var performanceMonitor = new Installer().PerformanceMonitor(); On 2012/07/06 23:49:36, Dan Beam wrote: > var performanceMonitor = new Installer.PerformanceMonitor(); Done.
http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10679009/diff/17002/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:15: * value: !number, On 2012/07/09 22:03:22, clintstaley wrote: > On 2012/07/06 23:49:36, Dan Beam wrote: > > technically {string,number,boolean,function} don't need ! because they're not > > "nullable", but you can leave if you want as it doesn't *hurt*. > > Not sure I understand, here. Can I not write value: null in the enum? > I honestly have no idea what that means either, but that's what it says in the doc: https://developers.google.com/closure/compiler/docs/js-for-compiler#null
I think you need a README.chromium for this third-party code. See http://www.chromium.org/developers/adding-3rd-party-libraries and 'tools/licenses.py scan'.
On 2012/07/10 08:01:46, Steve Block wrote: > I think you need a README.chromium for this third-party code. See > http://www.chromium.org/developers/adding-3rd-party-libraries and > 'tools/licenses.py scan'. Apologies. Forgot to add this one into GIT. Will post in a followup CL.
Will you have a bit of time to LGTM my CL that adds this missing file? Clint On 7/10/2012 1:01 AM, steveblock@chromium.org wrote: > I think you need a README.chromium for this third-party code. See > http://www.chromium.org/developers/adding-3rd-party-libraries and > 'tools/licenses.py scan'. > > https://chromiumcodereview.appspot.com/10679009/ >
Can you point me to your CL?
It looks like it's https://chromiumcodereview.appspot.com/10695131 |
