Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2653693002:
[tracing] Make tab-view rerender after reset/clear
|
Created:
3 years, 11 months ago by hjd Modified:
3 years, 10 months ago Reviewers:
charliea (OOO until 10-5) CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[tracing] Make tab-view rerender after reset/clear
If you remove then re-add a subView from a tab-view within a single
frame (as the memory-infra heap viewer) does then the tab-view does not
re-render the tab headers. This is expected Polymer behavior since
although the paths have changed the actual subView object has not so the
dirty checker refuses to update the dom.
You could get around this by depending directly on the correct
attributes of the subView in tab-view (tabLabel etc) and using
notifyPath but this doesn't work if the tab is detached when changes
happen.
Instead this CL wraps the subViews in a dummy object which we can
recreate at will to fool the dirty checker and force the template to
re-render.
BUG=catapult:#2750
Patch Set 1 #
Total comments: 11
Patch Set 2 : [tracing] Make tab-view rerender after reset/clear #
Total comments: 2
Messages
Total messages: 12 (1 generated)
hjd@chromium.org changed reviewers: + charliea@chromium.org
ptal, thanks! :)
https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (left): https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> Does this also solve https://github.com/catapult-project/catapult/issues/2754? https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> I see in the CL description that you considered observing subViews_.* instead of just subViews, but that didn't work because if the tab was detached while the change was made, the path wouldn't be notified (which makes sense to me). Wouldn't the path be updated when that tab was attached though, and things would work as expected? https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:79: <div id='tabs' hidden="[[tabsHidden]]"> nit: consistently use " over ' It does indeed look common: https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... versus https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:145: console.log('set selectedSubView', subView); nit: don't want console.logs in production code https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:276: return { subView: subView }; This can just be return { subView }; This matches the airbnb style guide that we're moving towards bettter (https://github.com/airbnb/javascript#es6-object-concise)
https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (left): https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> On 2017/01/24 01:01:04, charliea wrote: > Does this also solve https://github.com/catapult-project/catapult/issues/2754 Yep! :) https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> On 2017/01/24 01:01:04, charliea wrote: > I see in the CL description that you considered observing subViews_.* instead of > just subViews, but that didn't work because if the tab was detached while the > change was made, the path wouldn't be notified (which makes sense to me). > Wouldn't the path be updated when that tab was attached though, and things would > work as expected? I think so although I didn't try it. (I was so frustrated with Polymer by the end of yesterday that I just uploaded the first thing that worked...) I was worried that even if it did work it would require hacky changes to memory_dump_heap_details_breakdown_view.html but looking today maybe it wouldn't be so bad. I will try it and see. https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:79: <div id='tabs' hidden="[[tabsHidden]]"> On 2017/01/24 01:01:04, charliea wrote: > nit: consistently use " over ' > > It does indeed look common: > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... > versus > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... Done. https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:145: console.log('set selectedSubView', subView); On 2017/01/24 01:01:04, charliea wrote: > nit: don't want console.logs in production code oops, I thought I removed that. Thanks! https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:276: return { subView: subView }; On 2017/01/24 01:01:04, charliea wrote: > This can just be > > return { subView }; > > This matches the airbnb style guide that we're moving towards bettter > (https://github.com/airbnb/javascript#es6-object-concise) Yeah I keep forgetting :( Done.
On 2017/01/24 09:30:26, hjd wrote: > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > File tracing/tracing/ui/base/tab_view.html (left): > > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> > On 2017/01/24 01:01:04, charliea wrote: > > Does this also solve https://github.com/catapult-project/catapult/issues/2754 > Yep! :) > > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > File tracing/tracing/ui/base/tab_view.html (right): > > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> > On 2017/01/24 01:01:04, charliea wrote: > > I see in the CL description that you considered observing subViews_.* instead > of > > just subViews, but that didn't work because if the tab was detached while the > > change was made, the path wouldn't be notified (which makes sense to me). > > Wouldn't the path be updated when that tab was attached though, and things > would > > work as expected? > > I think so although I didn't try it. (I was so frustrated with Polymer by the > end of yesterday that I just uploaded the first thing that worked...) I was > worried that even if it did work it would require hacky changes to > memory_dump_heap_details_breakdown_view.html but looking today maybe it wouldn't > be so bad. I will try it and see. I think I understand the problem now. Imagine a tab in a tab-view, we expect that if the somewhere within the tab header's dom-repeat[1] we depend on item.tabLabel and then the tab calls notifyPath('tabLabel') that that part of the tab header should update. Unfortunately this doesn't happen because the tab element isn't within the dom-repeat. When notifyPath('tabLabel') is called the tab generates a tab-label-changed event which is passed to the tab-view (if the tab is the selected tab and hence attached to the dom) or disappears (if the tab is not the selected tab). The dom-repeat never receives the event and so has no chance to update. The solutions I see are: 1) Update all tab-headers whenever subViews.splices changes (as in this cl). Pros: Simple Cons: Kind of brute force, also we would have to listen to subViews.* to correctly update in some circumstances. 2) Have tab view listen for a custom 'tab-info-changed' event which would be fired by a tab. Pros: Clearer and less magical Cons: It's still going to be super awkward to force the tab-view to update 3) Manually set up listeners for all subViews then redirect those events to the dom-repeat so it can to the right thing Pros: Most precise solution in terms of change tracking Cons: Fiddly and complicated to setup and maintain. What are your thoughts? :) [1]: https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/u... > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > tracing/tracing/ui/base/tab_view.html:79: <div id='tabs' > hidden="[[tabsHidden]]"> > On 2017/01/24 01:01:04, charliea wrote: > > nit: consistently use " over ' > > > > It does indeed look common: > > > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... > > versus > > > https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/... > > Done. > > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > tracing/tracing/ui/base/tab_view.html:145: console.log('set selectedSubView', > subView); > On 2017/01/24 01:01:04, charliea wrote: > > nit: don't want console.logs in production code > > oops, I thought I removed that. Thanks! > > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > tracing/tracing/ui/base/tab_view.html:276: return { subView: subView }; > On 2017/01/24 01:01:04, charliea wrote: > > This can just be > > > > return { subView }; > > > > This matches the airbnb style guide that we're moving towards bettter > > (https://github.com/airbnb/javascript#es6-object-concise) > > Yeah I keep forgetting :( Done.
https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> On 2017/01/24 09:30:25, hjd wrote: > On 2017/01/24 01:01:04, charliea wrote: > > I see in the CL description that you considered observing subViews_.* instead > of > > just subViews, but that didn't work because if the tab was detached while the > > change was made, the path wouldn't be notified (which makes sense to me). > > Wouldn't the path be updated when that tab was attached though, and things > would > > work as expected? > > I think so although I didn't try it. (I was so frustrated with Polymer by the > end of yesterday that I just uploaded the first thing that worked...) I was > worried that even if it did work it would require hacky changes to > memory_dump_heap_details_breakdown_view.html but looking today maybe it wouldn't > be so bad. I will try it and see. Sounds good. I definitely think that way is the way that Polymer intends for it to be done. In my experience, Polymer can work great if you work the way it wants, but can make your life really miserable if you try to do otherwise (like much of tracing does, for little reason), so my vote is to try and follow the Polymer way as much as possible
On 2017/01/24 22:46:59, charliea wrote: > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > File tracing/tracing/ui/base/tab_view.html (right): > > https://codereview.chromium.org/2653693002/diff/1/tracing/tracing/ui/base/tab... > tracing/tracing/ui/base/tab_view.html:1: <!DOCTYPE html> > On 2017/01/24 09:30:25, hjd wrote: > > On 2017/01/24 01:01:04, charliea wrote: > > > I see in the CL description that you considered observing subViews_.* > instead > > of > > > just subViews, but that didn't work because if the tab was detached while > the > > > change was made, the path wouldn't be notified (which makes sense to me). > > > Wouldn't the path be updated when that tab was attached though, and things > > would > > > work as expected? > > > > I think so although I didn't try it. (I was so frustrated with Polymer by the > > end of yesterday that I just uploaded the first thing that worked...) I was > > worried that even if it did work it would require hacky changes to > > memory_dump_heap_details_breakdown_view.html but looking today maybe it > wouldn't > > be so bad. I will try it and see. > > Sounds good. I definitely think that way is the way that Polymer intends for it > to be done. In my experience, Polymer can work great if you work the way it > wants, but can make your life really miserable if you try to do otherwise (like > much of tracing does, for little reason), so my vote is to try and follow the > Polymer way as much as possible Yeah :( I think I understand the problem better now, It's kind of an unintuitive side effect of the fact we are iterating over elements but not actually adding them to the dom. Imagine a tab in a tab-view, we expect that if the somewhere within the tab header's dom-repeat[1] we depend on item.tabLabel and then the tab calls notifyPath('tabLabel') that that part of the tab header should update. Unfortunately this doesn't happen because the tab element isn't within the dom-repeat. When notifyPath('tabLabel') is called the tab generates a tab-label-changed event which is passed to the tab-view (if the tab is the selected tab and hence attached to the dom) or disappears (if the tab is not the selected tab). The dom-repeat never receives the event and never gets a chance to update. The solutions I see are: 1) Update all tab-headers whenever subViews.* changes (as in this cl). Pros: Simple Cons: Brute force and ugly. 2) Have tab view listen for a custom 'tab-info-changed' event which would be fired by a tab. Pros: Less magically but still pretty simple Cons: It's still going to be super awkward to force the tab-view to update 3) Manually set up listeners for all subViews then redirect those events to the dom-repeat so it can to the right thing Pros: Most precise solution in terms of change tracking Cons: Fiddly and complicated to setup and maintain. What are your thoughts? :) [1]: https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/u...
On 2017/01/25 10:36:28, hjd wrote:
> Yeah :(
>
> I think I understand the problem better now,
>
> It's kind of an unintuitive side effect of the fact we are iterating over
> elements but not actually adding them to the dom.
>
> Imagine a tab in a tab-view, we expect that if the somewhere within the tab
> header's dom-repeat[1] we depend on item.tabLabel and then the tab calls
> notifyPath('tabLabel') that that part of the tab header should update.
>
Yep, that totally makes sense.
> Unfortunately this doesn't happen because the tab element isn't within the
> dom-repeat.
I'm not sure I follow. What do you mean by "the tab element isn't within the
dom-repeat"? Do you mean that we only actually bind subproperties of the tab
into the DOM, not the actual tab itself? I don't think that this should matter.
According to https://www.polymer-project.org/1.0/docs/api/dom-repeat,
"Notifications for changes to items sub-properties will be forwarded to template
instances, which will update via the normal structured data notification
system."
> When notifyPath('tabLabel') is called the tab generates a
> tab-label-changed event which is passed to the tab-view
> (if the tab is the selected tab and hence attached to the dom)
> or disappears (if the tab is not the selected tab). The dom-repeat never
> receives the event and never gets a chance to update.
As mentioned above, having the subproperties bound into the DOM should be
sufficient for the model changes to propagate into the DOM.
https://codereview.chromium.org/2653693002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2653693002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:274: return subViews_.map((subView) => { https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/A... seems to suggest that you should be able to do: return subViews_.map(subView => ({subView})); (the parentheses outside of the {subView} indicate that the braces indicate an object literal and not the scope of the function)
https://codereview.chromium.org/2653693002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view_test.html (right): https://codereview.chromium.org/2653693002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view_test.html:126: test('instantiate_twoTabsSwitch', function() { Does this test case actually reproduce the problem? I patched the CL locally and the patched version and master seem to produce results that are identical pixel-for-pixel
On 2017/01/25 21:41:34, charliea wrote:
> On 2017/01/25 10:36:28, hjd wrote:
> > Yeah :(
> >
> > I think I understand the problem better now,
> >
> > It's kind of an unintuitive side effect of the fact we are iterating over
> > elements but not actually adding them to the dom.
> >
> > Imagine a tab in a tab-view, we expect that if the somewhere within the tab
> > header's dom-repeat[1] we depend on item.tabLabel and then the tab calls
> > notifyPath('tabLabel') that that part of the tab header should update.
> >
>
> Yep, that totally makes sense.
>
> > Unfortunately this doesn't happen because the tab element isn't within the
> > dom-repeat.
>
> I'm not sure I follow. What do you mean by "the tab element isn't within the
> dom-repeat"? Do you mean that we only actually bind subproperties of the tab
> into the DOM, not the actual tab itself? I don't think that this should
matter.
Yes, exactly.
> According to https://www.polymer-project.org/1.0/docs/api/dom-repeat,
> "Notifications for changes to items sub-properties will be forwarded to
template
> instances, which will update via the normal structured data notification
> system."
I agree it sounds like it should work but I don't think it does.
For example http://plnkr.co/edit/9wOjlJkDDcPCD3xLjthm?p=preview
replicates the situation (see elements.html) and it doesn't seem to work.
> > When notifyPath('tabLabel') is called the tab generates a
> > tab-label-changed event which is passed to the tab-view
> > (if the tab is the selected tab and hence attached to the dom)
> > or disappears (if the tab is not the selected tab). The dom-repeat never
> > receives the event and never gets a chance to update.
>
> As mentioned above, having the subproperties bound into the DOM should be
> sufficient for the model changes to propagate into the DOM.
AHHHHHHHHH
- thank you for making the Plunker that demonstrates the problem -
HHHHHHHHHHH! I *HATE* the tab view!
I truly have no idea what to do in this situation. The root problem here is
that, before the time that trace viewer used Polymer, it used raw Javascript to
control the DOM, and Javascript has no problem letting you treat DOM nodes as
data. Polymer, on the other hand, seems to *hate* when you treat DOM nodes as
data, and makes your life as miserable as possible when you do so. I thought I'd
figured out a sort of ugly way around this when I wrote the new tab view, but it
looks like I was wrong :-(
I can imagine the tab view working in a different way, where the analysis-view
is given a piece of data, eventSet, that it then hands to tab-view.
tab-view then does something like:
<tabs>
<template is="dom-repeat" items="[[computeEventTypes(eventSet)]]">
<tab on-tap="onTabTapped">[[item.friendlyName]]</tab>
</template>
</tabs>
onTabTapped: function(item) {
// Fire an event upwards to tell the analysis view to switch tabs.
this.fire('onTabTapped', item);
}
The problem right now is that we're asking the tab view to treat the subviews as
data, and that's just not working at all :-( The tab view can't be responsible
for the sub view: it can only be responsible for the tabs. I can't figure out a
way to have it responsible for both in a way that doesn't involving binding
Polymer items as elements, which just doesn't work well.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
