Issue 242750043: Response/Animation/Load Interaction Record subclasses

Issue 242750043: Response/Animation/Load Interaction Record subclasses (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
8 years, 11 months ago by benjhayden

Modified:
8 years, 10 months ago

Reviewers:
nduca

CC:
trace-viewer-review_googlegroups.com

Base URL:
git@github.com:google/trace-viewer@master

Visibility:
Public.

More Reviews

Description

Response/Animation/Load Interaction Record subclasses BUG= R=nduca@chromium.org Committed: https://chromium.googlesource.com/external/trace-viewer/+/e1cdfe53f346bf4b6c37c8a0367e470b7ba394e6

Patch Set 6 : comments #

Created: 8 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+174 lines, -101 lines)			Patch
M	BUILD.gn	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	trace_viewer.gyp	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
A	trace_viewer/extras/rail/animation_interaction_record.html	View	1 2 3 4 5	1 chunk	+39 lines, -0 lines	0 comments	Download
A	trace_viewer/extras/rail/load_interaction_record.html	View	1 2 3 4 5	1 chunk	+39 lines, -0 lines	0 comments	Download
M	trace_viewer/extras/rail/rail_interaction_record.html	View	1 2	1 chunk	+15 lines, -3 lines	0 comments	Download
M	trace_viewer/extras/rail/rail_ir_finder.html	View	1 2 3 4 5	4 chunks	+34 lines, -80 lines	0 comments	Download
M	trace_viewer/extras/rail/rail_ir_finder_test.html	View	1 2 3 4	2 chunks	+2 lines, -18 lines	0 comments	Download
A	trace_viewer/extras/rail/response_interaction_record.html	View	1 2 3 4 5	1 chunk	+39 lines, -0 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

nduca

lots of feedback. lets split apart and land the obvious stuff first. https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audits/utils.html File trace_viewer/extras/audits/utils.html ...

8 years, 11 months ago (2015-05-27 02:44:36 UTC) #2

benjhayden

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audits/utils.html File trace_viewer/extras/audits/utils.html (right): https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audits/utils.html#newcode82 trace_viewer/extras/audits/utils.html:82: function interpolate1(p0, x, p1) { On 2015/05/27 02:44:35, nduca ...

8 years, 10 months ago (2015-06-01 18:43:55 UTC) #3

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audit...
File trace_viewer/extras/audits/utils.html (right):

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audit...
trace_viewer/extras/audits/utils.html:82: function interpolate1(p0, x, p1) {
On 2015/05/27 02:44:35, nduca wrote:
> does base.utils' lerp work?

lerp does about half of what this function does.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audit...
trace_viewer/extras/audits/utils.html:90: function interpolate(points, value) {
On 2015/05/27 02:44:35, nduca wrote:
> lets put this where lerp goes. separate patch :)

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/audit...
trace_viewer/extras/audits/utils.html:104: mergeEvents: mergeEvents
On 2015/05/27 02:44:35, nduca wrote:
> wdyt about making a function that takes an array of ranges like mergeEvents
> takes, but that yields the "gaps" in the trace back to a callback, again in
the
> style of mergeEvents... [needless to say, separate patch applies here too ^_^]

https://codereview.appspot.com/241860043/

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
File trace_viewer/extras/rail/animation_interaction_record.html (right):

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/animation_interaction_record.html:37: get
normalizedUserPain() {
On 2015/05/27 02:44:35, nduca wrote:
> lets do the overrides separately so there's a good paper trail of our initial
> values

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
File trace_viewer/extras/rail/idle_interaction_record.html (right):

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:26: this.args = this.args
|| {};
On 2015/05/27 02:44:35, nduca wrote:
> any reason we're setting this here and not on the base class?

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:27: this.associatedEvents
= [];
On 2015/05/27 02:44:35, nduca wrote:
> any reason we're setting this here? could the base do this?

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:37: get
normalizedUserPain() {
On 2015/05/27 02:44:35, nduca wrote:
> I think a note on why we set this to zero, e.g. the rationale, would be good
> here.

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:42: return 1;
On 2015/05/27 02:44:35, nduca wrote:
> i think a //TODO(benshayden): Explain what this should be in the future would
be
> good here.

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:46:
IdleInteractionRecord.subtract = function(irs, bounds) {
On 2015/05/27 02:44:35, nduca wrote:
> i think its a bit confusing that the idle finding code here lives in the
record
> itself. the rest of ir finding is in the ir finder, so imagine for a moment
> explaining to another intern why the ir finder is elsewhere... i'd lean toward
> this being in the finder itself until we have a plan for breaking up the ir
> finder into pieces.

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:48: if (a.start < b.start)
On 2015/05/27 02:44:35, nduca wrote:
> a clean way of writing this is...
> 
> var delta = a.start - b.start
> if (delta !=== 0)
>   return delta;
> 

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:52: return 0;
On 2015/05/27 02:44:35, nduca wrote:
> the reason you do the way above is so that you can then be robust to ties.
> 
> namely, if delta is zero, compare on guid.  that is:
> return a.guid - b.guid
> 
> 
> may not be needed in this case [i can't be sure honestly?] but i thought i'd
> mention the style as a teachable.
> 

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/idle_interaction_record.html:58:
irs.forEach(function(ir, index) {
On 2015/05/27 02:44:35, nduca wrote:
> looks like a thing for utils.html

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
File trace_viewer/extras/rail/rail_interaction_record.html (right):

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/rail_interaction_record.html:49: // 0 means painless. 1
means "switch to firefox".
On 2015/05/27 02:44:36, nduca wrote:
> hahaahahaha omg keep this.

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/rail_interaction_record.html:54: get efficiency() {
On 2015/05/27 02:44:36, nduca wrote:
> lets put a comment on this too. maybe also normalizedEfficiency() so its clear
> that its 0-1?

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/rail_interaction_record.html:60: return (pain > 0.8) ?
(1 - pain) : (this.efficiency * (1 - pain));
On 2015/05/27 02:44:36, nduca wrote:
> i'd like to land something that is continuous in its weighting. the problem
with
> step functions is they amplify noise. continuous functions have a damping
effect
> on noise, which is important for a high quality benchmark.
> 
> maybe there's a very simple piecewise linear or easing function (eg
> http://easings.net/) we can define that mixes in efficiency such that at from
> p=0 to some pain-matters-more thresshold, efficiencyWeight goes from 1.0 to
some
> smaller number, and then drops rapidly from there.

Let's leave the actual function for another CL.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/rail_interaction_record.html:60: return (pain > 0.8) ?
(1 - pain) : (this.efficiency * (1 - pain));
On 2015/05/27 02:44:36, nduca wrote:
> lets put some explanatory text
> // The default rail score for an interaction range is blah blah, here's our
> rough idea of why, explain
> // in a few sentences our whiteboard conversation.
> if (a)
>   return
> 
> return
> 

Let's leave that thought for another CL and just get the studs up first.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
File trace_viewer/extras/rail/rail_ir_finder.html (right):

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/rail_ir_finder.html:225: return rirs;
On 2015/05/27 02:44:36, nduca wrote:
> we'll want a coding strategy here so we can test the idle finding code without
> having to create all the data for the gestures there too... e.g. here's a
bunch
> of existing irs, what're the idle gaps?

I think subtractEvents() does that?
https://codereview.appspot.com/241860043/

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
File trace_viewer/extras/rail/response_interaction_record.html (right):

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:19: */
On 2015/05/27 02:44:36, nduca wrote:
> lets make the subclasses as a standalone patch then come back and do the idle
> records as its own patch

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:21: function
ResponseInteractionRecord(gesture) {
On 2015/05/27 02:44:36, nduca wrote:
> one concern i have here is that the gesture object is an ad-hoc object inside
> the rail ir finder. I'd almost like to see the ir finder push into the ir's
> associatedEvents events array until we've figured out a bit more about how
> associated events.
> 
> the stylistic thing we try to avoid is objects moving between files that are
> adhoc, that is dont actually have a type. thats hard to explain to new
> contributors.

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:24:
tv.b.ui.getColorIdForGeneralPurposeString('mt_input'),
On 2015/05/27 02:44:36, nduca wrote:
> lets add new general purpose names for each of these and get rid of the mt_
> ones. fresh patch could get a fast review if you do this independently.

Before or after this cl?

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:26:
this.associatedEvents = gesture.latencyEvents;
On 2015/05/27 02:44:36, nduca wrote:
> prefer to push not overwrite

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:27: this.args =
this.args || {};
On 2015/05/27 02:44:36, nduca wrote:
> why clobber?

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:30: var PAIN_CURVE =
[[150, 0], [300, 0.2], [1000, 1]];
On 2015/05/27 02:44:36, nduca wrote:
> prefer constants at top of file before classes

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:40: return
tv.e.audits.interpolate(PAIN_CURVE, this.duration);
On 2015/05/27 02:44:36, nduca wrote:
> lets add an override for this one as a followup so we can discuss the
algorithm
> and have a codereview that shows the discussion

Done.

https://codereview.appspot.com/242750043/diff/80001/trace_viewer/extras/rail/...
trace_viewer/extras/rail/response_interaction_record.html:44:
ResponseInteractionRecord.forEach = function(gestures) {
On 2015/05/27 02:44:36, nduca wrote:
> i'm a bit skeptical of this. i think this could be a helper function in
> ir_finder... it smells that this has to be on every file, and that you have to
> read the implementation to understand what the call site is doing.
> 
> in the medium term, i coudl buy that we want to be figuring out how to factor
ir
> finder into smaller files as it grows. medium term though.

Done.

nduca

lgtm https://codereview.appspot.com/242750043/diff/160001/trace_viewer/extras/rail/animation_interaction_record.html File trace_viewer/extras/rail/animation_interaction_record.html (right): https://codereview.appspot.com/242750043/diff/160001/trace_viewer/extras/rail/animation_interaction_record.html#newcode20 trace_viewer/extras/rail/animation_interaction_record.html:20: function AnimationInteractionRecord(start, end, events) { lets have them ...

8 years, 10 months ago (2015-06-01 23:10:01 UTC) #4

benjhayden

https://codereview.appspot.com/242750043/diff/160001/trace_viewer/extras/rail/animation_interaction_record.html File trace_viewer/extras/rail/animation_interaction_record.html (right): https://codereview.appspot.com/242750043/diff/160001/trace_viewer/extras/rail/animation_interaction_record.html#newcode20 trace_viewer/extras/rail/animation_interaction_record.html:20: function AnimationInteractionRecord(start, end, events) { On 2015/06/01 23:10:01, nduca ...

8 years, 10 months ago (2015-06-02 20:45:22 UTC) #5

benjhayden

8 years, 10 months ago (2015-06-02 21:28:18 UTC) #6

Message was sent while issue was closed.

Committed patchset #6 (id:220001) manually as
e1cdfe53f346bf4b6c37c8a0367e470b7ba394e6 (presubmit successful).

Expand All Messages | Collapse All Messages