Navigate using FindController

Navigate using FindController

This allows navigating the timeline by entering a string in
the find input box in the format of (timestamp)@(pid).(tid)x(scaleX).
The pid and tid will be brought into view, and zoom will be applied.

This also adds new functions to Location class:
- loading from stableId and timestamp
- getters for outer most track and timestamp
As well as a new UIState class that contains a Location specifying the
point of interest on the timeline and the current scaleX value of the
timeline.

BUG=17
R=nduca@chromium.org

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

[nduca, 2015-03-12 05:32:15]

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
File trace_viewer/core/find_control.html (left):

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_control.html:115: this.filterTextChanged();
can this be a separate change? not a big deal if not, but its logically
separate.

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
File trace_viewer/core/find_controller.html (right):

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:34: get model() {
get model() {
 return this.timeline.modle;
}

you dont need a setter

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:62: getFilterTask_: function() {
createFilterTask_: function(filterText) --- dont read the filtertext yet again
its confusing

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:65: var promise = Promise.resolve();
i dont get why this is returning a promise when you say getXTask. Either it
returns a task or a promise, please be precise.

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:103: stateFromString =
tv.c.UIState.fromUserFriendlyString(
er, how does this not throw up an error dialog if you just try to find a vanilla
string?


and what test coverage is missing caused us to not notice this failure?

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:109: overlay.visible = true;
shouldn't you return here? its pretty awkward to have an exception handler let
the function keep on going.

[nduca, 2015-03-12 05:32:46]

also either fill inBUG=  ot be BUG=#733 or whatevr bug id you have on github. Or
remove the line entirely.

[jackyang, 2015-03-12 14:37:17]

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
File trace_viewer/core/find_control.html (left):

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_control.html:115: this.filterTextChanged();
On 2015/03/12 05:32:15, nduca wrote:
> can this be a separate change? not a big deal if not, but its logically
> separate.

Sounds good. I'll commit this directly since it's only one line and you've
already have taken a look; I don't think it's worth creating a new review for
it.

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
File trace_viewer/core/find_controller.html (right):

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:34: get model() {
On 2015/03/12 05:32:15, nduca wrote:
> get model() {
>  return this.timeline.modle;
> }
> 
> you dont need a setter

Done.

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:62: getFilterTask_: function() {
On 2015/03/12 05:32:15, nduca wrote:
> createFilterTask_: function(filterText) --- dont read the filtertext yet again
> its confusing

Done.

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:65: var promise = Promise.resolve();
On 2015/03/12 05:32:15, nduca wrote:
> i dont get why this is returning a promise when you say getXTask. Either it
> returns a task or a promise, please be precise.

Done. Renamed to getFilterPromise_

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:103: stateFromString =
tv.c.UIState.fromUserFriendlyString(
On 2015/03/12 05:32:15, nduca wrote:
> er, how does this not throw up an error dialog if you just try to find a
vanilla
> string?
> 
> 
> and what test coverage is missing caused us to not notice this failure?

I intended for fromUserFriendlyString to only throw if the string is
syntactically valid but the values in the string are invalid. That is:
if (no regex match) return undefined;
else if (scaleX <= 0) throw;
else if (no results for pid.tid in model) throw;
else return new UIState(..);

I don't really see a better approach to make this more intuitive.
In the meantime I've changed the comment for this to be more clear.

https://codereview.appspot.com/210660043/diff/20001/trace_viewer/core/find_co...
trace_viewer/core/find_controller.html:109: overlay.visible = true;
On 2015/03/12 05:32:15, nduca wrote:
> shouldn't you return here? its pretty awkward to have an exception handler let
> the function keep on going.
> 

Done.

[nduca, 2015-03-19 01:33:18]

lgtm! sorry for the delay i didn't see the review email! feel free to ping me
sooner in the future!

[jackyang, 2015-03-19 14:55:49]

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