findEmptyRangesBetweenExplicitRanges()

findEmptyRangesBetweenExplicitRanges()

R=nduca@chromium.org

Committed: https://chromium.googlesource.com/external/trace-viewer/+/e9dc685064f50a33080cef797e25e68519118cbb

[nduca, 2015-05-30 02:55:23]

https://codereview.appspot.com/236630043/ is gonna kinda cause merge conflicts
huh? guess if you land before yuhao you might get an opportunity to teach him
how to deal with git merge conflicts... ^_^

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
File trace_viewer/extras/audits/utils.html (right):

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:82: function subtractEvents(events,
bounds) {
findEmptyRangesInBetweenExplicitRanges maybe?

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:82: function subtractEvents(events,
bounds) {
Lets put a docstring on this. Bounds is a bit unclear what it does.

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:83: events.sort(function(a, b) {
events = events.slice() to clone it


that way you're not shuffling the event array that they're trying to give you...
that's a surprising side effect if you think about it.

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:83: events.sort(function(a, b) {
copy the conevntion of mergeRnages (post
https://codereview.appspot.com/236630043/): blahBlahExplicit(bounds, ranges,
lowFn, hiFn) maybe?

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:100: // otherwise other.start < event.end
// Comments are complete sentences.

[benjhayden, 2015-06-01 19:19:08]

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
File trace_viewer/extras/audits/utils.html (right):

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:82: function subtractEvents(events,
bounds) {
On 2015/05/30 02:55:22, nduca wrote:
> findEmptyRangesInBetweenExplicitRanges maybe?

Done.

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:82: function subtractEvents(events,
bounds) {
On 2015/05/30 02:55:22, nduca wrote:
> Lets put a docstring on this. Bounds is a bit unclear what it does. 

Done.

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:83: events.sort(function(a, b) {
On 2015/05/30 02:55:22, nduca wrote:
> events = events.slice() to clone it
> 
> 
> that way you're not shuffling the event array that they're trying to give
you...
> that's a surprising side effect if you think about it.

Done.

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:83: events.sort(function(a, b) {
On 2015/05/30 02:55:22, nduca wrote:
> copy the conevntion of mergeRnages (post
> https://codereview.appspot.com/236630043/): blahBlahExplicit(bounds, ranges,
> lowFn, hiFn) maybe?

Done.

https://codereview.appspot.com/241860043/diff/1/trace_viewer/extras/audits/ut...
trace_viewer/extras/audits/utils.html:100: // otherwise other.start < event.end
On 2015/05/30 02:55:22, nduca wrote:
> // Comments are complete sentences.

Done.

[nduca, 2015-06-02 00:36:13]

lgtm

https://codereview.appspot.com/241860043/diff/20001/trace_viewer/base/range_u...
File trace_viewer/base/range_utils.html (right):

https://codereview.appspot.com/241860043/diff/20001/trace_viewer/base/range_u...
trace_viewer/base/range_utils.html:94: if (!startFunction)
just require the startFunction and endFunction. The explicit one above does.

General rule of thumb: always match the style of code you're working in. If you
disagree with the style, put a patch that changes the style.

Rationale: people look for patterns when reading code. Someone would look at
this, and wonder why this one has optional functions and the others don't. Maybe
they'd draw meaning, when in fact its just the opinion of the two authors.

lgtm once thats changed to match the rest of the function.

[nduca, 2015-06-02 00:36:14]

lgtm

[benjhayden, 2015-06-02 17:39:10]

https://codereview.appspot.com/241860043/diff/20001/trace_viewer/base/range_u...
File trace_viewer/base/range_utils.html (right):

https://codereview.appspot.com/241860043/diff/20001/trace_viewer/base/range_u...
trace_viewer/base/range_utils.html:94: if (!startFunction)
On 2015/06/02 00:36:13, nduca wrote:
> just require the startFunction and endFunction. The explicit one above does.

Sorry, it doesn't look to me like mergeExplicitRanges requires
start/endFunctions:

function mergeExplicitRanges(inRanges, mergeThreshold, mergeFunction,
opt_startFunction, opt_endFunction)

[benjhayden, 2015-06-02 17:39:42]

Committed patchset #3 (id:40001) manually as
e9dc685064f50a33080cef797e25e68519118cbb (presubmit successful).