|
|
Created:
3 years, 10 months ago by hjd Modified:
3 years, 2 months ago Reviewers:
benjhayden CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[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
Patch Set 1 #Patch Set 2 : defer setting model until attached #Patch Set 3 : reset trace_viewer.html #Patch Set 4 : reset trace_viewer.html for real #Patch Set 5 : set model on timeline-view #
Total comments: 1
Patch Set 6 : remove trackViewModel_ #Messages
Total messages: 17 (6 generated)
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@chromium.org changed reviewers: + benjhayden@chromium.org
ptal, thanks :)
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?
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 ==========
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.
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.
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.
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!
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);
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!
The CQ bit was checked by hjd@chromium.org
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_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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] |