Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10820031:
Modifications to performance monitor UI to address real webui. (Closed)
|
Created:
8 years, 5 months ago by clintstaley Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionModifications to performance monitor UI to address real webui.
UI snapshot: http://imgur.com/js4nc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152214
Patch Set 1 : Final touchups for style. #
Total comments: 43
Patch Set 2 : Revision with all latest changes #
Total comments: 25
Patch Set 3 : Latest fixes per dbeam's review #Patch Set 4 : Further updates reflecting last week's fixes and tweaks #
Total comments: 24
Patch Set 5 : Fixes per dbeam #Patch Set 6 : Couple last fixes per dbeam #Patch Set 7 : Retrying because of weird Android build failure #Patch Set 8 : Once more into the breach.... #
Total comments: 2
Patch Set 9 : Revised build config to mollify Android tryserver #Messages
Total messages: 39 (0 generated)
I'm no expert on JS, CSS, HTML, or the GRD files, but here's what I found :) http://codereview.chromium.org/10820031/diff/2001/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/10820031/diff/2001/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:93: <include name="IDR_PERFORMANCE_MONITOR_HTML" file="resources\performance_monitor\chart.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" /> I think this list was supposed to be alphabetized, but whoever added PREDICTORS missed it. Could you move both PERFORMANCE_MONITOR and PREDICTORS to the proper spot? (Lines 106-107 and 121-22, respectively, in this version). http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:57: * webui. The webui uses numbers to uniquely identify metric and event nit: 1 space after periods. http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:323: metricValue.data = [series]; // data always ends with current open series nit: two spaces between code and comment. http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:402: result.points[i].time -= timezoneOffset; indentation http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:525: // Ensure at least one y axis, as a reference for event placement add a '.' http://codereview.chromium.org/10820031/diff/2001/chrome/browser/ui/webui/per... File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): http://codereview.chromium.org/10820031/diff/2001/chrome/browser/ui/webui/per... chrome/browser/ui/webui/performance_monitor/web_ui.cc:8: #include "chrome/browser/profiles/profile.h" sort http://codereview.chromium.org/10820031/diff/2001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): http://codereview.chromium.org/10820031/diff/2001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3261: 'browser/ui/metro_pinned_state_observer.h', What are all the extra changes in this file? (metro_*, keyboard_overlay_*, wallpaper_*)
http://codereview.chromium.org/10820031/diff/2001/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/10820031/diff/2001/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:93: <include name="IDR_PERFORMANCE_MONITOR_HTML" file="resources\performance_monitor\chart.html" flattenhtml="true" allowexternalscript="true" type="BINDATA" /> On 2012/07/26 22:10:30, D Cronin wrote: > I think this list was supposed to be alphabetized, but whoever added PREDICTORS > missed it. Could you move both PERFORMANCE_MONITOR and PREDICTORS to the proper > spot? (Lines 106-107 and 121-22, respectively, in this version). You're right. Thanks. Done http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:57: * webui. The webui uses numbers to uniquely identify metric and event On 2012/07/26 22:10:30, D Cronin wrote: > nit: 1 space after periods. Done, and elsewhere, too http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:323: metricValue.data = [series]; // data always ends with current open series On 2012/07/26 22:10:30, D Cronin wrote: > nit: two spaces between code and comment. Done. http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:402: result.points[i].time -= timezoneOffset; On 2012/07/26 22:10:30, D Cronin wrote: > indentation Done. http://codereview.chromium.org/10820031/diff/2001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:525: // Ensure at least one y axis, as a reference for event placement On 2012/07/26 22:10:30, D Cronin wrote: > add a '.' Done. http://codereview.chromium.org/10820031/diff/2001/chrome/browser/ui/webui/per... File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): http://codereview.chromium.org/10820031/diff/2001/chrome/browser/ui/webui/per... chrome/browser/ui/webui/performance_monitor/web_ui.cc:8: #include "chrome/browser/profiles/profile.h" On 2012/07/26 22:10:30, D Cronin wrote: > sort Done.
Folks, this is another of our CLs for the performance monitor page. UI is still in rough form. This CL adds use of the webui for obtaining data, vs earlier use of mocked-up test data
http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:5: div#chooseMetrics { you don't need any other qualifier than the id http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:28: div.chart { combine with the above also I doubt you need the tag name here either. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:34: background: #fff; white http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:40: .hidden { what is the point of this rule? normally .hidden means display: none. Are you overriding that somewhere? http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:187: * @param {!string} divId Id of division into which to put checkboxes you can make this a more flexible function if you make this an actual div instead of an id string. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate = $('#checkboxTemplate')[0]; you don't need the hashtag. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:208: if (e.target.checked) ternary imo http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:313: * }} result Object giving metric ID, and time/value point pairs for an extra space between result and Object http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:314: * that id. 4 more indent http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:377: ? http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:393: * each event of that type in the range requested. ditto http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:401: for (var i = 0; i < result.points.length; i++) { forEach http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:488: xaxis: {from: point.time, to: point.time} xAxis http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... chrome/browser/ui/webui/performance_monitor/web_ui.cc:25: source->add_resource_path("chart.css", IDR_PERFORMANCE_MONITOR_CHART_CSS); I don't think you need this these two (chart.*). Grit will inline them since you have flattenhtml.
Latest revision, with responses to estade@'s review. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:5: div#chooseMetrics { On 2012/07/28 01:01:16, Evan Stade wrote: > you don't need any other qualifier than the id CSS newbie mistakes: I thought perhaps greater specificity would be "good style". Fixed here and below. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:28: div.chart { On 2012/07/28 01:01:16, Evan Stade wrote: > combine with the above > > also I doubt you need the tag name here either. Removed the tag name, but the #charts style applies to the div enclosing all charts, of which there may eventually be more than just one. The .chart style applies to each individual chart div within the #charts div. Added comments clarifying this. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:34: background: #fff; On 2012/07/28 01:01:16, Evan Stade wrote: > white Done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:40: .hidden { On 2012/07/28 01:01:16, Evan Stade wrote: > what is the point of this rule? normally .hidden means display: none. Are you > overriding that somewhere? Ditto on CSS newbie comment. Fixed to display: none. (There does not appear to be a .hidden style already included in other .css files. Removal of this CSS entry causes class ="hidden" to fail in the HTML.) http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:208: if (e.target.checked) On 2012/07/28 01:01:16, Evan Stade wrote: > ternary imo I was always of the opinion that using a ternary to alternate between expression-statements was a sin, but if you mean to choose one of two functions to call, and then call the result, that's cool -- I'm just not used to that idiom yet. I modified it that way -- hope that's what you intended. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:313: * }} result Object giving metric ID, and time/value point pairs for On 2012/07/28 01:01:16, Evan Stade wrote: > an extra space between result and Object Done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:314: * that id. On 2012/07/28 01:01:16, Evan Stade wrote: > 4 more indent Done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:377: On 2012/07/28 01:01:16, Evan Stade wrote: > ? Not sure I understand the implied question -- sorry to be dense. Can you clarify? http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:393: * each event of that type in the range requested. On 2012/07/28 01:01:16, Evan Stade wrote: > ditto Done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:401: for (var i = 0; i < result.points.length; i++) { On 2012/07/28 01:01:16, Evan Stade wrote: > forEach Didn't see this in style rules (only the warning against for..in) Do we generally prefer forEach? I should change it other places too, if so. For this case, at least, done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:488: xaxis: {from: point.time, to: point.time} On 2012/07/28 01:01:16, Evan Stade wrote: > xAxis Sadly, this object is for use by Flot, which does not camel-capitalize, at least in this instance. Can't change the key name. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... chrome/browser/ui/webui/performance_monitor/web_ui.cc:25: source->add_resource_path("chart.css", IDR_PERFORMANCE_MONITOR_CHART_CSS); On 2012/07/28 01:01:16, Evan Stade wrote: > I don't think you need this these two (chart.*). Grit will inline them since you > have flattenhtml. I don't actually have flattenhtml on both, tho, so I added that. Any quick advice on when to use this option and when not? A "flattenhtml" search on the dev site produces no result and a general grep on the source tree doesn't show any inline docs on it (just lots of uses) Also, I found I got errors w/o the chart.js add_resource_path, even with flattenhtml. Was able to eliminate the .css call, though.
http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:6: float: left; don't use float for layout. It's mostly just useful for allowing text to flow around something. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:28: div.chart { On 2012/07/31 02:34:32, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > combine with the above > > > > also I doubt you need the tag name here either. > > Removed the tag name, but the #charts style applies to the div enclosing all > charts, of which there may eventually be more than just one. The .chart style > applies to each individual chart div within the #charts div. Added comments > clarifying this. in this case are you sure you need the height/width on #charts? http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:40: .hidden { On 2012/07/31 02:34:32, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > what is the point of this rule? normally .hidden means display: none. Are you > > overriding that somewhere? > > Ditto on CSS newbie comment. Fixed to display: none. > > (There does not appear to be a .hidden style already included in other .css > files. Removal of this CSS entry causes class ="hidden" to fail in the HTML.) > > ah. You should be using the hidden attribute, not a class. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:377: On 2012/07/31 02:34:32, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > ? > > Not sure I understand the implied question -- sorry to be dense. Can you > clarify? why is there a blank line here http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:401: for (var i = 0; i < result.points.length; i++) { On 2012/07/31 02:34:32, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > forEach > > Didn't see this in style rules (only the warning against for..in) Do we > generally prefer forEach? I should change it other places too, if so. > > For this case, at least, done. > I personally prefer forEach. I don't think it's in the style guide. for..in is frowned upon because it doesn't behave as you'd expect (it gives back keys, not values) http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:488: xaxis: {from: point.time, to: point.time} On 2012/07/31 02:34:32, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > xAxis > > Sadly, this object is for use by Flot, which does not camel-capitalize, at least > in this instance. Can't change the key name. ok http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... chrome/browser/ui/webui/performance_monitor/web_ui.cc:25: source->add_resource_path("chart.css", IDR_PERFORMANCE_MONITOR_CHART_CSS); On 2012/07/31 02:34:32, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > I don't think you need this these two (chart.*). Grit will inline them since > you > > have flattenhtml. > > I don't actually have flattenhtml on both, tho, so I added that. Any quick > advice on when to use this option and when not? A "flattenhtml" search on the > dev site produces no result and a general grep on the source tree doesn't show > any inline docs on it (just lots of uses) > > Also, I found I got errors w/o the chart.js add_resource_path, even with > flattenhtml. Was able to eliminate the .css call, though. flattenhtml inlines references. You only need flattenhtml on the html file. Otherwise what you have looks fine.
On 2012/07/31 22:57:01, Evan Stade wrote: > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.css (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.css:6: float: left; > don't use float for layout. It's mostly just useful for allowing text to flow > around something. > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.css:28: div.chart { > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > combine with the above > > > > > > also I doubt you need the tag name here either. > > > > Removed the tag name, but the #charts style applies to the div enclosing all > > charts, of which there may eventually be more than just one. The .chart style > > applies to each individual chart div within the #charts div. Added comments > > clarifying this. > > in this case are you sure you need the height/width on #charts? > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.css:40: .hidden { > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > what is the point of this rule? normally .hidden means display: none. Are > you > > > overriding that somewhere? > > > > Ditto on CSS newbie comment. Fixed to display: none. > > > > (There does not appear to be a .hidden style already included in other .css > > files. Removal of this CSS entry causes class ="hidden" to fail in the HTML.) > > > > > > ah. You should be using the hidden attribute, not a class. > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.js (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.js:377: > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > ? > > > > Not sure I understand the implied question -- sorry to be dense. Can you > > clarify? > > why is there a blank line here > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.js:401: for (var i = 0; i < > result.points.length; i++) { > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > forEach > > > > Didn't see this in style rules (only the warning against for..in) Do we > > generally prefer forEach? I should change it other places too, if so. > > > > For this case, at least, done. > > > > I personally prefer forEach. I don't think it's in the style guide. for..in is > frowned upon because it doesn't behave as you'd expect (it gives back keys, not > values) > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.js:488: xaxis: {from: > point.time, to: point.time} > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > xAxis > > > > Sadly, this object is for use by Flot, which does not camel-capitalize, at > least > > in this instance. Can't change the key name. > > ok > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... > File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... > chrome/browser/ui/webui/performance_monitor/web_ui.cc:25: > source->add_resource_path("chart.css", IDR_PERFORMANCE_MONITOR_CHART_CSS); > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > I don't think you need this these two (chart.*). Grit will inline them since > > you > > > have flattenhtml. > > > > I don't actually have flattenhtml on both, tho, so I added that. Any quick > > advice on when to use this option and when not? A "flattenhtml" search on the > > dev site produces no result and a general grep on the source tree doesn't show > > any inline docs on it (just lots of uses) > > > > Also, I found I got errors w/o the chart.js add_resource_path, even with > > flattenhtml. Was able to eliminate the .css call, though. > > flattenhtml inlines references. You only need flattenhtml on the html file. > Otherwise what you have looks fine. CSS files are also references (?), and the .grd file has a lot of flattenhtml for those. Or are you saying the flattenhtml is recursively applied to all things included by the HTML file?
On 2012/08/01 00:03:59, clintstaley wrote: > On 2012/07/31 22:57:01, Evan Stade wrote: > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > File chrome/browser/resources/performance_monitor/chart.css (right): > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.css:6: float: left; > > don't use float for layout. It's mostly just useful for allowing text to flow > > around something. > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.css:28: div.chart { > > On 2012/07/31 02:34:32, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > combine with the above > > > > > > > > also I doubt you need the tag name here either. > > > > > > Removed the tag name, but the #charts style applies to the div enclosing all > > > charts, of which there may eventually be more than just one. The .chart > style > > > applies to each individual chart div within the #charts div. Added comments > > > clarifying this. > > > > in this case are you sure you need the height/width on #charts? > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.css:40: .hidden { > > On 2012/07/31 02:34:32, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > what is the point of this rule? normally .hidden means display: none. Are > > you > > > > overriding that somewhere? > > > > > > Ditto on CSS newbie comment. Fixed to display: none. > > > > > > (There does not appear to be a .hidden style already included in other .css > > > files. Removal of this CSS entry causes class ="hidden" to fail in the > HTML.) > > > > > > > > > > ah. You should be using the hidden attribute, not a class. > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > File chrome/browser/resources/performance_monitor/chart.js (right): > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.js:377: > > On 2012/07/31 02:34:32, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > ? > > > > > > Not sure I understand the implied question -- sorry to be dense. Can you > > > clarify? > > > > why is there a blank line here > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.js:401: for (var i = 0; i < > > result.points.length; i++) { > > On 2012/07/31 02:34:32, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > forEach > > > > > > Didn't see this in style rules (only the warning against for..in) Do we > > > generally prefer forEach? I should change it other places too, if so. > > > > > > For this case, at least, done. > > > > > > > I personally prefer forEach. I don't think it's in the style guide. for..in is > > frowned upon because it doesn't behave as you'd expect (it gives back keys, > not > > values) > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.js:488: xaxis: {from: > > point.time, to: point.time} > > On 2012/07/31 02:34:32, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > xAxis > > > > > > Sadly, this object is for use by Flot, which does not camel-capitalize, at > > least > > > in this instance. Can't change the key name. > > > > ok > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... > > File chrome/browser/ui/webui/performance_monitor/web_ui.cc (right): > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/ui/webui/per... > > chrome/browser/ui/webui/performance_monitor/web_ui.cc:25: > > source->add_resource_path("chart.css", IDR_PERFORMANCE_MONITOR_CHART_CSS); > > On 2012/07/31 02:34:32, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > I don't think you need this these two (chart.*). Grit will inline them > since > > > you > > > > have flattenhtml. > > > > > > I don't actually have flattenhtml on both, tho, so I added that. Any quick > > > advice on when to use this option and when not? A "flattenhtml" search on > the > > > dev site produces no result and a general grep on the source tree doesn't > show > > > any inline docs on it (just lots of uses) > > > > > > Also, I found I got errors w/o the chart.js add_resource_path, even with > > > flattenhtml. Was able to eliminate the .css call, though. > > > > flattenhtml inlines references. You only need flattenhtml on the html file. > > Otherwise what you have looks fine. > > CSS files are also references (?), and the .grd file has a lot of flattenhtml > for those. Or are you saying the flattenhtml is recursively applied to all > things included by the HTML file? the latter. The css files that have flattenhtml are those with references like: content: url('../images/checkbox_white.png');
http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:6: float: left; On 2012/07/31 22:57:02, Evan Stade wrote: > don't use float for layout. It's mostly just useful for allowing text to flow > around something. I'd prefer not locking things down in absolute pixels, and I was going by the model of the ubiquitous 3-column layout. Should I use flexbox instead? http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:28: div.chart { On 2012/07/31 22:57:02, Evan Stade wrote: > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > combine with the above > > > > > > also I doubt you need the tag name here either. > > > > Removed the tag name, but the #charts style applies to the div enclosing all > > charts, of which there may eventually be more than just one. The .chart style > > applies to each individual chart div within the #charts div. Added comments > > clarifying this. > > in this case are you sure you need the height/width on #charts? Well, I am still a little mystified on how to get divs to actually wrap around their children. Enlightenment would be appreciated. If there are several charts in the future, the only alternative I can think of is to make the height of the #charts div be X*500px, and I'm thus leaving the #charts style as a placeholder for that. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:40: .hidden { On 2012/07/31 22:57:02, Evan Stade wrote: > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > what is the point of this rule? normally .hidden means display: none. Are > you > > > overriding that somewhere? > > > > Ditto on CSS newbie comment. Fixed to display: none. > > > > (There does not appear to be a .hidden style already included in other .css > > files. Removal of this CSS entry causes class ="hidden" to fail in the HTML.) > > > > > > ah. You should be using the hidden attribute, not a class. Done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:187: * @param {!string} divId Id of division into which to put checkboxes On 2012/07/28 01:01:16, Evan Stade wrote: > you can make this a more flexible function if you make this an actual div > instead of an id string. Done. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate = $('#checkboxTemplate')[0]; On 2012/07/28 01:01:16, Evan Stade wrote: > you don't need the hashtag. Hmm. This doesn't work for me (I get an undefined ref). That's a JQuery $, right? http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:377: On 2012/07/31 22:57:02, Evan Stade wrote: > On 2012/07/31 02:34:32, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > ? > > > > Not sure I understand the implied question -- sorry to be dense. Can you > > clarify? > > why is there a blank line here Done.
http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:6: float: left; On 2012/08/01 00:25:45, clintstaley wrote: > On 2012/07/31 22:57:02, Evan Stade wrote: > > don't use float for layout. It's mostly just useful for allowing text to flow > > around something. > > I'd prefer not locking things down in absolute pixels, and I was going by the > model of the ubiquitous 3-column layout. Should I use flexbox instead? > I don't actually know what the layout of this page looks like, but yes, flexbox is often the best bet. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate = $('#checkboxTemplate')[0]; On 2012/08/01 00:25:45, clintstaley wrote: > On 2012/07/28 01:01:16, Evan Stade wrote: > > you don't need the hashtag. > > Hmm. This doesn't work for me (I get an undefined ref). That's a JQuery $, > right? > it's from our util.js. Note all the places in resources/ that use $('') do not use hashtags.
On 2012/08/01 02:06:08, Evan Stade wrote: > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.css (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.css:6: float: left; > On 2012/08/01 00:25:45, clintstaley wrote: > > On 2012/07/31 22:57:02, Evan Stade wrote: > > > don't use float for layout. It's mostly just useful for allowing text to > flow > > > around something. > > > > I'd prefer not locking things down in absolute pixels, and I was going by the > > model of the ubiquitous 3-column layout. Should I use flexbox instead? > > > > I don't actually know what the layout of this page looks like, but yes, flexbox > is often the best bet. > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.js (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate > = $('#checkboxTemplate')[0]; > On 2012/08/01 00:25:45, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > you don't need the hashtag. > > > > Hmm. This doesn't work for me (I get an undefined ref). That's a JQuery $, > > right? > > > > it's from our util.js. Note all the places in resources/ that use $('') do not > use hashtags. Sorry, that one was rhetorical on my part. Unless I'm seriously mistaken, that's a JQuery $, supplied by Flot. Aaron and I looked closely at this issue, because the JQuery $, supplied by Flot, overrides the util.js, and we decided just to keep the JQuery one and use it. I do believe you *are* looking at a JQuery $, there, and I guess it doesnt default to id-notation the way our util.js does (?).
On 2012/08/01 02:06:08, Evan Stade wrote: > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.css (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.css:6: float: left; > On 2012/08/01 00:25:45, clintstaley wrote: > > On 2012/07/31 22:57:02, Evan Stade wrote: > > > don't use float for layout. It's mostly just useful for allowing text to > flow > > > around something. > > > > I'd prefer not locking things down in absolute pixels, and I was going by the > > model of the ubiquitous 3-column layout. Should I use flexbox instead? > > > > I don't actually know what the layout of this page looks like, but yes, flexbox > is often the best bet. > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.js (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate > = $('#checkboxTemplate')[0]; > On 2012/08/01 00:25:45, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > you don't need the hashtag. > > > > Hmm. This doesn't work for me (I get an undefined ref). That's a JQuery $, > > right? > > > > it's from our util.js. Note all the places in resources/ that use $('') do not > use hashtags. Sorry, that one was rhetorical on my part. Unless I'm seriously mistaken, that's a JQuery $, supplied by Flot. Aaron and I looked closely at this issue, because the JQuery $, supplied by Flot, overrides the util.js, and we decided just to keep the JQuery one and use it. I do believe you *are* looking at a JQuery $, there, and I guess it doesnt default to id-notation the way our util.js does (?).
http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate = $('#checkboxTemplate')[0]; On 2012/08/01 02:06:08, Evan Stade wrote: > On 2012/08/01 00:25:45, clintstaley wrote: > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > you don't need the hashtag. > > > > Hmm. This doesn't work for me (I get an undefined ref). That's a JQuery $, > > right? > > > > it's from our util.js. Note all the places in resources/ that use $('') do not > use hashtags. estade: these pages use a jQuery plugin (which requires jQuery obviously), so I told clientstaley to just use Sizzle instead of our custom $ alias in util.js. jQuery is also unfortunately aliased to $, so I can see the confusion, but in this case assuming this is jQuery (and we're going to keep it) he does need # in front of the ID as this is a selector (roughly equivalent to document.querySelector) not just an ID.
On 2012/08/01 18:20:20, Dan Beam wrote: > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.js (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.js:196: var checkboxTemplate > = $('#checkboxTemplate')[0]; > On 2012/08/01 02:06:08, Evan Stade wrote: > > On 2012/08/01 00:25:45, clintstaley wrote: > > > On 2012/07/28 01:01:16, Evan Stade wrote: > > > > you don't need the hashtag. > > > > > > Hmm. This doesn't work for me (I get an undefined ref). That's a JQuery $, > > > right? > > > > > > > it's from our util.js. Note all the places in resources/ that use $('') do not > > use hashtags. > > estade: these pages use a jQuery plugin (which requires jQuery obviously), so I > told clientstaley to just use Sizzle instead of our custom $ alias in util.js. > jQuery is also unfortunately aliased to $, so I can see the confusion, but in > this case assuming this is jQuery (and we're going to keep it) he does need # in > front of the ID as this is a selector (roughly equivalent to > document.querySelector) not just an ID. Thanks, Dan. And it was you and I, now that I recall, not Aaron and I, who discussed this.
Just posted latest, with all review comments incorporated. http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.css:6: float: left; On 2012/08/01 02:06:08, Evan Stade wrote: > On 2012/08/01 00:25:45, clintstaley wrote: > > On 2012/07/31 22:57:02, Evan Stade wrote: > > > don't use float for layout. It's mostly just useful for allowing text to > flow > > > around something. > > > > I'd prefer not locking things down in absolute pixels, and I was going by the > > model of the ubiquitous 3-column layout. Should I use flexbox instead? > > > > I don't actually know what the layout of this page looks like, but yes, flexbox > is often the best bet. Flexbox worked fine, much better than that float tinkertoy system. I can never get the latter to behave properly. I did need to do some flexbox nesting to get the left/right layout arrangement. If there's a better way to do that, I'd be happy to hear about it. Doesn't seem as though there's any flag to tell a *child* to move left or right. box-pack looks like a global at the parent level... ?
On 2012/08/01 21:37:40, clintstaley wrote: > Just posted latest, with all review comments incorporated. > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > File chrome/browser/resources/performance_monitor/chart.css (right): > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > chrome/browser/resources/performance_monitor/chart.css:6: float: left; > On 2012/08/01 02:06:08, Evan Stade wrote: > > On 2012/08/01 00:25:45, clintstaley wrote: > > > On 2012/07/31 22:57:02, Evan Stade wrote: > > > > don't use float for layout. It's mostly just useful for allowing text to > > flow > > > > around something. > > > > > > I'd prefer not locking things down in absolute pixels, and I was going by > the > > > model of the ubiquitous 3-column layout. Should I use flexbox instead? > > > > > > > I don't actually know what the layout of this page looks like, but yes, > flexbox > > is often the best bet. > > Flexbox worked fine, much better than that float tinkertoy system. I can never > get the latter to behave properly. > > I did need to do some flexbox nesting to get the left/right layout arrangement. > If there's a better way to do that, I'd be happy to hear about it. Doesn't seem > as though there's any flag to tell a *child* to move left or right. box-pack > looks like a global at the parent level... ? not 100% sure this is what you're asking, but I think that what you want is to put a placeholder div in the middle with flex: 1.
On 2012/08/02 01:00:29, Evan Stade wrote: > On 2012/08/01 21:37:40, clintstaley wrote: > > Just posted latest, with all review comments incorporated. > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > File chrome/browser/resources/performance_monitor/chart.css (right): > > > > > http://codereview.chromium.org/10820031/diff/8001/chrome/browser/resources/pe... > > chrome/browser/resources/performance_monitor/chart.css:6: float: left; > > On 2012/08/01 02:06:08, Evan Stade wrote: > > > On 2012/08/01 00:25:45, clintstaley wrote: > > > > On 2012/07/31 22:57:02, Evan Stade wrote: > > > > > don't use float for layout. It's mostly just useful for allowing text to > > > flow > > > > > around something. > > > > > > > > I'd prefer not locking things down in absolute pixels, and I was going by > > the > > > > model of the ubiquitous 3-column layout. Should I use flexbox instead? > > > > > > > > > > I don't actually know what the layout of this page looks like, but yes, > > flexbox > > > is often the best bet. > > > > Flexbox worked fine, much better than that float tinkertoy system. I can never > > get the latter to behave properly. > > > > I did need to do some flexbox nesting to get the left/right layout > arrangement. > > If there's a better way to do that, I'd be happy to hear about it. Doesn't > seem > > as though there's any flag to tell a *child* to move left or right. box-pack > > looks like a global at the parent level... ? > > not 100% sure this is what you're asking, but I think that what you want is to > put a placeholder div in the middle with flex: 1. Uh... Yeah.. I knew that :) Sorry for being dumb; obviously that's better than what I did.
All changes are in. Would like to get this one landed as soon as it deserves to be, as others in the team are awaiting it. (Our last CL in the alpha set for the new performance monitor feature.)
lgtm https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:44: * Offset, in mS, by which to subtract to convert GMT to local time isn't milliseconds abbreviated to ms?
https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... File chrome/browser/resources/performance_monitor/chart.css (right): https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.css:5: #chooseBlock { the style guide says this should be #choose-block https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.css:16: * dimensions if you redesign for more than one chart.*/ some people prefer not to use "we" or "you" in comments, additionally 1 \s between sentences in comments https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.css:24: .chart { why can't these two rules be combined until they differ, i.e. #charts, .charts { height: 500px; width: 900px; } ? https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... File chrome/browser/resources/performance_monitor/chart.html (right): https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.html:10: as of 6/2012.--> . --> (needs a space) https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.html:16: <script type="text/javascript" src="jquery.js"></script> I think you should omit [type] here or be consistent https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:57: * webui. The webui uses numbers to uniquely identify metric and event should these say "webui handler" instead of webui? https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:60: * @type {Object.<string, { I think wrapping for these long @type expressions is usually like this: * @type {!{blah: type, blee: type, blar: type, blag: type, * bluh: type, blug: type, blue: type}} https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:107: * allMetrics All metrics from which to select. nit: 4 \s instead of 2 \s before allMetrics https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:187: * @param {!HTMLDivElement} div div into which to put checkboxes Generally we capitalize after the parameter name and add a period at the end (implying the description should be a full sentence). @param {!HTMLDivElement} div A <div> into which to put checkboxes. (something like this) https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/ui/... File chrome/browser/ui/webui/performance_monitor/web_ui.h (right): https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/browser/ui/... chrome/browser/ui/webui/performance_monitor/web_ui.h:20: } // namespace performance_monitor nit: \n here, IMO https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10820031/diff/12013/chrome/chrome_brow... chrome/chrome_browser.gypi:3740: 'browser/ui/views/keyboard_overlay_delegate.cc', er, you seem to have leftover merge changes here, unless I'm misunderstanding...?
Fixes per dbeam's comments. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.css:5: #chooseBlock { On 2012/08/03 18:40:36, Dan Beam wrote: > the style guide says this should be #choose-block Sorry. Changed here and elsewhere. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.css:16: * dimensions if you redesign for more than one chart.*/ On 2012/08/03 18:40:36, Dan Beam wrote: > some people prefer not to use "we" or "you" in comments, additionally 1 \s > between sentences in comments Can change it if you really feel strongly, but my own style is to use these more familiar pronouns. The alternative of passive voice or "if one redesigns" doesn't seem as readable. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.css:24: .chart { On 2012/08/03 18:40:36, Dan Beam wrote: > why can't these two rules be combined until they differ, i.e. > > #charts, > .charts { > height: 500px; > width: 900px; > } > > ? Done. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.html:10: as of 6/2012.--> On 2012/08/03 18:40:36, Dan Beam wrote: > . --> (needs a space) Done. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.html:16: <script type="text/javascript" src="jquery.js"></script> On 2012/08/03 18:40:36, Dan Beam wrote: > I think you should omit [type] here or be consistent Done. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:44: * Offset, in mS, by which to subtract to convert GMT to local time On 2012/08/03 17:50:59, Evan Stade wrote: > isn't milliseconds abbreviated to ms? I've seen mS, and even msec, but happy to adjust to ms. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:57: * webui. The webui uses numbers to uniquely identify metric and event On 2012/08/03 18:40:36, Dan Beam wrote: > should these say "webui handler" instead of webui? Done. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:60: * @type {Object.<string, { On 2012/08/03 18:40:36, Dan Beam wrote: > I think wrapping for these long @type expressions is usually like this: > > * @type {!{blah: type, blee: type, blar: type, blag: type, > * bluh: type, blug: type, blue: type}} I've gotten different responses on this from different reviewers. This indentation, for instance, is the result of prior review. And, given the types in question,I don't think I can get the : lineup implied in your diagram with more than one type per line. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:107: * allMetrics All metrics from which to select. On 2012/08/03 18:40:36, Dan Beam wrote: > nit: 4 \s instead of 2 \s before allMetrics Done. http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.js:187: * @param {!HTMLDivElement} div div into which to put checkboxes On 2012/08/03 18:40:36, Dan Beam wrote: > Generally we capitalize after the parameter name and add a period at the end > (implying the description should be a full sentence). > > @param {!HTMLDivElement} div A <div> into which to put checkboxes. > > (something like this) Done, here and elsewhere. Of course, it's a sentence fragment either way, but it is certainly more readable as a noun phrase with the capital to separate it off, which I assume is the main idea (?). http://codereview.chromium.org/10820031/diff/12013/chrome/browser/ui/webui/pe... File chrome/browser/ui/webui/performance_monitor/web_ui.h (right): http://codereview.chromium.org/10820031/diff/12013/chrome/browser/ui/webui/pe... chrome/browser/ui/webui/performance_monitor/web_ui.h:20: } // namespace performance_monitor On 2012/08/03 18:40:36, Dan Beam wrote: > nit: \n here, IMO Done. http://codereview.chromium.org/10820031/diff/12013/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/10820031/diff/12013/chrome/chrome_browser.gypi... chrome/chrome_browser.gypi:3740: 'browser/ui/views/keyboard_overlay_delegate.cc', On 2012/08/03 18:40:36, Dan Beam wrote: > er, you seem to have leftover merge changes here, unless I'm > misunderstanding...? Not sure what happened here -- apologies. Fixed.
http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... chrome/browser/resources/performance_monitor/chart.css:16: * dimensions if you redesign for more than one chart.*/ On 2012/08/06 21:06:47, clintstaley wrote: > On 2012/08/03 18:40:36, Dan Beam wrote: > > some people prefer not to use "we" or "you" in comments, additionally 1 \s > > between sentences in comments > > > Can change it if you really feel strongly, but my own style is to use these more > familiar pronouns. The alternative of passive voice or "if one redesigns" > doesn't seem as readable. I don't care about the use of pronouns but it's pretty easy to avoid them without resorting to passive voice. "The dimensions would need to be larger if #charts contained more than one chart."
On 2012/08/07 00:46:35, Evan Stade wrote: > http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... > File chrome/browser/resources/performance_monitor/chart.css (right): > > http://codereview.chromium.org/10820031/diff/12013/chrome/browser/resources/p... > chrome/browser/resources/performance_monitor/chart.css:16: * dimensions if you > redesign for more than one chart.*/ > On 2012/08/06 21:06:47, clintstaley wrote: > > On 2012/08/03 18:40:36, Dan Beam wrote: > > > some people prefer not to use "we" or "you" in comments, additionally 1 \s > > > between sentences in comments > > > > > > Can change it if you really feel strongly, but my own style is to use these > more > > familiar pronouns. The alternative of passive voice or "if one redesigns" > > doesn't seem as readable. > > I don't care about the use of pronouns but it's pretty easy to avoid them > without resorting to passive voice. "The dimensions would need to be larger if > #charts contained more than one chart." Good point. I adjusted the comment accordingly.
http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.html:48: </div> move the <script>s from the <head> to right here and remove type="text/javascript" from chart.js IMO http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:5: 'use strict'; I think you want to put this 'use strict'; inside of the cr.define(), otherwise if this is ever included in another file that doesn't want to use ES5 strict there are (likely) errors. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:92: * data: ?Array.<{time: !number, longDescription: !string}> nit: ! in front of number|string|boolean|function is not necessary http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:117: * @param {Array.<{metricType: !String, shortDescription: !String}>} s/String/string/g (String = new String("blah"), string = "blah") http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:128: yAxis: {min: 0, max: metric.maxValue, color: 'rgb(0, 0, 255'}, color: 'rgba(0,0,255)' ^--- right? or simply ... 'blue' http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:150: color: 'rgb(255, 0, 0)', this could be 'red' and it'd probably be easier to read, but maybe these aren't the final colors... http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:410: console.log('Got event at time ' + element.time); remove http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:453: * @return {boolean} Is data ready? nit: Whether the data is ready. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:502: console.log('Event out of time range ' + this.start + ' -> ' + remove or make error or warning?
Back to you, Dan. Thanks for the review. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.html:48: </div> On 2012/08/15 20:53:53, Dan Beam wrote: > move the <script>s from the <head> to right here and remove > type="text/javascript" from chart.js IMO Done. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:5: 'use strict'; On 2012/08/15 20:53:53, Dan Beam wrote: > I think you want to put this 'use strict'; inside of the cr.define(), otherwise > if this is ever included in another file that doesn't want to use ES5 strict > there are (likely) errors. Done. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:92: * data: ?Array.<{time: !number, longDescription: !string}> On 2012/08/15 20:53:53, Dan Beam wrote: > nit: ! in front of number|string|boolean|function is not necessary I guess I need to understand this a bit better. number is a nullable type, right? String also ? I wanted to clarify that it wasn't OK to send back, for instance, a null for the tag of longDescription. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:117: * @param {Array.<{metricType: !String, shortDescription: !String}>} On 2012/08/15 20:53:53, Dan Beam wrote: > s/String/string/g (String = new String("blah"), string = "blah") Sorry. Too much Java. :) Fixed here and elsewhere. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:128: yAxis: {min: 0, max: metric.maxValue, color: 'rgb(0, 0, 255'}, On 2012/08/15 20:53:53, Dan Beam wrote: > color: 'rgba(0,0,255)' > ^--- right? > > or simply ... 'blue' That's a Flot property, not HTML. Flot wants rgb notation. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:150: color: 'rgb(255, 0, 0)', On 2012/08/15 20:53:53, Dan Beam wrote: > this could be 'red' and it'd probably be easier to read, but maybe these aren't > the final colors... Same answer, sadly. Flot requirement. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:410: console.log('Got event at time ' + element.time); On 2012/08/15 20:53:53, Dan Beam wrote: > remove We're still building this thing, so some log messages are going to be needed, but I dropped it for the CL anyway. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:453: * @return {boolean} Is data ready? On 2012/08/15 20:53:53, Dan Beam wrote: > nit: Whether the data is ready. Done. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:502: console.log('Event out of time range ' + this.start + ' -> ' + On 2012/08/15 20:53:53, Dan Beam wrote: > remove or make error or warning? Agreed, but would like to do this in the next iteration, since it will take a bit more to cover all the error conditions. We need the log for this (intermediate) CL.
lgtm w/nits http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:92: * data: ?Array.<{time: !number, longDescription: !string}> On 2012/08/16 00:20:25, clintstaley wrote: > On 2012/08/15 20:53:53, Dan Beam wrote: > > nit: ! in front of number|string|boolean|function is not necessary > > I guess I need to understand this a bit better. number is a nullable type, > right? String also ? I wanted to clarify that it wasn't OK to send back, for > instance, a null for the tag of longDescription. ! in front of number|boolean|string|function isn't necessary, and complex (non-builtin) types without ! in front adds an implicit (ComplexType|undefined) in their type list. https://developers.google.com/closure/compiler/docs/js-for-compiler#null http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:128: yAxis: {min: 0, max: metric.maxValue, color: 'rgb(0, 0, 255'}, On 2012/08/16 00:20:25, clintstaley wrote: > On 2012/08/15 20:53:53, Dan Beam wrote: > > color: 'rgba(0,0,255)' > > ^--- right? > > > > or simply ... 'blue' > > That's a Flot property, not HTML. Flot wants rgb notation. > a) you're missing an end ')', which is mainly what I meant b) jQuery.color.parse('blue') and jQuery.color.parse('red') both work just fine
lgtm for random chrome stuff
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10820031/8005
Try job failure for 10820031-8005 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10820031/8006
Try job failure for 10820031-8006 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10820031/4011
Try job failure for 10820031-4011 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
Fixes for dbeam's last few post-lgtm review comments. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:92: * data: ?Array.<{time: !number, longDescription: !string}> On 2012/08/16 00:32:42, Dan Beam wrote: > On 2012/08/16 00:20:25, clintstaley wrote: > > On 2012/08/15 20:53:53, Dan Beam wrote: > > > nit: ! in front of number|string|boolean|function is not necessary > > > > I guess I need to understand this a bit better. number is a nullable type, > > right? String also ? I wanted to clarify that it wasn't OK to send back, for > > instance, a null for the tag of longDescription. > > ! in front of number|boolean|string|function isn't necessary, and complex > (non-builtin) types without ! in front adds an implicit (ComplexType|undefined) > in their type list. > > https://developers.google.com/closure/compiler/docs/js-for-compiler#null Done. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:117: * @param {Array.<{metricType: !String, shortDescription: !String}>} On 2012/08/16 00:20:25, clintstaley wrote: > On 2012/08/15 20:53:53, Dan Beam wrote: > > s/String/string/g (String = new String("blah"), string = "blah") > > Sorry. Too much Java. :) Fixed here and elsewhere. Done. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:128: yAxis: {min: 0, max: metric.maxValue, color: 'rgb(0, 0, 255'}, On 2012/08/16 00:32:42, Dan Beam wrote: > On 2012/08/16 00:20:25, clintstaley wrote: > > On 2012/08/15 20:53:53, Dan Beam wrote: > > > color: 'rgba(0,0,255)' > > > ^--- right? > > > > > > or simply ... 'blue' > > > > That's a Flot property, not HTML. Flot wants rgb notation. > > > > a) you're missing an end ')', which is mainly what I meant > b) jQuery.color.parse('blue') and jQuery.color.parse('red') both work just fine Doh. Sorry to miss that. And thanks for the JQuery lesson. Both fixed now. http://codereview.chromium.org/10820031/diff/4007/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:150: color: 'rgb(255, 0, 0)', On 2012/08/16 00:20:25, clintstaley wrote: > On 2012/08/15 20:53:53, Dan Beam wrote: > > this could be 'red' and it'd probably be easier to read, but maybe these > aren't > > the final colors... > > Same answer, sadly. Flot requirement. > Done.
Yaron, welcome to the reviewers list :). I won't trouble you for a code review per se; Evan and Dan have done that already. I'm adding you due to the issue in the related Email, on which we'd value your advice. The Android redflag on the last CL leads to the details of the build break.
http://codereview.chromium.org/10820031/diff/4011/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/10820031/diff/4011/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:109: <include name="IDR_PERFORMANCE_MONITOR_CHART_CSS" file="resources\performance_monitor\chart.css" flattenhtml="true" type="BINDATA" /> Can you also exclude these resources from android by adding: <if expr="not pp_ifdef('android')"> http://codereview.chromium.org/10820031/diff/4011/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): http://codereview.chromium.org/10820031/diff/4011/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:217: if (url.host() == chrome::kChromeUIPerformanceMonitorHost) So I think this is causing your linker error on Android. While it's true that the code to refer to BrowserList was always there, the linker is smart enough to see that there's no access to the code and optimizes it away. You've made that code reachable and now there are missing symbols. You can move this to the !android list below
On 2012/08/17 20:08:19, Yaron wrote: > http://codereview.chromium.org/10820031/diff/4011/chrome/browser/browser_reso... > File chrome/browser/browser_resources.grd (right): >http://codereview.chromium.org/10820031/diff/4011/chrome/browser/browser_resources.grd#newcode109 > chrome/browser/browser_resources.grd:109: <include > name="IDR_PERFORMANCE_MONITOR_CHART_CSS" > file="resources\performance_monitor\chart.css" flattenhtml="true" type="BINDATA" > /> > Can you also exclude these resources from android by adding: > <if expr="not pp_ifdef('android')"> > > http://codereview.chromium.org/10820031/diff/4011/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > http://codereview.chromium.org/10820031/diff/4011/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:217: if (url.host() > == chrome::kChromeUIPerformanceMonitorHost) > So I think this is causing your linker error on Android. While it's true that > the code to refer to BrowserList was always there, the linker is smart enough to > see that there's no access to the code and optimizes it away. You've made that > code reachable and now there are missing symbols. You can move this to the > !android list below OK on the controller_factory and browser_resources.grd. Do you want us to add more of our related files to the sources! under Android in the chrome_browser.gypi? Or does that matter?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10820031/9007
Change committed as 152214 |
