[tracing] Defer setting model until timeline-view is attached

[tracing] Defer setting model until timeline-view is attached

Setting the model on tr-ui-timeline-view before the element is
attached causes a crash, this happens (for example) if we open
a trace then quickly switch away to another tab.

BUG=catapult:#3184

[hjd, 2017-02-10 12:35:50]

Description was changed from

==========
[tracing] Hack to make tracing not crash when loading offscreen

BUG=catapult:#3184
==========

to

==========
[tracing] Don't set model before timeline-view is attached

Setting the model on tr-ui-timeline-view before the element is
attached causes a crash, this happens (for example) if we open
a trace then quickly switch away to another tab. This CL waits
for at least one frame before setting the model which fixes the
bug. I'm sure there are better fixes if we refactored
tr-ui-timeline-view but I don't understand that code well enough
to do that right now.

BUG=catapult:#3184
==========

[hjd, 2017-02-10 12:35:50]

hjd@chromium.org changed reviewers:
+ benjhayden@chromium.org

[hjd, 2017-02-10 12:36:04]

ptal, thanks :)

[benjhayden, 2017-02-10 17:26:13]

On 2017/02/10 at 12:36:04, hjd wrote:
> ptal, thanks :)

I think that the right way to do this might be to use Polymer's isAttached flag
and attached() callback like breakdown-span:
https://github.com/catapult-project/catapult/blob/540c2319b770c51f285bf293290...
That would require moving most of timeline-view's |set model()| to a new method
|updateContents_()|, as per usual for most of the rest of tracing/ui.
Do you want to try that instead?

[hjd, 2017-02-13 12:07:14]

Description was changed from

==========
[tracing] Don't set model before timeline-view is attached

Setting the model on tr-ui-timeline-view before the element is
attached causes a crash, this happens (for example) if we open
a trace then quickly switch away to another tab. This CL waits
for at least one frame before setting the model which fixes the
bug. I'm sure there are better fixes if we refactored
tr-ui-timeline-view but I don't understand that code well enough
to do that right now.

BUG=catapult:#3184
==========

to

==========
[tracing] Defer setting model until timeline-view is attached

Setting the model on tr-ui-timeline-view before the element is
attached causes a crash, this happens (for example) if we open
a trace then quickly switch away to another tab.

BUG=catapult:#3184
==========

[hjd, 2017-02-13 12:16:45]

On 2017/02/10 17:26:13, benjhayden wrote:
> On 2017/02/10 at 12:36:04, hjd wrote:
> > ptal, thanks :)
> 
> I think that the right way to do this might be to use Polymer's isAttached
flag
> and attached() callback like breakdown-span:
>
https://github.com/catapult-project/catapult/blob/540c2319b770c51f285bf293290...
> That would require moving most of timeline-view's |set model()| to a new
method
> |updateContents_()|, as per usual for most of the rest of tracing/ui.
> Do you want to try that instead?

Okay I updated the patch! It does seem better, thanks. I couldn't follow the
exact
pattern in breakdown_span as timeline_view doesn't remember the model but it is
quite similar.

[benjhayden, 2017-02-15 20:58:50]

On 2017/02/13 at 12:16:45, hjd wrote:
> On 2017/02/10 17:26:13, benjhayden wrote:
> > On 2017/02/10 at 12:36:04, hjd wrote:
> > > ptal, thanks :)
> > 
> > I think that the right way to do this might be to use Polymer's isAttached
flag
> > and attached() callback like breakdown-span:
> >
https://github.com/catapult-project/catapult/blob/540c2319b770c51f285bf293290...
> > That would require moving most of timeline-view's |set model()| to a new
method
> > |updateContents_()|, as per usual for most of the rest of tracing/ui.
> > Do you want to try that instead?
> 
> Okay I updated the patch! It does seem better, thanks. I couldn't follow the
exact
> pattern in breakdown_span as timeline_view doesn't remember the model but it
is quite similar.

Is there a reason why timeline-view shouldn't remember the model?
The semantics of a first-class field like "this.model_" seem simpler than a
field like "deferredModelToSet_", which is sometimes defined and sometimes
undefined.
Adding model_ to timeline-view would also be consistent with apeliotes' plan to
hoist the model from timeline-track-view up to timeline-view so that dashboard
can use timeline-track-view. I'll share the doc with you separately since it's
google.com.

[hjd, 2017-02-15 21:59:35]

On 2017/02/15 20:58:50, benjhayden wrote:
> On 2017/02/13 at 12:16:45, hjd wrote:
> > On 2017/02/10 17:26:13, benjhayden wrote:
> > > On 2017/02/10 at 12:36:04, hjd wrote:
> > > > ptal, thanks :)
> > > 
> > > I think that the right way to do this might be to use Polymer's isAttached
> flag
> > > and attached() callback like breakdown-span:
> > >
>
https://github.com/catapult-project/catapult/blob/540c2319b770c51f285bf293290...
> > > That would require moving most of timeline-view's |set model()| to a new
> method
> > > |updateContents_()|, as per usual for most of the rest of tracing/ui.
> > > Do you want to try that instead?
> > 
> > Okay I updated the patch! It does seem better, thanks. I couldn't follow the
> exact
> > pattern in breakdown_span as timeline_view doesn't remember the model but it
> is quite similar.
> 
> Is there a reason why timeline-view shouldn't remember the model?
> The semantics of a first-class field like "this.model_" seem simpler than a
> field like "deferredModelToSet_", which is sometimes defined and sometimes
> undefined.
> Adding model_ to timeline-view would also be consistent with apeliotes' plan
to
> hoist the model from timeline-track-view up to timeline-view so that dashboard
> can use timeline-track-view. I'll share the doc with you separately since it's
> http://google.com.

It adds a new reference to existing state that we have to avoid desynchronizing.

For example if someone changes model_  on timeline-track-view currently
that change is reflected everywhere but if timeline-view has its own record of
model_ instead we'll end up in a confusing situation.

However if it is only temporary while we move model out of timeline-track-view
then it doesn't seem so bad.

[benjhayden, 2017-02-17 22:52:59]

On 2017/02/15 at 21:59:35, hjd wrote:
> On 2017/02/15 20:58:50, benjhayden wrote:
> > On 2017/02/13 at 12:16:45, hjd wrote:
> > > On 2017/02/10 17:26:13, benjhayden wrote:
> > > > On 2017/02/10 at 12:36:04, hjd wrote:
> > > > > ptal, thanks :)
> > > > 
> > > > I think that the right way to do this might be to use Polymer's
isAttached
> > flag
> > > > and attached() callback like breakdown-span:
> > > >
> >
https://github.com/catapult-project/catapult/blob/540c2319b770c51f285bf293290...
> > > > That would require moving most of timeline-view's |set model()| to a new
> > method
> > > > |updateContents_()|, as per usual for most of the rest of tracing/ui.
> > > > Do you want to try that instead?
> > > 
> > > Okay I updated the patch! It does seem better, thanks. I couldn't follow
the
> > exact
> > > pattern in breakdown_span as timeline_view doesn't remember the model but
it
> > is quite similar.
> > 
> > Is there a reason why timeline-view shouldn't remember the model?
> > The semantics of a first-class field like "this.model_" seem simpler than a
> > field like "deferredModelToSet_", which is sometimes defined and sometimes
> > undefined.
> > Adding model_ to timeline-view would also be consistent with apeliotes' plan
to
> > hoist the model from timeline-track-view up to timeline-view so that
dashboard
> > can use timeline-track-view. I'll share the doc with you separately since
it's
> > http://google.com.
> 
> It adds a new reference to existing state that we have to avoid
desynchronizing.
> 
> For example if someone changes model_  on timeline-track-view currently
> that change is reflected everywhere but if timeline-view has its own record of
> model_ instead we'll end up in a confusing situation.
> 
> However if it is only temporary while we move model out of timeline-track-view
> then it doesn't seem so bad.

Apologies for the delay!
Hm, temporary code isn't. It hasn't yet been decided who will be taking over
apeliotes' plan for hoisting model to timeline-view.
But I think that, in this case, the risk of desyncing timeline-view.model from
timeline-track-view.model is mitigated by timeline-view's |set model()| which
propagates the new model to its timeline-track-view.
Does that sound right?
Please feel free to schedule a VC!

[benjhayden, 2017-02-22 19:59:31]

1 nit then lgtm if you're happy with it :-)

https://codereview.chromium.org/2671813006/diff/80001/tracing/tracing/ui/time...
File tracing/tracing/ui/timeline_view.html (right):

https://codereview.chromium.org/2671813006/diff/80001/tracing/tracing/ui/time...
tracing/tracing/ui/timeline_view.html:385: var modelInstanceChanged = this.model
!== this.trackViewModel_();
I think it might be confusing to have both |get model| and |trackViewModel_|.
How does this look?
var modelInstanceChanged = (this.trackView_ === undefined) || (this.model !==
this.trackView_.model);

[hjd, 2017-02-22 20:15:34]

On 2017/02/22 19:59:31, benjhayden wrote:
> 1 nit then lgtm if you're happy with it :-)
> 
>
https://codereview.chromium.org/2671813006/diff/80001/tracing/tracing/ui/time...
> File tracing/tracing/ui/timeline_view.html (right):
> 
>
https://codereview.chromium.org/2671813006/diff/80001/tracing/tracing/ui/time...
> tracing/tracing/ui/timeline_view.html:385: var modelInstanceChanged =
this.model
> !== this.trackViewModel_();
> I think it might be confusing to have both |get model| and |trackViewModel_|.
> How does this look?
> var modelInstanceChanged = (this.trackView_ === undefined) || (this.model !==
> this.trackView_.model);

sgtm, done thanks!

[hjd, 2017-02-22 20:16:00]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-02-22 20:16:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org
Link to the patchset: https://codereview.chromium.org/2671813006/#ps100001
(title: "remove trackViewModel_")

[commit-bot: I haz the power, 2017-02-22 20:16:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:32:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:32:14]

Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[hjd, 2017-02-27 12:21:50]

On 2017/02/22 20:32:14, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[Note to self: Landing is blocked on fixing a test, see
https://github.com/catapult-project/catapult/issues/3297]