Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10900035:
Version 2 of CPM UI, with rearranged controls, multiple graphs, event rollovers, etc. (Closed)
|
Created:
8 years, 3 months ago by clintstaley Modified:
8 years, 3 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionVersion 2 of CPM UI, with rearranged controls, multiple graphs, event rollovers, etc.
http://imgur.com/yvJww
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156246
Patch Set 1 #
Total comments: 43
Patch Set 2 : Fixes per jyasskin's comments #
Total comments: 12
Patch Set 3 : Latest fixes per jyasskin's review #
Total comments: 19
Patch Set 4 : Latest changes per jyasskin and dcronin #
Total comments: 2
Patch Set 5 : Small comment fixes #Patch Set 6 : Fixes for dumb timer config errors #Patch Set 7 : Presubmit fixes #
Messages
Total messages: 18 (0 generated)
New UI for CPM, to go along with Devlin's recent CL on the C++ side.
http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.css:7: background: rgb(238, 238, 255); Why rgb(...) instead of #eef? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.html:19: <div id="choose-time-range-header" class="tab-label"> Why <div class="tab-label"> instead of <h1>? (Accessibility should be helped by using the standard elements, but maybe that's not how we do things in extensions.) http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.html:27: <button id="back-time"><<</button> title="Back"? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.html:64: <script src="chrome://resources/js/cr.js"></script> Why move the scripts inside the div? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:9: * Enum for time ranges, giving a descriptive name, time span prior to |now|, Mention that time span and resolution are in milliseconds, that resolution is the amount of time between data points, and that the label frequency is in number-of-data-points. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:10: * data point resolution, and time-label frequency and format for each. Where's the definition for symbols in the format string? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:11: * @enum {{ It looks like these objects also grow an 'element' field after setupTimeRangeTab_ is called. It would be good to document that here. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:98: var resizeDelay_ = 500; 500ms is well inside human perception. Did you mean this to feel like a delay? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:112: * metricCategoryId: number, What sort of number? Is there a list of possible values, or are they made up by another part of Chrome? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:116: * details: !Array.<!Object>, Please describe the details objects. You might want to use an @typedef for the whole value type of this map, since you use it again in getMetricTypesCallback. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:173: * event mouseover. These will vary per event type. Are the event types described somewhere? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:207: this.resizeTimer_ = setInterval(this.checkResize_.bind(this), 100); I would probably schedule a call to checkResize from the resize event, instead of polling it every 100ms while the user isn't resizing the page. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:250: for (var c = 0; c < categories.length; c++) { Use categories.forEach(function(category) { ... }, this) ? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:256: for (var m = 0; m < category.details.length; m++) { Use category.details.forEach(function(metric) { ... }, this) ? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:259: metric.color = ColorTable_[metric.metricId % ColorTable_.length]; Where's it documented that metricId is densely allocated? http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:345: * and a map |optionCategoryMap| with values that each include a property This might be more clear if you write down an example value for |optionCategoryMap|. Trying to describe types in English rarely goes well. ;) http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:346: * |description|, and a property |details|, which gives an array of grammar: "gives an array of that in turn" http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:349: * Set up a checkbox group for each entry in |optionCategoryMap|, which Similarly, it might work better to write some sample output HTML here. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:387: }.bind(this)); Another option to replace this bind() call, and the ", this" argument to Array.forEach(), is to put "var self=this;" at the top of the method, and then refer to 'self' instead of 'this' in nested functions. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:401: * @param {!Object} category The metric category for which to create Name the type |category| is supposed to have.
Fixes in, Jeff. Back to you. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... File chrome/browser/resources/performance_monitor/chart.css (right): http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.css:7: background: rgb(238, 238, 255); On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Why rgb(...) instead of #eef? The automated style checker dinged me for the #eef, saying I should use rgb notation for non-gray shades. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... File chrome/browser/resources/performance_monitor/chart.html (right): http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.html:19: <div id="choose-time-range-header" class="tab-label"> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Why <div class="tab-label"> instead of <h1>? (Accessibility should be helped by > using the standard elements, but maybe that's not how we do things in > extensions.) We're still working on that left-bar, and among other things may be turning it into an accordion. Seems useful to be able to identify tags that are tab labels. The accessibility point is something I hadn't thought of, tho. Will bear this in mind and prefer standard tags when possible. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.html:27: <button id="back-time"><<</button> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > title="Back"? Accessiblity? Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.html:64: <script src="chrome://resources/js/cr.js"></script> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Why move the scripts inside the div? Sloppiness.:) Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:9: * Enum for time ranges, giving a descriptive name, time span prior to |now|, On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Mention that time span and resolution are in milliseconds, that resolution is > the amount of time between data points, and that the label frequency is in > number-of-data-points. Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:10: * data point resolution, and time-label frequency and format for each. On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Where's the definition for symbols in the format string? Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:11: * @enum {{ On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > It looks like these objects also grow an 'element' field after > setupTimeRangeTab_ is called. It would be good to document that here. Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:98: var resizeDelay_ = 500; On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > 500ms is well inside human perception. Did you mean this to feel like a delay? No, I meant it to *not* feel like a delay. If there's a better approach to this problem, I'd love to learn it. Apparently HTML resize events, at least in most browsers, come in a firehose, one event per mouse movement as the window is dragged, without an easy way to determine when the actual mouseup-indicated resize event occurs. Solutions on the web are like this one -- "debouncing" the event stream by waiting a bit since the last event, on the assumption that means the user is "done" resizing. I wanted to wait as long as I could without irritating the user. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:112: * metricCategoryId: number, On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > What sort of number? Is there a list of possible values, or are they made up by > another part of Chrome? Made up on the C++ side, as values of an enum. Didn't want to be specific as to the C++ implementation, as long as the numbers were unique. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:116: * details: !Array.<!Object>, On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Please describe the details objects. You might want to use an @typedef for the > whole value type of this map, since you use it again in getMetricTypesCallback. Not quite, actually. The result of getMetricTypesCallback is undecorated. These are decorated. But, I could use an @typedef for the decorated metric details, which I will do. (A git grep suggests I'm the sixth one in the entire Chrome codebase :)) Added PerformanceMonitor.EventDetails, too, on the same lines. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:173: * event mouseover. These will vary per event type. On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Are the event types described somewhere? They're built on the C++ side. The UI is meant to be "event agnostic", so that we can create new events on the C++ side, and have them displayed on the UI side without UI modification. Thus the whole bit with the "some label" and "some value". http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:207: this.resizeTimer_ = setInterval(this.checkResize_.bind(this), 100); On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > I would probably schedule a call to checkResize from the resize event, instead > of polling it every 100ms while the user isn't resizing the page. Thought of this, but it doesn't quite work. We need to check after a decent interval has passed *since* the last resize event. There's no way a given resize event can know it's the "last one for a while" without someone checking later. I did, however, change it to cancel the timer once I do a resize, and reinstate it once more resize events come in. Again, if there's a prettier way to do this I would welcome enlightenment. This is quite a hack. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:250: for (var c = 0; c < categories.length; c++) { On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Use categories.forEach(function(category) { ... }, this) ? I always forget whether it's the forEach or the for(..in) that's poison with arrays. Latter, obviously. Fixed here and in a dozen other places. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:256: for (var m = 0; m < category.details.length; m++) { On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Use category.details.forEach(function(metric) { ... }, this) ? Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:259: metric.color = ColorTable_[metric.metricId % ColorTable_.length]; On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Where's it documented that metricId is densely allocated? Good point. Added to the docs. (Webui currently does densely populate them.) http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:345: * and a map |optionCategoryMap| with values that each include a property On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > This might be more clear if you write down an example value for > |optionCategoryMap|. Trying to describe types in English rarely goes well. ;) Added two paragraphs of concrete example for metricCategoryMap_ http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:346: * |description|, and a property |details|, which gives an array of On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > grammar: "gives an array of that in turn" Done. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:349: * Set up a checkbox group for each entry in |optionCategoryMap|, which On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Similarly, it might work better to write some sample output HTML here. Hoping the new concrete example might be sufficient. Lemme know if not. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:387: }.bind(this)); On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Another option to replace this bind() call, and the ", this" argument to > Array.forEach(), is to put "var self=this;" at the top of the method, and then > refer to 'self' instead of 'this' in nested functions. Right. Closure-based "this". I've used it, but been corrected by Aaron, IIRC, to use bind instead. Guess it's all a matter of style. http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo... chrome/browser/resources/performance_monitor/chart.js:401: * @param {!Object} category The metric category for which to create On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > Name the type |category| is supposed to have. Clarified that it's a value from metricCategoryMap_. Would you like me to add a third @typedef?
Aaron, I'm curious about your reasoning behind preferring
function(){...this...}.bind(this) over "var self=this; function(){...self...}" I
assume that also applies to Array.forEach(function, this)?
http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo...
File chrome/browser/resources/performance_monitor/chart.css (right):
http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo...
chrome/browser/resources/performance_monitor/chart.css:7: background: rgb(238,
238, 255);
On 2012/08/30 21:40:18, clintstaley wrote:
> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote:
> > Why rgb(...) instead of #eef?
>
> The automated style checker dinged me for the #eef, saying I should use rgb
> notation for non-gray shades.
So weird.
http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo...
File chrome/browser/resources/performance_monitor/chart.js (right):
http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo...
chrome/browser/resources/performance_monitor/chart.js:250: for (var c = 0; c <
categories.length; c++) {
On 2012/08/30 21:40:18, clintstaley wrote:
> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote:
> > Use categories.forEach(function(category) { ... }, this) ?
>
> I always forget whether it's the forEach or the for(..in) that's poison with
> arrays. Latter, obviously. Fixed here and in a dozen other places.
Thanks!
http://codereview.chromium.org/10900035/diff/1/chrome/browser/resources/perfo...
chrome/browser/resources/performance_monitor/chart.js:401: * @param {!Object}
category The metric category for which to create
On 2012/08/30 21:40:18, clintstaley wrote:
> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote:
> > Name the type |category| is supposed to have.
>
> Clarified that it's a value from metricCategoryMap_. Would you like me to add
a
> third @typedef?
This should be ok. There's 1 indirection either way.
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
File chrome/browser/resources/performance_monitor/chart.js (right):
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
chrome/browser/resources/performance_monitor/chart.js:103: var resizeDelay_ =
500;
On 2012/08/30 21:40:18, clintstaley wrote:
> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote:
> > 500ms is well inside human perception. Did you mean this to feel like a
delay?
>
> No, I meant it to *not* feel like a delay. If there's a better approach to
this
> problem, I'd love to learn it. Apparently HTML resize events, at least in
most
> browsers, come in a firehose, one event per mouse movement as the window is
> dragged, without an easy way to determine when the actual mouseup-indicated
> resize event occurs. Solutions on the web are like this one -- "debouncing"
the
> event stream by waiting a bit since the last event, on the assumption that
means
> the user is "done" resizing. I wanted to wait as long as I could without
> irritating the user.
The general guideline I've seen is that 200ms is about the limit of conscious
perception, although people will react in unconscious ways to smaller delays, so
setting the resizeDelay to 100ms should ensure that it's invisible with a bit of
margin for error. (It'll still visibly lag behind the resize drag, but nobody
will see the delay from the mouseup to it redrawing.)
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
chrome/browser/resources/performance_monitor/chart.js:144: * event mouseover.
These will vary per event type.
On 2012/08/30 21:40:18, clintstaley wrote:
> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote:
> > Are the event types described somewhere?
>
> They're built on the C++ side. The UI is meant to be "event agnostic", so
that
> we can create new events on the C++ side, and have them displayed on the UI
side
> without UI modification. Thus the whole bit with the "some label" and "some
> value".
I guess what I'm wondering is how JS code uses these extra properties without
knowing their names. Or is it that the values of data actually have three
properties, {time, label, value}? (Probably not, since you could express that in
the jsdoc.)
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
chrome/browser/resources/performance_monitor/chart.js:362: * For instance,
|optionCategoryMap| might be metricCategoryMap_, and
I meant something more like:
For example: setupCheckboxes_(div, {'option1': {description: 'Option1
Description', details: [{id: 3, name: 'Name 3', color: 'green'}, {id: 4, name:
'Name4'}]}, 3, check, uncheck)
will populate |div| with the HTML
<div class="something">
<label>Name3<input type="checkbox" style="background-color: green"></label>
</div>
with the checkbox wired to call check() or uncheck() when it becomes checked or
unchecked, respectively, passing (this, {id: 3, name: 'Name 3', color:
'green'}).
With the accurate values and HTML, of course.
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
chrome/browser/resources/performance_monitor/chart.js:458: this.resizeTimer =
setInterval(this.checkResize_.bind(this), 100);
On 2012/08/30 21:40:18, clintstaley wrote:
> On 2012/08/30 00:15:10, Jeffrey Yasskin wrote:
> > I would probably schedule a call to checkResize from the resize event,
instead
> > of polling it every 100ms while the user isn't resizing the page.
>
> Thought of this, but it doesn't quite work. We need to check after a decent
> interval has passed *since* the last resize event. There's no way a given
> resize event can know it's the "last one for a while" without someone checking
> later. I did, however, change it to cancel the timer once I do a resize, and
> reinstate it once more resize events come in.
>
> Again, if there's a prettier way to do this I would welcome enlightenment.
This
> is quite a hack.
I think what I would do here is use setTimeout() to schedule a single update
(and save that an update is scheduled), rather than polling periodically. Then
checkResize_ can run unconditionally and clear the update-scheduled field. I
think that'll eliminate the need for the lastResizeTime_ field.
The "resizeTimer" on this line appears to be missing its trailing underscore.
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
chrome/browser/resources/performance_monitor/chart.js:749: return false;
This 'return false' doesn't return all the way out of isDataReady. I think you
want something like:
return chartData.metrics.every(function(metric) { return metric.data; }) &&
chartData.events.every(function(event) { return event.data; });
http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe...
chrome/browser/resources/performance_monitor/chart.js:810: console.log('Event
out of time range ' + this.start + ' -> ' +
Remember that 'this' gets reset within a function literal. Either use the 'var
self=this;' trick at the top of getEventMarks_ or pass 'this' through forEach.
Jeff, back to you. http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:103: var resizeDelay_ = 500; On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > On 2012/08/30 21:40:18, clintstaley wrote: > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > 500ms is well inside human perception. Did you mean this to feel like a > delay? > > > > No, I meant it to *not* feel like a delay. If there's a better approach to > this > > problem, I'd love to learn it. Apparently HTML resize events, at least in > most > > browsers, come in a firehose, one event per mouse movement as the window is > > dragged, without an easy way to determine when the actual mouseup-indicated > > resize event occurs. Solutions on the web are like this one -- "debouncing" > the > > event stream by waiting a bit since the last event, on the assumption that > means > > the user is "done" resizing. I wanted to wait as long as I could without > > irritating the user. > > The general guideline I've seen is that 200ms is about the limit of conscious > perception, although people will react in unconscious ways to smaller delays, so > setting the resizeDelay to 100ms should ensure that it's invisible with a bit of > margin for error. (It'll still visibly lag behind the resize drag, but nobody > will see the delay from the mouseup to it redrawing.) Right, but then if they pause the mouse for even 1/10th of a second while dragging the window, it'll redraw the graphs on them, in mid-resize. A slow mouse user will see redraw flicker and mouse hanging. Redraws are very expensive. http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:144: * event mouseover. These will vary per event type. On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > On 2012/08/30 21:40:18, clintstaley wrote: > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > Are the event types described somewhere? > > > > They're built on the C++ side. The UI is meant to be "event agnostic", so > that > > we can create new events on the C++ side, and have them displayed on the UI > side > > without UI modification. Thus the whole bit with the "some label" and "some > > value". > > I guess what I'm wondering is how JS code uses these extra properties without > knowing their names. Or is it that the values of data actually have three > properties, {time, label, value}? (Probably not, since you could express that in > the jsdoc.) The *value* has predictable keys of "label" and "value". The keys of these mysterious properties are indeterminate. IIRC, the obvious alternative of a list of such objects would have made the C++-side design much more complex. http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:362: * For instance, |optionCategoryMap| might be metricCategoryMap_, and On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > I meant something more like: > > For example: setupCheckboxes_(div, {'option1': {description: 'Option1 > Description', details: [{id: 3, name: 'Name 3', color: 'green'}, {id: 4, name: > 'Name4'}]}, 3, check, uncheck) > will populate |div| with the HTML > <div class="something"> > <label>Name3<input type="checkbox" style="background-color: green"></label> > </div> > with the checkbox wired to call check() or uncheck() when it becomes checked or > unchecked, respectively, passing (this, {id: 3, name: 'Name 3', color: > 'green'}). > > > With the accurate values and HTML, of course. OK. Was hoping a clear description would suffice :), but HTML example now added. http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:458: this.resizeTimer = setInterval(this.checkResize_.bind(this), 100); On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > On 2012/08/30 21:40:18, clintstaley wrote: > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > I would probably schedule a call to checkResize from the resize event, > instead > > > of polling it every 100ms while the user isn't resizing the page. > > > > Thought of this, but it doesn't quite work. We need to check after a decent > > interval has passed *since* the last resize event. There's no way a given > > resize event can know it's the "last one for a while" without someone checking > > later. I did, however, change it to cancel the timer once I do a resize, and > > reinstate it once more resize events come in. > > > > Again, if there's a prettier way to do this I would welcome enlightenment. > This > > is quite a hack. > > I think what I would do here is use setTimeout() to schedule a single update > (and save that an update is scheduled), rather than polling periodically. Then > checkResize_ can run unconditionally and clear the update-scheduled field. I > think that'll eliminate the need for the lastResizeTime_ field. > > The "resizeTimer" on this line appears to be missing its trailing underscore. Wow. I tested it, too. Dunno what happened. But, I'm not sure the setTimeout will help much. It can't check unconditionally, and will just have to reset itself if there has been a mouse movement within the last 500 ms, indicating that the drag operation is not yet complete. Or am I missing something? Just reiterating, my best understanding is that we must wait until the resize events have "quieted down". That might be 30 seconds later if the user has a hard time deciding on the new window size, or 500 ms from now if he jerks the mouse once and then lets it sit. http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:749: return false; On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > This 'return false' doesn't return all the way out of isDataReady. I think you > want something like: > > return chartData.metrics.every(function(metric) { return metric.data; }) && > chartData.events.every(function(event) { return event.data; }); This worked OK with standard for-loops, too. But the every is too cool not to use :). Done http://codereview.chromium.org/10900035/diff/6001/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:810: console.log('Event out of time range ' + this.start + ' -> ' + On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > Remember that 'this' gets reset within a function literal. Either use the 'var > self=this;' trick at the top of getEventMarks_ or pass 'this' through forEach. Thanks. Missed this one when scanning for "this" usage. Done
http://codereview.chromium.org/10900035/diff/8004/chrome/browser/resources/pe... File chrome/browser/resources/performance_monitor/chart.js (right): http://codereview.chromium.org/10900035/diff/8004/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:103: var resizeDelay_ = 500; On 2012/09/04 19:28:56, clintstaley wrote: > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > On 2012/08/30 21:40:18, clintstaley wrote: > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > 500ms is well inside human perception. Did you mean this to feel like a > > delay? > > > > > > No, I meant it to *not* feel like a delay. If there's a better approach to > > this > > > problem, I'd love to learn it. Apparently HTML resize events, at least in > > most > > > browsers, come in a firehose, one event per mouse movement as the window is > > > dragged, without an easy way to determine when the actual mouseup-indicated > > > resize event occurs. Solutions on the web are like this one -- "debouncing" > > the > > > event stream by waiting a bit since the last event, on the assumption that > > means > > > the user is "done" resizing. I wanted to wait as long as I could without > > > irritating the user. > > > > The general guideline I've seen is that 200ms is about the limit of conscious > > perception, although people will react in unconscious ways to smaller delays, > so > > setting the resizeDelay to 100ms should ensure that it's invisible with a bit > of > > margin for error. (It'll still visibly lag behind the resize drag, but nobody > > will see the delay from the mouseup to it redrawing.) > > Right, but then if they pause the mouse for even 1/10th of a second while > dragging the window, it'll redraw the graphs on them, in mid-resize. A slow > mouse user will see redraw flicker and mouse hanging. Redraws are very > expensive. Fair enough. http://codereview.chromium.org/10900035/diff/8004/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:144: * event mouseover. These will vary per event type. On 2012/09/04 19:28:56, clintstaley wrote: > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > On 2012/08/30 21:40:18, clintstaley wrote: > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > Are the event types described somewhere? > > > > > > They're built on the C++ side. The UI is meant to be "event agnostic", so > > that > > > we can create new events on the C++ side, and have them displayed on the UI > > side > > > without UI modification. Thus the whole bit with the "some label" and "some > > > value". > > > > I guess what I'm wondering is how JS code uses these extra properties without > > knowing their names. Or is it that the values of data actually have three > > properties, {time, label, value}? (Probably not, since you could express that > in > > the jsdoc.) > > The *value* has predictable keys of "label" and "value". The keys of these > mysterious properties are indeterminate. IIRC, the obvious alternative of a > list of such objects would have made the C++-side design much more complex. > I see. So effectively each array element maps a time to a set of label/value pairs, and JS code is expected to just iterate over all of them using a for/in loop, with an explicit exclusion for the 'time' property. I'd probably say that instead of that the set of keys varies per event type, since the keys actually shouldn't be used for anything. http://codereview.chromium.org/10900035/diff/8004/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:369: * optionCategoryMap : { Ok, great. Now you can simplify the previous two paragraphs by taking advantage of this example argument. Perhaps: Generalized function for setting up checkbox blocks into |div| for either events or metrics described by the data in |optionCategoryMap|. If |optionCategoryMap| has the value: <example value here> We might call this.setupCheckboxes_(div, optionCategoryMap, 'metricId', addMetric, dropMetric). Note that 'metricId' matches the key in the details arrays which stores an identifying object (often a numeric ID) for the metric. This would fill |div| with HTML like: <example HTML here> and when the user unchecks the "CPU Usage" checkbox, for example, dropMetric(this, 1/*==optionCategoryMap[1].details[0].metricId*/) would be called. http://codereview.chromium.org/10900035/diff/8004/chrome/browser/resources/pe... chrome/browser/resources/performance_monitor/chart.js:501: this.resizeTimer_ = setInterval(this.checkResize_.bind(this), 100); On 2012/09/04 19:28:56, clintstaley wrote: > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > On 2012/08/30 21:40:18, clintstaley wrote: > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > I would probably schedule a call to checkResize from the resize event, > > instead > > > > of polling it every 100ms while the user isn't resizing the page. > > > > > > Thought of this, but it doesn't quite work. We need to check after a decent > > > interval has passed *since* the last resize event. There's no way a given > > > resize event can know it's the "last one for a while" without someone > checking > > > later. I did, however, change it to cancel the timer once I do a resize, > and > > > reinstate it once more resize events come in. > > > > > > Again, if there's a prettier way to do this I would welcome enlightenment. > > This > > > is quite a hack. > > > > I think what I would do here is use setTimeout() to schedule a single update > > (and save that an update is scheduled), rather than polling periodically. Then > > checkResize_ can run unconditionally and clear the update-scheduled field. I > > think that'll eliminate the need for the lastResizeTime_ field. > > > > The "resizeTimer" on this line appears to be missing its trailing underscore. > > Wow. I tested it, too. Dunno what happened. But, I'm not sure the setTimeout > will help much. It can't check unconditionally, and will just have to reset > itself if there has been a mouse movement within the last 500 ms, indicating > that the drag operation is not yet complete. Or am I missing something? Ah, *I* had missed that you reset lastResizeTime on each resize event, so the redraw happens 500ms after the last one instead of after the first. Still, you could use: if (this.resizeTimer_ != null) clearTimeout(this.resizeTimer_) this.resizeTimer_ = setTimeout(this.checkResize_.bind(this), resizeDelay_) to avoid all the polling. (You don't actually even need to check for resizeTimer_!=null since "Passing an invalid ID to clearTimeout does not have any effect (and doesn't throw an exception).", but keeping resizeTimer_ set only when a resize is pending should help debugging some.)
https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:138: * Similar data for events as for metrics, though no yAxis info is needed nit: Be consistent in spelling y-axis/yAxis in comments. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:253: * Receive a list of all metric categories, each with its correspoinding nit: corresponding https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:423: * @param {!Object} optionCategory Map A map of metric/event categories. nit: This should be optionCategoryMap (no space), right? https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:506: * Check to see if resize events have recently occured, and if a long nit: occurred https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:862: var axisMap = {}; // Maps category ids to yAxis numbers nit: |yAxisNumber|s? Make it clear this is a variable.
Back to you, jyasskin and dcronin https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:138: * Similar data for events as for metrics, though no yAxis info is needed On 2012/09/07 18:44:54, D Cronin wrote: > nit: Be consistent in spelling y-axis/yAxis in comments. Done. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:144: * event mouseover. These will vary per event type. On 2012/09/05 22:21:54, Jeffrey Yasskin wrote: > On 2012/09/04 19:28:56, clintstaley wrote: > > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > > On 2012/08/30 21:40:18, clintstaley wrote: > > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > > Are the event types described somewhere? > > > > > > > > They're built on the C++ side. The UI is meant to be "event agnostic", so > > > that > > > > we can create new events on the C++ side, and have them displayed on the > UI > > > side > > > > without UI modification. Thus the whole bit with the "some label" and > "some > > > > value". > > > > > > I guess what I'm wondering is how JS code uses these extra properties > without > > > knowing their names. Or is it that the values of data actually have three > > > properties, {time, label, value}? (Probably not, since you could express > that > > in > > > the jsdoc.) > > > > The *value* has predictable keys of "label" and "value". The keys of these > > mysterious properties are indeterminate. IIRC, the obvious alternative of a > > list of such objects would have made the C++-side design much more complex. > > > > I see. So effectively each array element maps a time to a set of label/value > pairs, and JS code is expected to just iterate over all of them using a for/in > loop, with an explicit exclusion for the 'time' property. > > I'd probably say that instead of that the set of keys varies per event type, > since the keys actually shouldn't be used for anything. I assume you mean for me to revise the comment accordingly, which I've done now. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:253: * Receive a list of all metric categories, each with its correspoinding On 2012/09/07 18:44:54, D Cronin wrote: > nit: corresponding Done. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:369: * optionCategoryMap : { On 2012/09/05 22:21:54, Jeffrey Yasskin wrote: > Ok, great. Now you can simplify the previous two paragraphs by taking advantage > of this example argument. Perhaps: > > Generalized function for setting up checkbox blocks into |div| for either events > or metrics described by the data in |optionCategoryMap|. If |optionCategoryMap| > has the value: > > <example value here> > > We might call this.setupCheckboxes_(div, optionCategoryMap, 'metricId', > addMetric, dropMetric). Note that 'metricId' matches the key in the details > arrays which stores an identifying object (often a numeric ID) for the metric. > This would fill |div| with HTML like: > > <example HTML here> > > and when the user unchecks the "CPU Usage" checkbox, for example, > dropMetric(this, 1/*==optionCategoryMap[1].details[0].metricId*/) would be > called. > I rephrased the comment to put the concrete example first. However, a general statement of the rules for optionCategoryMap is still needed IMO, so I retained the formal statement after the concrete discussion. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:423: * @param {!Object} optionCategory Map A map of metric/event categories. On 2012/09/07 18:44:54, D Cronin wrote: > nit: This should be optionCategoryMap (no space), right? Good eyes. Done https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:501: this.resizeTimer_ = setInterval(this.checkResize_.bind(this), 100); On 2012/09/05 22:21:54, Jeffrey Yasskin wrote: > On 2012/09/04 19:28:56, clintstaley wrote: > > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > > On 2012/08/30 21:40:18, clintstaley wrote: > > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > > I would probably schedule a call to checkResize from the resize event, > > > instead > > > > > of polling it every 100ms while the user isn't resizing the page. > > > > > > > > Thought of this, but it doesn't quite work. We need to check after a > decent > > > > interval has passed *since* the last resize event. There's no way a given > > > > resize event can know it's the "last one for a while" without someone > > checking > > > > later. I did, however, change it to cancel the timer once I do a resize, > > and > > > > reinstate it once more resize events come in. > > > > > > > > Again, if there's a prettier way to do this I would welcome enlightenment. > > > > This > > > > is quite a hack. > > > > > > I think what I would do here is use setTimeout() to schedule a single update > > > (and save that an update is scheduled), rather than polling periodically. > Then > > > checkResize_ can run unconditionally and clear the update-scheduled field. I > > > think that'll eliminate the need for the lastResizeTime_ field. > > > > > > The "resizeTimer" on this line appears to be missing its trailing > underscore. > > > > Wow. I tested it, too. Dunno what happened. But, I'm not sure the > setTimeout > > will help much. It can't check unconditionally, and will just have to reset > > itself if there has been a mouse movement within the last 500 ms, indicating > > that the drag operation is not yet complete. Or am I missing something? > > Ah, *I* had missed that you reset lastResizeTime on each resize event, so the > redraw happens 500ms after the last one instead of after the first. > > Still, you could use: > > if (this.resizeTimer_ != null) > clearTimeout(this.resizeTimer_) > this.resizeTimer_ = setTimeout(this.checkResize_.bind(this), resizeDelay_) > > to avoid all the polling. (You don't actually even need to check for > resizeTimer_!=null since "Passing an invalid ID to clearTimeout does not have > any effect (and doesn't throw an exception).", but keeping resizeTimer_ set only > when a resize is pending should help debugging some.) I considered this, but didn't like recreating and clearing so many timers. If you prefer it, tho, I'll change it. It is marginally more responsive since it ensures a timer check exactly 500mS after last mouse move. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:506: * Check to see if resize events have recently occured, and if a long On 2012/09/07 18:44:54, D Cronin wrote: > nit: occurred Done. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:862: var axisMap = {}; // Maps category ids to yAxis numbers On 2012/09/07 18:44:54, D Cronin wrote: > nit: |yAxisNumber|s? Make it clear this is a variable. I'm going to differ on this. yAxisNumber is just a local, not a persistent property. The concept of yAxis number is independent of any program variable.
https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:501: this.resizeTimer_ = setInterval(this.checkResize_.bind(this), 100); On 2012/09/07 20:39:52, clintstaley wrote: > On 2012/09/05 22:21:54, Jeffrey Yasskin wrote: > > On 2012/09/04 19:28:56, clintstaley wrote: > > > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > > > On 2012/08/30 21:40:18, clintstaley wrote: > > > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > > > I would probably schedule a call to checkResize from the resize event, > > > > instead > > > > > > of polling it every 100ms while the user isn't resizing the page. > > > > > > > > > > Thought of this, but it doesn't quite work. We need to check after a > > decent > > > > > interval has passed *since* the last resize event. There's no way a > given > > > > > resize event can know it's the "last one for a while" without someone > > > checking > > > > > later. I did, however, change it to cancel the timer once I do a > resize, > > > and > > > > > reinstate it once more resize events come in. > > > > > > > > > > Again, if there's a prettier way to do this I would welcome > enlightenment. > > > > > > This > > > > > is quite a hack. > > > > > > > > I think what I would do here is use setTimeout() to schedule a single > update > > > > (and save that an update is scheduled), rather than polling periodically. > > Then > > > > checkResize_ can run unconditionally and clear the update-scheduled field. > I > > > > think that'll eliminate the need for the lastResizeTime_ field. > > > > > > > > The "resizeTimer" on this line appears to be missing its trailing > > underscore. > > > > > > Wow. I tested it, too. Dunno what happened. But, I'm not sure the > > setTimeout > > > will help much. It can't check unconditionally, and will just have to reset > > > itself if there has been a mouse movement within the last 500 ms, indicating > > > that the drag operation is not yet complete. Or am I missing something? > > > > Ah, *I* had missed that you reset lastResizeTime on each resize event, so the > > redraw happens 500ms after the last one instead of after the first. > > > > Still, you could use: > > > > if (this.resizeTimer_ != null) > > clearTimeout(this.resizeTimer_) > > this.resizeTimer_ = setTimeout(this.checkResize_.bind(this), resizeDelay_) > > > > to avoid all the polling. (You don't actually even need to check for > > resizeTimer_!=null since "Passing an invalid ID to clearTimeout does not have > > any effect (and doesn't throw an exception).", but keeping resizeTimer_ set > only > > when a resize is pending should help debugging some.) > > I considered this, but didn't like recreating and clearing so many timers. If > you prefer it, tho, I'll change it. It is marginally more responsive since it > ensures a timer check exactly 500mS after last mouse move. I don't care about this enough to insist either way. My thinking is that processor wakeups are much more expensive than a bit of extra processing during an existing event. Clearing and re-setting a timer is a bit of extra processing, but it's only done during a resize event that's happening anyway, while the interval poll causes code to run (and wake up the processor) at times other code might not be running at all. The fact that clearing-and-setting a single timeout gives you a more precise wakeup is just an added bonus, which isn't all that important in this particular example. https://chromiumcodereview.appspot.com/10900035/diff/10005/chrome/browser/res... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10900035/diff/10005/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:518: * The timer for resizing after a delay since the last resize event has This looks like an intermediate checkin with somewhat inconsistent code for the timer delay and checking. Could you double-check?
Back to you, jyasskin. I believe the timer code is correct (runs fine, at least, and I see no errors by inspection). I clarified the comment a bit in case that's useful. That's the only change in the latest. https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10900035/diff/8004/chrome/browser/reso... chrome/browser/resources/performance_monitor/chart.js:501: this.resizeTimer_ = setInterval(this.checkResize_.bind(this), 100); On 2012/09/07 21:27:17, Jeffrey Yasskin wrote: > On 2012/09/07 20:39:52, clintstaley wrote: > > On 2012/09/05 22:21:54, Jeffrey Yasskin wrote: > > > On 2012/09/04 19:28:56, clintstaley wrote: > > > > On 2012/08/31 22:28:16, Jeffrey Yasskin wrote: > > > > > On 2012/08/30 21:40:18, clintstaley wrote: > > > > > > On 2012/08/30 00:15:10, Jeffrey Yasskin wrote: > > > > > > > I would probably schedule a call to checkResize from the resize > event, > > > > > instead > > > > > > > of polling it every 100ms while the user isn't resizing the page. > > > > > > > > > > > > Thought of this, but it doesn't quite work. We need to check after a > > > decent > > > > > > interval has passed *since* the last resize event. There's no way a > > given > > > > > > resize event can know it's the "last one for a while" without someone > > > > checking > > > > > > later. I did, however, change it to cancel the timer once I do a > > resize, > > > > and > > > > > > reinstate it once more resize events come in. > > > > > > > > > > > > Again, if there's a prettier way to do this I would welcome > > enlightenment. > > > > > > > > This > > > > > > is quite a hack. > > > > > > > > > > I think what I would do here is use setTimeout() to schedule a single > > update > > > > > (and save that an update is scheduled), rather than polling > periodically. > > > Then > > > > > checkResize_ can run unconditionally and clear the update-scheduled > field. > > I > > > > > think that'll eliminate the need for the lastResizeTime_ field. > > > > > > > > > > The "resizeTimer" on this line appears to be missing its trailing > > > underscore. > > > > > > > > Wow. I tested it, too. Dunno what happened. But, I'm not sure the > > > setTimeout > > > > will help much. It can't check unconditionally, and will just have to > reset > > > > itself if there has been a mouse movement within the last 500 ms, > indicating > > > > that the drag operation is not yet complete. Or am I missing something? > > > > > > Ah, *I* had missed that you reset lastResizeTime on each resize event, so > the > > > redraw happens 500ms after the last one instead of after the first. > > > > > > Still, you could use: > > > > > > if (this.resizeTimer_ != null) > > > clearTimeout(this.resizeTimer_) > > > this.resizeTimer_ = setTimeout(this.checkResize_.bind(this), resizeDelay_) > > > > > > to avoid all the polling. (You don't actually even need to check for > > > resizeTimer_!=null since "Passing an invalid ID to clearTimeout does not > have > > > any effect (and doesn't throw an exception).", but keeping resizeTimer_ set > > only > > > when a resize is pending should help debugging some.) > > > > I considered this, but didn't like recreating and clearing so many timers. If > > you prefer it, tho, I'll change it. It is marginally more responsive since it > > ensures a timer check exactly 500mS after last mouse move. > > I don't care about this enough to insist either way. My thinking is that > processor wakeups are much more expensive than a bit of extra processing during > an existing event. Clearing and re-setting a timer is a bit of extra processing, > but it's only done during a resize event that's happening anyway, while the > interval poll causes code to run (and wake up the processor) at times other code > might not be running at all. The fact that clearing-and-setting a single timeout > gives you a more precise wakeup is just an added bonus, which isn't all that > important in this particular example. I changed it anyway. Code is cleaner, at least. Works fine, too. https://chromiumcodereview.appspot.com/10900035/diff/10005/chrome/browser/res... File chrome/browser/resources/performance_monitor/chart.js (right): https://chromiumcodereview.appspot.com/10900035/diff/10005/chrome/browser/res... chrome/browser/resources/performance_monitor/chart.js:518: * The timer for resizing after a delay since the last resize event has On 2012/09/07 21:27:17, Jeffrey Yasskin wrote: > This looks like an intermediate checkin with somewhat inconsistent code for the > timer delay and checking. Could you double-check? I did check it for correct function, and it appears to work fine. The resize call will maintain a timer that triggers 500ms after the last resize event. The checkResize is thus called only when no resize events have occurred for 500ms, and it clears and nulls the timer (which must exist for checkResize to be called at all) and does the requisite redraws. What do you feel is missing? The comment parses a little awkwardly; I can clarify that.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10900035/17002
Presubmit check for 10900035-17002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. Was the CSS checker useful? Send feedback or hate mail to dbeam@chromium.org. ** Presubmit Warnings ** chrome/browser/resources/performance_monitor/chart.css: - Make all zero length terms (i.e. 0px) 0 unless inside of hsl() or part of @keyframe. text-shadow: #F0F0F0 0px 1px 0px; ** Presubmit ERRORS ** Missing LGTM from an OWNER for files in these directories: chrome/browser/resources Presubmit checks took 1.8s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10900035/8005
Try job failure for 10900035-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/10900035/8005
Change committed as 156246 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
