tquery: Remove hard dependency on UI

tquery: Remove hard dependency on UI

Make it possible to use tquery without depending on UI objects
(BrushingStateController). Also unbreak tquery in the process.

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4d7eedc5a080bf868fcb9c4053d023cde5a46aa6

[Sami, 2015-10-15 16:58:36]

skyostil@chromium.org changed reviewers:
+ petrcermak@chromium.org

[petrcermak, 2015-10-15 17:38:42]

Looks good overall. Here's a bunch of comments.

One more nit: I personally prefer "UI" over "ui" (ignore unless you are
indifferent).

Thanks,
Petr

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
File tracing/tracing/core/scripting_controller.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
tracing/tracing/core/scripting_controller.html:45: }.bind(this));
no need to use bind, you can just use the second optional parameter of
Array.prototype.forEach:

this.scriptObjectValues_.forEach(..., this);

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
File tracing/tracing/extras/tquery/tquery.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery.html:28: for (var i = 0; i <
event.subSlices.length; i++)
Alternatively, you could do the following:

event.subSlices.forEach(addEventTreeToSelection.bind(undefined, selection));

I leave the choice up to you :-)

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery.html:153: function(event) {
FYI, this is addEventTreeToSelection.bind(undefined, selection). Feel Free to
ignore.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery.html:156: } }(container), this);
'container' is available in the closure and it's never modified, so there is no
need to pass it as an argument in an extra wrapped function (which makes the
code less readable and potentially slower).

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
File tracing/tracing/extras/tquery/tquery_test.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:13: <link rel="import"
href="/tracing/model/event_set.html">
I don't think the following imports are necessary anymore:

event_target
task
event_set

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:20: function
createTestModel(sliceCount) {
This could possibly reuse createTestModelWithSlices. Feel free to skip this for
performance reasons.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:49: return
tquery.ready().then(function(selection) {
Does this actually run the function before the test exits (given that
tquery.ready() a task)?

[Sami, 2015-10-15 18:03:22]

Thanks for the comments; all addressed.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
File tracing/tracing/core/scripting_controller.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
tracing/tracing/core/scripting_controller.html:45: }.bind(this));
On 2015/10/15 17:38:42, petrcermak wrote:
> no need to use bind, you can just use the second optional parameter of
> Array.prototype.forEach:
> 
> this.scriptObjectValues_.forEach(..., this);

Done.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
File tracing/tracing/extras/tquery/tquery.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery.html:28: for (var i = 0; i <
event.subSlices.length; i++)
On 2015/10/15 17:38:42, petrcermak wrote:
> Alternatively, you could do the following:
> 
> event.subSlices.forEach(addEventTreeToSelection.bind(undefined, selection));
> 
> I leave the choice up to you :-)

That's pretty concise, I'll steal it.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery.html:156: } }(container), this);
On 2015/10/15 17:38:42, petrcermak wrote:
> 'container' is available in the closure and it's never modified, so there is
no
> need to pass it as an argument in an extra wrapped function (which makes the
> code less readable and potentially slower).

Ah, well spotted. Fixed.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
File tracing/tracing/extras/tquery/tquery_test.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:13: <link rel="import"
href="/tracing/model/event_set.html">
On 2015/10/15 17:38:42, petrcermak wrote:
> I don't think the following imports are necessary anymore:
> 
> event_target
> task
> event_set

Thanks, didn't notice that.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:20: function
createTestModel(sliceCount) {
On 2015/10/15 17:38:42, petrcermak wrote:
> This could possibly reuse createTestModelWithSlices. Feel free to skip this
for
> performance reasons.

Good idea, cuts down on repetition.

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:49: return
tquery.ready().then(function(selection) {
On 2015/10/15 17:38:42, petrcermak wrote:
> Does this actually run the function before the test exits (given that
> tquery.ready() a task)?

Yes, the test harness has a special hack^Whook that knows to wait for a
{Promise} to complete if the test returns one. I checked this by changing the
expectation, which made the test fail.

[petrcermak, 2015-10-15 18:28:00]

LGTM with a few more comments.

Thanks,
Petr

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
File tracing/tracing/core/scripting_controller.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
tracing/tracing/core/scripting_controller.html:45: }.bind(this));
On 2015/10/15 18:03:22, Sami wrote:
> On 2015/10/15 17:38:42, petrcermak wrote:
> > no need to use bind, you can just use the second optional parameter of
> > Array.prototype.forEach:
> > 
> > this.scriptObjectValues_.forEach(..., this);
> 
> Done.

Not on this line though :-)

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
File tracing/tracing/extras/tquery/tquery_test.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/extras/tque...
tracing/tracing/extras/tquery/tquery_test.html:49: return
tquery.ready().then(function(selection) {
On 2015/10/15 18:03:22, Sami wrote:
> On 2015/10/15 17:38:42, petrcermak wrote:
> > Does this actually run the function before the test exits (given that
> > tquery.ready() a task)?
> 
> Yes, the test harness has a special hack^Whook that knows to wait for a
> {Promise} to complete if the test returns one. I checked this by changing the
> expectation, which made the test fail.

I had no idea about that. Thanks for the explanation.

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/core/sc...
File tracing/tracing/core/scripting_object.html (right):

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/core/sc...
tracing/tracing/core/scripting_object.html:8: <link rel="import"
href="/tracing/base/utils.html">
I'd suggest changing this import to /tracing/base/base.html (since you don't
need anything specific).

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
File tracing/tracing/ui/scripting_control.html (right):

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
tracing/tracing/ui/scripting_control.html:8: <link rel="import"
href="/tracing/base/task.html">
No need for this import anymore.

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
tracing/tracing/ui/scripting_control.html:9: <link rel="import"
href="/tracing/core/scripting_controller.html">
ditto I think

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
tracing/tracing/ui/scripting_control.html:10: <link rel="import"
href="/tracing/ui/timeline_track_view.html">
Do you need this?

[Sami, 2015-10-15 18:34:39]

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
File tracing/tracing/core/scripting_controller.html (right):

https://codereview.chromium.org/1398213005/diff/1/tracing/tracing/core/script...
tracing/tracing/core/scripting_controller.html:45: }.bind(this));
On 2015/10/15 18:28:00, petrcermak wrote:
> On 2015/10/15 18:03:22, Sami wrote:
> > On 2015/10/15 17:38:42, petrcermak wrote:
> > > no need to use bind, you can just use the second optional parameter of
> > > Array.prototype.forEach:
> > > 
> > > this.scriptObjectValues_.forEach(..., this);
> > 
> > Done.
> 
> Not on this line though :-)

Heh, off-by-a-function. Fixed.

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/core/sc...
File tracing/tracing/core/scripting_object.html (right):

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/core/sc...
tracing/tracing/core/scripting_object.html:8: <link rel="import"
href="/tracing/base/utils.html">
On 2015/10/15 18:28:00, petrcermak wrote:
> I'd suggest changing this import to /tracing/base/base.html (since you don't
> need anything specific).

Right, I just need |tr| I think.

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
File tracing/tracing/ui/scripting_control.html (right):

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
tracing/tracing/ui/scripting_control.html:8: <link rel="import"
href="/tracing/base/task.html">
On 2015/10/15 18:28:00, petrcermak wrote:
> No need for this import anymore.

Oops, thanks. I wish the linter did this for us.

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
tracing/tracing/ui/scripting_control.html:9: <link rel="import"
href="/tracing/core/scripting_controller.html">
On 2015/10/15 18:28:00, petrcermak wrote:
> ditto I think

Done.

https://codereview.chromium.org/1398213005/diff/20001/tracing/tracing/ui/scri...
tracing/tracing/ui/scripting_control.html:10: <link rel="import"
href="/tracing/ui/timeline_track_view.html">
On 2015/10/15 18:28:00, petrcermak wrote:
> Do you need this?

Not anymore.

[Sami, 2015-10-15 18:34:44]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-10-15 18:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1398213005/#ps40001
(title: "More review tweaks.")

[commit-bot: I haz the power, 2015-10-15 18:34:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1398213005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1398213005/40001

[commit-bot: I haz the power, 2015-10-15 18:41:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...