Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2252313002:
[Media Router WebUI] Use updated sink on click (Closed)
Description[Media Router WebUI] Use updated sink on click
Since crrev.com/2221933003, the dom-repeat element that creates the sink
list doesn't seem to correctly update its element-item mapping. Instead
we will use the most current sink object with the same ID in our own
map.
BUG=637222
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7e3c3e80f15d513f9c2fe7a59b1854c7e1ff47e9
Cr-Commit-Position: refs/heads/master@{#413573}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix root cause by copying updated sink #Messages
Total messages: 24 (9 generated)
Description was changed from ========== [Media Router WebUI] Use updated sink on click Since crrev.com/2221933003, the dom-repeat element that creates the sink list doesn't seem to correctly update its element-item mapping. Instead we will use the most current sink object with the same ID in our own map. BUG=637222 ========== to ========== [Media Router WebUI] Use updated sink on click Since crrev.com/2221933003, the dom-repeat element that creates the sink list doesn't seem to correctly update its element-item mapping. Instead we will use the most current sink object with the same ID in our own map. BUG=637222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
btolsch@chromium.org changed reviewers: + imcheng@chromium.org
Hi Derek, PTAL but if you won't be able to look at this I can also ask Mark. Thanks!
btolsch@chromium.org changed reviewers: + mfoltz@chromium.org
I'm not sure if Derek will see this and it'd be nice to fix this soon. Would you be able to take a look, Mark?
https://codereview.chromium.org/2252313002/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2252313002/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1768: // again. I'm not a Polymer expert but this seems like a workaround and not the real bug fix. Reviewing the documentation, I had some questions: 1. The dom-repeat should re-render itself if is configured to observe changes to the model (observe property). 2. The normal way to use events on stamped items is to look up the model object ('binding scope') associated with the item, not the DOM element. Why do we need to look things up in our own mapping objects based on a DOM id? https://www.polymer-project.org/1.0/docs/api/dom-repeat
Root cause found, PTAL. https://codereview.chromium.org/2252313002/diff/1/chrome/browser/resources/me... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2252313002/diff/1/chrome/browser/resources/me... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1768: // again. On 2016/08/19 17:21:46, mark a. foltz wrote: > I'm not a Polymer expert but this seems like a workaround and not the real bug > fix. > > Reviewing the documentation, I had some questions: > > 1. The dom-repeat should re-render itself if is configured to observe changes to > the model (observe property). > 2. The normal way to use events on stamped items is to look up the model object > ('binding scope') associated with the item, not the DOM element. Why do we need > to look things up in our own mapping objects based on a DOM id? > > https://www.polymer-project.org/1.0/docs/api/dom-repeat This is indeed a workaround. I took another look at Jennifer's CL and found the root cause. The proper fix is in PS2. 1. If you look at L2032, that is what we have always used to trigger the dom-repeat render; instead of directly mutating the array with polymer's shift/splice/etc. we just assign a new array. From the documentation, I think the |observe| property is just for filter and sort. 2. Maybe I'm misunderstanding, but |onSinkClick_| gets a DOM element so we would still need to call |this.$$('#sinkList').modelForElement(event.target)| and then call |model.get('item')|. Calling |itemForElement| shortcuts this. Is there a different idea or suggestion you had?
On 2016/08/19 at 18:04:04, btolsch wrote: > Root cause found, PTAL. > > https://codereview.chromium.org/2252313002/diff/1/chrome/browser/resources/me... > File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): > > https://codereview.chromium.org/2252313002/diff/1/chrome/browser/resources/me... > chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1768: // again. > On 2016/08/19 17:21:46, mark a. foltz wrote: > > I'm not a Polymer expert but this seems like a workaround and not the real bug > > fix. > > > > Reviewing the documentation, I had some questions: > > > > 1. The dom-repeat should re-render itself if is configured to observe changes to > > the model (observe property). > > 2. The normal way to use events on stamped items is to look up the model object > > ('binding scope') associated with the item, not the DOM element. Why do we need > > to look things up in our own mapping objects based on a DOM id? > > > > https://www.polymer-project.org/1.0/docs/api/dom-repeat > > This is indeed a workaround. I took another look at Jennifer's CL and found the root cause. The proper fix is in PS2. This fix makes more sense. > 1. If you look at L2032, that is what we have always used to trigger the dom-repeat render; instead of directly mutating the array with polymer's shift/splice/etc. we just assign a new array. From the documentation, I think the |observe| property is just for filter and sort. > 2. Maybe I'm misunderstanding, but |onSinkClick_| gets a DOM element so we would still need to call |this.$$('#sinkList').modelForElement(event.target)| and then call |model.get('item')|. Calling |itemForElement| shortcuts this. Is there a different idea or suggestion you had? No that explanation makes sense. Polymer seems to have a couple of different ways of navigating custom properties. I need to familiarize myself further with it.
lgtm
The CQ bit was checked by mfoltz@chromium.org
lgtm
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by btolsch@chromium.org
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
It looks like there is a flaky/failing test on linux_android_rel_ng unrelated to this. Can someone try pushing this again on Monday if that's cleared up? This also needs to be merged back to 53, which I can't do now that this didn't hit master. Can someone please do that as well while I'm gone?
The CQ bit was checked by mfoltz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router WebUI] Use updated sink on click Since crrev.com/2221933003, the dom-repeat element that creates the sink list doesn't seem to correctly update its element-item mapping. Instead we will use the most current sink object with the same ID in our own map. BUG=637222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router WebUI] Use updated sink on click Since crrev.com/2221933003, the dom-repeat element that creates the sink list doesn't seem to correctly update its element-item mapping. Instead we will use the most current sink object with the same ID in our own map. BUG=637222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7e3c3e80f15d513f9c2fe7a59b1854c7e1ff47e9 Cr-Commit-Position: refs/heads/master@{#413573} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7e3c3e80f15d513f9c2fe7a59b1854c7e1ff47e9 Cr-Commit-Position: refs/heads/master@{#413573} |
