Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
123713002:
[chromeos] Add an about:power UI. (Closed)
|
Created:
6 years, 11 months ago by Siva Chandra Modified:
6 years, 11 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, Sameer Nanda, Ami GONE FROM CHROMIUM Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[chromeos] Add an about:power UI.
This UI currently only displays battery charge percentage over time.
Later iterations will add more plots.
BUG=312956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243716
Patch Set 1 #
Total comments: 30
Patch Set 2 : Address comments #Patch Set 3 : Temporary upload #Patch Set 4 : Address comments #
Total comments: 54
Patch Set 5 : Address comments #
Total comments: 22
Patch Set 6 : Address comments, fix boundary conditions #Patch Set 7 : #
Messages
Total messages: 20 (0 generated)
https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/app/generated_... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/app/generated_... chrome/app/generated_resources.grd:15358: + <message name="IDS_POWER_TITLE" desc="Title of the page"> please give these more descriptive names so they won't conflict with other power-related strings in chrome. maybe e.g. "IDS_ABOUT_POWER_TITLE"? (there are already a bunch of IDS_ABOUT_VERSION" strings.) https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/power.html (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.html:18: <div> </div> https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:10: * corresponding data in plots was sampled. can you just store the timestamps in |plots| as well? why not just pass in |power_supply_array| directly? does the "in the past" here mean that these numbers are all negative relative to the current time? https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:13: * line graph with 'color'. document that this is ordered by ascending time https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:14: * @param {number} ymin A number representing the minimum of the values to instead of passing in the min and max values, can this function just find them from |plots|? https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:25: var rectColor = '#888888'; // A darker shade of grey nit: '#888', '#ccc'. don't think the comments are needed either https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:33: ctx.fillText('Enough data not available yet.', 15, 15); shouldn't this be a string resource? also, "Not enough data available yet." is better https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:63: PowerDataCollector::PowerSupplySnapshot snapshot = power_supply[i]; use "const PowerDataCollector::PowerSupplySnapshot& snapshot" to avoid a copy here https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:65: now.ToInternalValue() - power_supply[i].time.ToInternalValue()); you don't need ToInternalValue() here; just do "now - power_supply[i].time" https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:67: base::DictionaryValue *element = new base::DictionaryValue; nit: base::DictionaryValue* also, safer to use: scoped_ptr<base::DictionaryValue> element(new base::DictionaryValue); here and then use: data.Append(element.release()); below https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:69: element->SetInteger("time_past", time_delta.InSeconds()); nit: seconds_past?
Responded to all comments. Also fixed how time is calculated on the javascript side. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/app/generated_... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/app/generated_... chrome/app/generated_resources.grd:15358: + <message name="IDS_POWER_TITLE" desc="Title of the page"> On 2014/01/03 16:40:47, Daniel Erat wrote: > please give these more descriptive names so they won't conflict with other > power-related strings in chrome. maybe e.g. "IDS_ABOUT_POWER_TITLE"? (there are > already a bunch of IDS_ABOUT_VERSION" strings.) Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/power.html (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.html:18: <div> On 2014/01/03 16:40:47, Daniel Erat wrote: > </div> Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:10: * corresponding data in plots was sampled. On 2014/01/03 16:40:47, Daniel Erat wrote: > can you just store the timestamps in |plots| as well? Do you mean like this: plots = { tdata, // Array of time stamps plot_data // Array of objects = { color, ydata // Array of values } }; The way it is currently, |plots| is equal to |plot_data| above. > why not just pass in > |power_supply_array| directly? Passing |power_supply_array| directly would tie this function to battery charge info. I want to keep this function generic enough so that it will be used by future metrics (which would also be sampled over time). > does the "in the past" here mean that these numbers are all negative relative to > the current time? The values are not negative as the name has 'past' in it. So, it is N (+ve number) seconds in the past. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:13: * line graph with 'color'. On 2014/01/03 16:40:47, Daniel Erat wrote: > document that this is ordered by ascending time Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:14: * @param {number} ymin A number representing the minimum of the values to On 2014/01/03 16:40:47, Daniel Erat wrote: > instead of passing in the min and max values, can this function just find them > from |plots|? I agree. But the purpose here is to have the flexibility to show more range when desired than to just show the min and max found in the data. For battery charge for example, I think is better to show the range 0-100 than just what is in the data. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:25: var rectColor = '#888888'; // A darker shade of grey On 2014/01/03 16:40:47, Daniel Erat wrote: > nit: '#888', '#ccc'. don't think the comments are needed either Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:33: ctx.fillText('Enough data not available yet.', 15, 15); On 2014/01/03 16:40:47, Daniel Erat wrote: > shouldn't this be a string resource? > > also, "Not enough data available yet." is better Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:63: PowerDataCollector::PowerSupplySnapshot snapshot = power_supply[i]; On 2014/01/03 16:40:47, Daniel Erat wrote: > use "const PowerDataCollector::PowerSupplySnapshot& snapshot" to avoid a copy > here Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:65: now.ToInternalValue() - power_supply[i].time.ToInternalValue()); On 2014/01/03 16:40:47, Daniel Erat wrote: > you don't need ToInternalValue() here; just do "now - power_supply[i].time" Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:67: base::DictionaryValue *element = new base::DictionaryValue; On 2014/01/03 16:40:47, Daniel Erat wrote: > nit: base::DictionaryValue* > > also, safer to use: > > scoped_ptr<base::DictionaryValue> element(new base::DictionaryValue); > > here and then use: > > data.Append(element.release()); > > below Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/ui/web... chrome/browser/ui/webui/chromeos/power_ui.cc:69: element->SetInteger("time_past", time_delta.InSeconds()); On 2014/01/03 16:40:47, Daniel Erat wrote: > nit: seconds_past? Done.
(haven't looked at the new version yet; just some replies to the previous comment) also, can you post a screenshot somewhere of what this looks like? https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:10: * corresponding data in plots was sampled. On 2014/01/03 23:34:59, Siva Chandra wrote: > On 2014/01/03 16:40:47, Daniel Erat wrote: > > can you just store the timestamps in |plots| as well? > > Do you mean like this: > > plots = { > tdata, // Array of time stamps > plot_data // Array of objects = { color, ydata // Array of values } > }; > > The way it is currently, |plots| is equal to |plot_data| above. > > > why not just pass in > > |power_supply_array| directly? > > Passing |power_supply_array| directly would tie this function to battery charge > info. I want to keep this function generic enough so that it will be used by > future metrics (which would also be sampled over time). > > > does the "in the past" here mean that these numbers are all negative relative > to > > the current time? > > The values are not negative as the name has 'past' in it. So, it is N (+ve > number) seconds in the past. no, i meant something more like: lines = [ { data: [ [ <time as milliseconds since the epoch, e.g. Date.ToUTC()>, <y-value> ], [ <time>, <y-value> ], etc. ], color: "#456" }, { data: [ [ <time>, <y-value> ], [ <time>, <y-value> ] ], color: "#856" }, etc. ] several points: - avoids the possibility of |tdata| and |plots| being different sizes - not restricted to only graphing lines that were sampled at the same times - can later display timestamps on the graphs if you choose to - absolute timestamps also mean that you can display old data later without needing to track what the timestamps were relative to https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:14: * @param {number} ymin A number representing the minimum of the values to On 2014/01/03 23:34:59, Siva Chandra wrote: > On 2014/01/03 16:40:47, Daniel Erat wrote: > > instead of passing in the min and max values, can this function just find them > > from |plots|? > > I agree. But the purpose here is to have the flexibility to show more range when > desired than to just show the min and max found in the data. For battery charge > for example, I think is better to show the range 0-100 than just what is in the > data. sure, makes sense. can you update the comments to make it more clear that these hold the bounds of the graph? "the minimum of the values" made me think that it refers to the values that are being graphed. how about, "Minimum bound of the y-axis" and "Maximum bound of the y-axis"? https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:37: function yDataToString(index) { i don't understand the use of the word "index" in this method and some of the following ones. it usually refers to e.g. an index into an array, but it seems like that's not the case here. would something like "value" be more appropriate, like: function valueToString(value) { return Number(value).toPrecision(yprecision); } ? https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:165: color: '#0000FF', // Blue color. nit: indent two spaces instead of 4
I will address the comments soon. I have a point for one comment below.
On 2014/01/04 00:02:16, Daniel Erat wrote:
> no, i meant something more like:
>
> lines = [
> {
> data: [
> [ <time as milliseconds since the epoch, e.g. Date.ToUTC()>, <y-value>
],
> [ <time>, <y-value> ],
> etc.
> ],
> color: "#456"
> },
> {
> data: [
> [ <time>, <y-value> ],
> [ <time>, <y-value> ]
> ],
> color: "#856"
> },
> etc.
> ]
At this point, I am really not sure which power metric would require multiple
data to be shown on a single plot. However, there is one which I do see would be
useful if shown on a single plot: It is probably more useful to show the
occupancy for all processor C-states on a single plot. So, I was setting up the
function in question here for such a use case where the occupancy of each
c-state would all be sampled at the same time. Hence, packing the time with each
sample is probably a memory overhead. Not only that, the plotting logic will be
that much more complex if we cater to mutiple data not all sampled together.
Unless you feel that we should support this generality right away, I would
prefer putting it off until we find a power metric which requires this
generality.
On 2014/01/04 00:29:04, Siva Chandra wrote:
> I will address the comments soon. I have a point for one comment below.
>
> On 2014/01/04 00:02:16, Daniel Erat wrote:
> > no, i meant something more like:
> >
> > lines = [
> > {
> > data: [
> > [ <time as milliseconds since the epoch, e.g. Date.ToUTC()>, <y-value>
> ],
> > [ <time>, <y-value> ],
> > etc.
> > ],
> > color: "#456"
> > },
> > {
> > data: [
> > [ <time>, <y-value> ],
> > [ <time>, <y-value> ]
> > ],
> > color: "#856"
> > },
> > etc.
> > ]
>
> At this point, I am really not sure which power metric would require multiple
> data to be shown on a single plot. However, there is one which I do see would
be
> useful if shown on a single plot: It is probably more useful to show the
> occupancy for all processor C-states on a single plot. So, I was setting up
the
> function in question here for such a use case where the occupancy of each
> c-state would all be sampled at the same time. Hence, packing the time with
each
> sample is probably a memory overhead. Not only that, the plotting logic will
be
> that much more complex if we cater to mutiple data not all sampled together.
> Unless you feel that we should support this generality right away, I would
> prefer putting it off until we find a power metric which requires this
> generality.
sounds okay to me (although i am doubtful that the overhead of passing a
timestamp for each point would matter in any meaningful way).
please document that the timestamps array and each timeseries must have the same
number of values (and check this within the code).
PTaL at the latest patch set. Addressed all comments. Screenshot: http://tinypic.com/r/fawm1/5 https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:10: * corresponding data in plots was sampled. On 2014/01/04 00:02:17, Daniel Erat wrote: > On 2014/01/03 23:34:59, Siva Chandra wrote: > > On 2014/01/03 16:40:47, Daniel Erat wrote: > > > can you just store the timestamps in |plots| as well? > > > > Do you mean like this: > > > > plots = { > > tdata, // Array of time stamps > > plot_data // Array of objects = { color, ydata // Array of values } > > }; > > > > The way it is currently, |plots| is equal to |plot_data| above. > > > > > why not just pass in > > > |power_supply_array| directly? > > > > Passing |power_supply_array| directly would tie this function to battery > charge > > info. I want to keep this function generic enough so that it will be used by > > future metrics (which would also be sampled over time). > > > > > does the "in the past" here mean that these numbers are all negative > relative > > to > > > the current time? > > > > The values are not negative as the name has 'past' in it. So, it is N (+ve > > number) seconds in the past. > > no, i meant something more like: > > lines = [ > { > data: [ > [ <time as milliseconds since the epoch, e.g. Date.ToUTC()>, <y-value> ], > [ <time>, <y-value> ], > etc. > ], > color: "#456" > }, > { > data: [ > [ <time>, <y-value> ], > [ <time>, <y-value> ] > ], > color: "#856" > }, > etc. > ] > > several points: > - avoids the possibility of |tdata| and |plots| being different sizes > - not restricted to only graphing lines that were sampled at the same times > - can later display timestamps on the graphs if you choose to > - absolute timestamps also mean that you can display old data later without > needing to track what the timestamps were relative to Discussion about this is present elsewhere. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:14: * @param {number} ymin A number representing the minimum of the values to On 2014/01/04 00:02:17, Daniel Erat wrote: > On 2014/01/03 23:34:59, Siva Chandra wrote: > > On 2014/01/03 16:40:47, Daniel Erat wrote: > > > instead of passing in the min and max values, can this function just find > them > > > from |plots|? > > > > I agree. But the purpose here is to have the flexibility to show more range > when > > desired than to just show the min and max found in the data. For battery > charge > > for example, I think is better to show the range 0-100 than just what is in > the > > data. > > sure, makes sense. can you update the comments to make it more clear that these > hold the bounds of the graph? "the minimum of the values" made me think that it > refers to the values that are being graphed. how about, "Minimum bound of the > y-axis" and "Maximum bound of the y-axis"? Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:37: function yDataToString(index) { On 2014/01/04 00:02:17, Daniel Erat wrote: > i don't understand the use of the word "index" in this method and some of the > following ones. it usually refers to e.g. an index into an array, but it seems > like that's not the case here. would something like "value" be more appropriate, > like: > > function valueToString(value) { > return Number(value).toPrecision(yprecision); > } > > ? Done. https://chromiumcodereview.appspot.com/123713002/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/power.js:165: color: '#0000FF', // Blue color. On 2014/01/04 00:02:17, Daniel Erat wrote: > nit: indent two spaces instead of 4 Done.
Forgot to mention, the latest patch set adds a mouse feature. If a user hovers over the plot, then a vertical time guide is displayed with the point on the plot highlighted. The screenshot shows this.
+cc fischman@
(Adding Erik as a reviewer for the JS code.) https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... File chrome/browser/resources/chromeos/power.html (right): https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.html:11: <head> </head> (if this was copied from somewhere that also got this wrong, please update that other location too) https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:6: function getTimeString(timePast) { timePast -> secPast https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:40: ctx.fillText(text, 15, 15); move these "15" magic numbers into an "errorOffsetPixels" or similar variable https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:61: return ctx.measureText(text).width + 2; why +2? (i.e. please add a comment) https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:64: function drawText(text, x, y) { move this up and make printErrorText() use it https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:222: function onMouseOut(event) { use canvas.addEventListener('mouseout', function(event) { ... https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:228: canvas.onmousemove = onMouseOverOrMove; use addEventListener for these too https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:239: function showBatteryChargeData(power_supply_array) { powerSupplyArray
https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... File chrome/browser/resources/chromeos/power.css (right): https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.css:20: div.plot-canvas-div { No need for the tag name here and below https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... File chrome/browser/resources/chromeos/power.html (right): https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.html:5: <title id="power" i18n-content="titleText"></title> This ID is not needed. You can do `document.title = str` from your code. https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.html:17: <canvas id="battery-charge-canvas" width=600 height=200></canvas> Missing quotes around the attributes https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... File chrome/browser/resources/chromeos/power.js (right): https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:15: * @param {DOMElement} canvas The canvas on which the line graph is drawn. Invalid type. Do you mean {Element} or maybe more specific {HTMLCanvasElement}? https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:16: * @param {Array.integer} tdata The time (in seconds) in the past when the Array.<number> JS only has one number type https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:29: function plotLineGraph(canvas, tdata, plots, ymin, ymax, yprecision) { tdata -> tData or maybe timeData? ymax -> yMax ... https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:33: var gridColor = '#CCC'; #ccc https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:38: ctx.clearRect(0, 0, canvas.scrollWidth, canvas.scrollHeight); canvas.width and height is more idiomatic https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:48: for (var count in plots) { Don't use for-in loops over arrays. Stick to plain old c style loops. https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:55: function valueToString(value) { i18n? Why not use Intl for all your number to string needs? https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:80: var colorBackup = ctx.strokeStyle; ctx.save() ... ctx.restore() https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:92: function drawRect(x, y, width, height, color) { Is there a reason why this (and other functions) are local functions of plotLineGraph? https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:157: for (var i = 0; i < size; ++i) { prefer i++ for consistency https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:208: var x = event.clientX - canvas.offsetLeft + canvas.parentNode.scrollLeft; Maybe use canvas.getBoundingClientRect() instead? It would simplify the code and make it less error prone. https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:227: canvas.onmouseover = onMouseOverOrMove; Do you need touch events too? https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:239: function showBatteryChargeData(power_supply_array) { no underscores in js https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:241: tdata = new Array(); tdata = []; https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:248: plots = new Array(); plots = [ { color: ... } ]; https://codereview.chromium.org/123713002/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/power.js:260: var PowerUI = { Only classes/constructors should be capitalized.
PTaL at patch set 5. I have posted few questions against arv's comments. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... File chrome/browser/resources/chromeos/power.html (right): https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.html:11: <head> On 2014/01/06 23:02:41, Daniel Erat wrote: > </head> > > (if this was copied from somewhere that also got this wrong, please update that > other location too) Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.html:17: <canvas id="battery-charge-canvas" width=600 height=200></canvas> On 2014/01/06 23:29:57, arv wrote: > Missing quotes around the attributes Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:6: function getTimeString(timePast) { On 2014/01/06 23:02:41, Daniel Erat wrote: > timePast -> secPast Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:15: * @param {DOMElement} canvas The canvas on which the line graph is drawn. On 2014/01/06 23:29:57, arv wrote: > Invalid type. Do you mean {Element} or maybe more specific {HTMLCanvasElement}? Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:16: * @param {Array.integer} tdata The time (in seconds) in the past when the On 2014/01/06 23:29:57, arv wrote: > Array.<number> > > JS only has one number type Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:29: function plotLineGraph(canvas, tdata, plots, ymin, ymax, yprecision) { On 2014/01/06 23:29:57, arv wrote: > tdata -> tData or maybe timeData? > > ymax -> yMax > > ... Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:33: var gridColor = '#CCC'; On 2014/01/06 23:29:57, arv wrote: > #ccc Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:38: ctx.clearRect(0, 0, canvas.scrollWidth, canvas.scrollHeight); On 2014/01/06 23:29:57, arv wrote: > canvas.width and height is more idiomatic Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:40: ctx.fillText(text, 15, 15); On 2014/01/06 23:02:41, Daniel Erat wrote: > move these "15" magic numbers into an "errorOffsetPixels" or similar variable Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:48: for (var count in plots) { On 2014/01/06 23:29:57, arv wrote: > Don't use for-in loops over arrays. Stick to plain old c style loops. Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:55: function valueToString(value) { On 2014/01/06 23:29:57, arv wrote: > i18n? > > Why not use Intl for all your number to string needs? How to i18n-ize numbers from within a js chrome resource? https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:61: return ctx.measureText(text).width + 2; On 2014/01/06 23:02:41, Daniel Erat wrote: > why +2? > > (i.e. please add a comment) Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:64: function drawText(text, x, y) { On 2014/01/06 23:02:41, Daniel Erat wrote: > move this up and make printErrorText() use it Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:80: var colorBackup = ctx.strokeStyle; On 2014/01/06 23:29:57, arv wrote: > ctx.save() > ... > ctx.restore() Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:92: function drawRect(x, y, width, height, color) { On 2014/01/06 23:29:57, arv wrote: > Is there a reason why this (and other functions) are local functions of > plotLineGraph? I am viewing this as a pseudo plotting object which "manages" itself. That is, when invoked, it creates a 'state' and operates on the state. If I make the functions global, we will still be doing the same, but will require to pass this state to each of the helper functions. Also, I do not see a need to make the helpers global. They are "private" to this pseudo class. All other functions should only be calling the main plotting function. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:157: for (var i = 0; i < size; ++i) { On 2014/01/06 23:29:57, arv wrote: > prefer i++ for consistency Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:208: var x = event.clientX - canvas.offsetLeft + canvas.parentNode.scrollLeft; On 2014/01/06 23:29:57, arv wrote: > Maybe use canvas.getBoundingClientRect() instead? It would simplify the code and > make it less error prone. Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:222: function onMouseOut(event) { On 2014/01/06 23:02:41, Daniel Erat wrote: > use canvas.addEventListener('mouseout', function(event) { ... Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:227: canvas.onmouseover = onMouseOverOrMove; On 2014/01/06 23:29:57, arv wrote: > Do you need touch events too? Single touch events are already captured is it not? I tested it on pixel and it works. Sliding, I am not sure we want it. Do we need to do something special to enable/disable/handle? https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:228: canvas.onmousemove = onMouseOverOrMove; On 2014/01/06 23:02:41, Daniel Erat wrote: > use addEventListener for these too Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:239: function showBatteryChargeData(power_supply_array) { On 2014/01/06 23:02:41, Daniel Erat wrote: > powerSupplyArray Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:239: function showBatteryChargeData(power_supply_array) { On 2014/01/06 23:29:57, arv wrote: > no underscores in js Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:241: tdata = new Array(); On 2014/01/06 23:29:57, arv wrote: > tdata = []; Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:248: plots = new Array(); On 2014/01/06 23:29:57, arv wrote: > plots = [ > { > color: ... > } > ]; Done. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:260: var PowerUI = { On 2014/01/06 23:29:57, arv wrote: > Only classes/constructors should be capitalized. Done.
https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:55: function valueToString(value) { On 2014/01/07 01:19:19, Siva Chandra wrote: > On 2014/01/06 23:29:57, arv wrote: > > i18n? > > > > Why not use Intl for all your number to string needs? > > How to i18n-ize numbers from within a js chrome resource? Intl is part of JavaScript and it provides locale aware number formatting. I'm not sure what https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Something along the lines of: new Intl.NumberFormat('fr-CA', {style: 'decimal', maximumSignificantDigits: 2}).format(1.234567) I'm not sure you really care about this though?
https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:112: var yhalfStr = valueToString((yMax + yMin) / 2); nit: yHalfStr https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:115: var yhalfWidth = getTextWidth(yhalfStr); nit: yHalfWidth https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:117: var xminStr = tData[0]; xMinStr, xMaxStr, xMinWidth, xMaxWidth https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:122: var xorigin = padding + Math.max(yMinWidth, yMaxWidth, xminWidth / 2); xOrigin, yOrigin https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:162: var yvalRange = yMax - yMin; yValRange https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:165: var ydata = plot.data; yData https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:169: var xpos = xorigin + Math.floor(i / (size - 1) * (width - 1)); xPos https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:171: var ypos = yorigin + height - 1 - yPos https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:196: var ypos = yorigin + height - 1 - yPos https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:203: var yloc; yLoc https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/u... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/u... chrome/browser/ui/webui/chromeos/power_ui.cc:64: base::TimeDelta time_delta = now - power_supply[i].time; i still have a strong preference for passing absolute time values here rather than offsets from the present time: using x values that actually *decrease* as you move to the right along the x-axis seems strange, and i also don't like seeing the javascript code receive offsets that aren't tethered to any particular point in time. you could convert the recorded times to JS's native time format here with: base::Time time = base::Time::Now() - (base::TimeTicks::Now() - power_supply[i].time); element->SetDouble("time", time.ToJsTime()); and then in the JS code just do: new Date(sample.time)
PTaL at patch set 6. Addressed all comments. I tested it on a dev device running overnight. It exposed a few border condition bugs which I have now fixed. I am sufficiently confident about the plotting logic now. https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/270001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:55: function valueToString(value) { On 2014/01/07 15:23:54, arv wrote: > On 2014/01/07 01:19:19, Siva Chandra wrote: > > On 2014/01/06 23:29:57, arv wrote: > > > i18n? > > > > > > Why not use Intl for all your number to string needs? > > > > How to i18n-ize numbers from within a js chrome resource? > > Intl is part of JavaScript and it provides locale aware number formatting. I'm > not sure what > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... > > Something along the lines of: > > new Intl.NumberFormat('fr-CA', {style: 'decimal', maximumSignificantDigits: > 2}).format(1.234567) > > I'm not sure you really care about this though? I am tempted to say i18n for numbers does not matter/is not required. None of the other chromeos JS scripts use it for numbers. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:112: var yhalfStr = valueToString((yMax + yMin) / 2); On 2014/01/07 16:31:58, Daniel Erat wrote: > nit: yHalfStr Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:115: var yhalfWidth = getTextWidth(yhalfStr); On 2014/01/07 16:31:58, Daniel Erat wrote: > nit: yHalfWidth Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:117: var xminStr = tData[0]; On 2014/01/07 16:31:58, Daniel Erat wrote: > xMinStr, xMaxStr, xMinWidth, xMaxWidth Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:122: var xorigin = padding + Math.max(yMinWidth, yMaxWidth, xminWidth / 2); On 2014/01/07 16:31:58, Daniel Erat wrote: > xOrigin, yOrigin Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:162: var yvalRange = yMax - yMin; On 2014/01/07 16:31:58, Daniel Erat wrote: > yValRange Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:165: var ydata = plot.data; On 2014/01/07 16:31:58, Daniel Erat wrote: > yData Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:169: var xpos = xorigin + Math.floor(i / (size - 1) * (width - 1)); On 2014/01/07 16:31:58, Daniel Erat wrote: > xPos Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:171: var ypos = yorigin + height - 1 - On 2014/01/07 16:31:58, Daniel Erat wrote: > yPos Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:196: var ypos = yorigin + height - 1 - On 2014/01/07 16:31:58, Daniel Erat wrote: > yPos Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:203: var yloc; On 2014/01/07 16:31:58, Daniel Erat wrote: > yLoc Done. https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/u... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/123713002/diff/380001/chrome/browser/u... chrome/browser/ui/webui/chromeos/power_ui.cc:64: base::TimeDelta time_delta = now - power_supply[i].time; On 2014/01/07 16:31:58, Daniel Erat wrote: > i still have a strong preference for passing absolute time values here rather > than offsets from the present time: using x values that actually *decrease* as > you move to the right along the x-axis seems strange, and i also don't like > seeing the javascript code receive offsets that aren't tethered to any > particular point in time. > > you could convert the recorded times to JS's native time format here with: > > base::Time time = base::Time::Now() - > (base::TimeTicks::Now() - power_supply[i].time); > element->SetDouble("time", time.ToJsTime()); > > and then in the JS code just do: > > new Date(sample.time) Done.
Thanks! LGTM if arv@ is happy with it too.
LGTM
*.grd LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/123713002/520001
Message was sent while issue was closed.
Change committed as 243716 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
