Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873843003:
[DevTools] Restore tree selection after reload (Closed)
|
Created:
3 years, 7 months ago by eostroukhov Modified:
3 years, 7 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Restore tree selection after reload
Application pane will now restore selection if the element is readded
within 1s after page reload and if the user does not change tree
selection by hand.
BUG=683704
Review-Url: https://codereview.chromium.org/2873843003
Cr-Commit-Position: refs/heads/master@{#471851}
Committed: https://chromium.googlesource.com/chromium/src/+/622b9016fb49d73c33ffd3a6c9adda9b2055c09c
Patch Set 1 #
Total comments: 10
Patch Set 2 : [DevTools] Restore tree selection after reload #
Total comments: 4
Patch Set 3 : [DevTools] Restore tree selection after reload #
Messages
Total messages: 32 (23 generated)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js (right): https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:111: this._sidebarTree.addEventListener(UI.TreeOutline.Events.ElementAttached, event => this._elementAdded(event)); this._elementAdded.bind(this) https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:253: // This is to prevent selection jumping on slow page loads. Why not on first user interaction? https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:267: if (index !== 0) if (index) https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:639: this._storagePanel.setLastSelectedItemURL(itemURL); I still think we should combine with this one. https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:63: [networkManager.addEventListener(SDK.NetworkManager.Events.ResponseReceived, () => this._onResponseRecieved())]; Ain't this an unrelated change?
Thank you for the comments. Please take another look. https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js (right): https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:111: this._sidebarTree.addEventListener(UI.TreeOutline.Events.ElementAttached, event => this._elementAdded(event)); On 2017/05/11 23:08:41, dgozman wrote: > this._elementAdded.bind(this) Done. Waiting for "::" operator :) https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:253: // This is to prevent selection jumping on slow page loads. On 2017/05/11 23:08:41, dgozman wrote: > Why not on first user interaction? Done. https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:267: if (index !== 0) On 2017/05/11 23:08:41, dgozman wrote: > if (index) Done. (Node.js has opposite convention ;) ) https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:639: this._storagePanel.setLastSelectedItemURL(itemURL); On 2017/05/11 23:08:41, dgozman wrote: > I still think we should combine with this one. Did that. It now actually better restores selection on reload. https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2873843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:63: [networkManager.addEventListener(SDK.NetworkManager.Events.ResponseReceived, () => this._onResponseRecieved())]; On 2017/05/11 23:08:41, dgozman wrote: > Ain't this an unrelated change? It just becomes immediately obvious if we are restoring the selection. If this fix is not there, then restored selection shows empty cookies view.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2873843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2873843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:63: [networkManager.addEventListener(SDK.NetworkManager.Events.ResponseReceived, () => this._onResponseRecieved())]; addEventListener(..., this._onResponseReceived, this) https://codereview.chromium.org/2873843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:138: _onResponseRecieved() { typo in Received
Thanks! https://codereview.chromium.org/2873843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2873843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:63: [networkManager.addEventListener(SDK.NetworkManager.Events.ResponseReceived, () => this._onResponseRecieved())]; On 2017/05/12 23:59:17, dgozman wrote: > addEventListener(..., this._onResponseReceived, this) Done. https://codereview.chromium.org/2873843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:138: _onResponseRecieved() { On 2017/05/12 23:59:17, dgozman wrote: > typo in Received Done.
The CQ bit was checked by eostroukhov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2873843003/#ps40001 (title: "[DevTools] Restore tree selection after reload")
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eostroukhov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494864143910750,
"parent_rev": "44cdf9e94c51e582a375fa8dfccbf4518898d63a", "commit_rev":
"622b9016fb49d73c33ffd3a6c9adda9b2055c09c"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Restore tree selection after reload Application pane will now restore selection if the element is readded within 1s after page reload and if the user does not change tree selection by hand. BUG=683704 ========== to ========== [DevTools] Restore tree selection after reload Application pane will now restore selection if the element is readded within 1s after page reload and if the user does not change tree selection by hand. BUG=683704 Review-Url: https://codereview.chromium.org/2873843003 Cr-Commit-Position: refs/heads/master@{#471851} Committed: https://chromium.googlesource.com/chromium/src/+/622b9016fb49d73c33ffd3a6c9ad... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/622b9016fb49d73c33ffd3a6c9ad... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
