Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121763002:
[net-internals] Fix JS exception on stopping capturing while in Capture view (Closed)
|
Created:
4 years, 5 months ago by Rafał Chłodnicki Modified:
4 years, 5 months ago CC:
chromium-reviews, eroman, arv+watch_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[net-internals] Fix JS exception on stopping capturing while in Capture view
Store a map of link enabled states so that we can use it to enable next
visibile view on hidding active view. Map is ordered so it works to iterate it.
BUG=616382
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dc50caf6b42799a0f3a532872750a09ac4dacc6e
Cr-Commit-Position: refs/heads/master@{#405942}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 17 (7 generated)
Description was changed from ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 ========== to ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
rchlodnicki@opera.com changed reviewers: + mmenke@chromium.org
eroman@chromium.org changed reviewers: + eroman@chromium.org
https://codereview.chromium.org/2121763002/diff/1/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tab_switcher_view.js (left): https://codereview.chromium.org/2121763002/diff/1/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tab_switcher_view.js:122: for (var view in this.viewList_) { I agree that the current code is quite wrong. The problem with your proposed map from tabId --> visibility is the visibility state is never updated in the map (whereas before it queried the underlying View). At a minimum this is a layering issue. Instead I think we can accomplish this using the existing tabIdToView map: for (var curTabId in this.tabIdToView_) { var curView = thistabIdToView_[curTabId]; if (curView.isVisible()) this.switchToTab(curTabId); break; }
On 2016/07/06 20:06:30, eroman wrote: > https://codereview.chromium.org/2121763002/diff/1/chrome/browser/resources/ne... > File chrome/browser/resources/net_internals/tab_switcher_view.js (left): > > https://codereview.chromium.org/2121763002/diff/1/chrome/browser/resources/ne... > chrome/browser/resources/net_internals/tab_switcher_view.js:122: for (var view > in this.viewList_) { > I agree that the current code is quite wrong. > > The problem with your proposed map from tabId --> visibility is the visibility > state is never updated in the map (whereas before it queried the underlying > View). At a minimum this is a layering issue. > > Instead I think we can accomplish this using the existing tabIdToView map: > > for (var curTabId in this.tabIdToView_) { > var curView = thistabIdToView_[curTabId]; > if (curView.isVisible()) > this.switchToTab(curTabId); > break; > } This is not about whether view is currently visible. View visible means that the view is currently the active one (selected). This is about whether view is even shown within the sidebar as some views can get hidden completely from the sidebar. This loops wants to find the first view that is shown on the sidebar. Some renaming of methods could make this clearer but I don't think that is directly related to this fix and could be done later.
Any comments?
LGTM Thanks for fixing this. I am still not terribly excited about this construction, but an improvement nevertheless.
The CQ bit was checked by rchlodnicki@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Description was changed from ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [net-internals] Fix JS exception on stopping capturing while in Capture view Store a map of link enabled states so that we can use it to enable next visibile view on hidding active view. Map is ordered so it works to iterate it. BUG=616382 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dc50caf6b42799a0f3a532872750a09ac4dacc6e Cr-Commit-Position: refs/heads/master@{#405942} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/dc50caf6b42799a0f3a532872750a09ac4dacc6e Cr-Commit-Position: refs/heads/master@{#405942} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
