Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1868983003:
[Media Router WebUI] Switch back to sink view after blocking issue is resolved. (Closed)
|
Created:
4 years, 8 months ago by apacible Modified:
4 years, 8 months ago Reviewers:
mark a. foltz CC:
arv+watch_chromium.org, chromium-reviews, media-router+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router WebUI] Switch back to sink view after blocking issue is resolved.
When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back.
Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved.
This change also fixes an issue where the issue banner's offsetHeight is always 0.
BUG=601502
Committed: https://crrev.com/fee1baea9fdb05cb5867f46eb1ffe5c850be9d9c
Cr-Commit-Position: refs/heads/master@{#387451}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Changes per mfoltz@'s comments. #
Total comments: 7
Patch Set 3 : Rebase. #Patch Set 4 : Changes per mfoltz@'s comments. #
Total comments: 2
Patch Set 5 : Rebase. #Patch Set 6 : Changes per mfoltz@'s comments. Fix styling bug. #
Messages
Total messages: 22 (12 generated)
Description was changed from ========== issue banner blocking BUG= ========== to ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. BUG=601502 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. BUG=601502 ========== to ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. BUG=601502 ==========
Patchset #1 (id:20001) has been deleted
apacible@chromium.org changed reviewers: + mfoltz@chromium.org
PTAL, thanks!
https://codereview.chromium.org/1868983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1227: if (!!newIssue && newIssue.isBlocking) { What if there is a new, non-blocking issue? What if there is a old issue? Is the assumption that newIssue will replace it? https://codereview.chromium.org/1868983003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1230: // Blocking issues cannot be replaced by a non-blocking issue. I didn't quite follow the comment (or the logic below). It seems like if there is no new issue you should always show the sink list (regardless of the old issue or not). Can the comment be clarified? https://codereview.chromium.org/1868983003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_sink_list_tests.js (right): https://codereview.chromium.org/1868983003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_sink_list_tests.js:182: test('sink list visibility non blocking issue', function(done) { Are there test cases for issue replacement (non-blocking with blocking, non-blocking with null, blocking with null)?
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1868983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1227: if (!!newIssue && newIssue.isBlocking) { > What if there is a new, non-blocking issue? > > What if there is a old issue? Is the assumption that newIssue will replace it? Non-blocking issues do not use the ISSUE view. A blocking issue cannot be replaced by a non-blocking issue. However, a non-blocking issue can be replaced by a blocking issue. This is all handled in the IssueManager (C++ side). https://codereview.chromium.org/1868983003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1230: // Blocking issues cannot be replaced by a non-blocking issue. On 2016/04/07 21:29:50, mark a. foltz wrote: > I didn't quite follow the comment (or the logic below). It seems like if there > is no new issue you should always show the sink list (regardless of the old > issue or not). Can the comment be clarified? Done. https://codereview.chromium.org/1868983003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_sink_list_tests.js (right): https://codereview.chromium.org/1868983003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_sink_list_tests.js:182: test('sink list visibility non blocking issue', function(done) { On 2016/04/07 21:29:50, mark a. foltz wrote: > Are there test cases for issue replacement (non-blocking with blocking, > non-blocking with null, blocking with null)? Issue replacement with null was added in the current patch. Added replacing non-blocking with blocking in a new patch.
https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1223: * @param {?media_router.Issue} oldIssue The previous issue. In the current implementation, this parameter is unused. Remove it? https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1223: * @param {?media_router.Issue} oldIssue The previous issue. In the current implementation, this parameter is unused. Remove it? https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1236: this.updateElementPositioning_(); Should we do this only when modifying this.currentView_?
https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1223: * @param {?media_router.Issue} oldIssue The previous issue. On 2016/04/13 19:37:48, mark a. foltz wrote: > In the current implementation, this parameter is unused. Remove it? Done. https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1236: this.updateElementPositioning_(); On 2016/04/13 19:37:48, mark a. foltz wrote: > Should we do this only when modifying this.currentView_? No, in the case where there's a new or just resolved non-blocking issue, we want to call this.
LGTM with nits https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1236: this.updateElementPositioning_(); On 2016/04/13 at 20:35:35, apacible wrote: > On 2016/04/13 19:37:48, mark a. foltz wrote: > > Should we do this only when modifying this.currentView_? > > No, in the case where there's a new or just resolved non-blocking issue, we want to call this. Consider adding a comment explaining since it wasn't obvious to me :) https://codereview.chromium.org/1868983003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1111: * @param {?media_router.Issue} newIssue The new issue. The new issue, or null if the blocking issue was resolved.
Patchset #7 (id:180001) has been deleted
https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1236: this.updateElementPositioning_(); On 2016/04/14 18:01:00, mark a. foltz wrote: > On 2016/04/13 at 20:35:35, apacible wrote: > > On 2016/04/13 19:37:48, mark a. foltz wrote: > > > Should we do this only when modifying this.currentView_? > > > > No, in the case where there's a new or just resolved non-blocking issue, we > want to call this. > > Consider adding a comment explaining since it wasn't obvious to me :) Done. Also reworked this a bit to avoid duplicate calls. Updating |currentView_| already calls updateElementPositioning_(). https://codereview.chromium.org/1868983003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1868983003/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1111: * @param {?media_router.Issue} newIssue The new issue. On 2016/04/14 18:01:00, mark a. foltz wrote: > The new issue, or null if the blocking issue was resolved. Done.
Description was changed from ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. BUG=601502 ========== to ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. This change also fixes an issue where the issue banner's offsetHeight is always 0. BUG=601502 ==========
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1868983003/#ps160001 (title: "Changes per mfoltz@'s comments. Fix styling bug.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868983003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868983003/160001
Message was sent while issue was closed.
Description was changed from ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. This change also fixes an issue where the issue banner's offsetHeight is always 0. BUG=601502 ========== to ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. This change also fixes an issue where the issue banner's offsetHeight is always 0. BUG=601502 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. This change also fixes an issue where the issue banner's offsetHeight is always 0. BUG=601502 ========== to ========== [Media Router WebUI] Switch back to sink view after blocking issue is resolved. When a blocking issue is resolved, container's |issue| is set to null. Currently, the UI doesn't switch back to the sink list view, leaving the user in a relatively blank issue view with no way of switching back. Note that blocking issues take precedence to non-blocking issues, so there is no case where a blocking issue can be replaced by a non-blocking issue. It always becomes null when it is resolved. This change also fixes an issue where the issue banner's offsetHeight is always 0. BUG=601502 Committed: https://crrev.com/fee1baea9fdb05cb5867f46eb1ffe5c850be9d9c Cr-Commit-Position: refs/heads/master@{#387451} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fee1baea9fdb05cb5867f46eb1ffe5c850be9d9c Cr-Commit-Position: refs/heads/master@{#387451} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
