Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801723003:
[DevTools] Do not reset storage tree items (Closed)
|
Created:
3 years, 8 months ago by eostroukhov Modified:
3 years, 8 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] Do not reset storage tree items
These tree items are updated in response to storage-specific events so
they should not be removed when navigation happens.
BUG=701413
Review-Url: https://codereview.chromium.org/2801723003
Cr-Commit-Position: refs/heads/master@{#463835}
Committed: https://chromium.googlesource.com/chromium/src/+/46f77da2781539ca0cefc84e7bf86b138bea7bc0
Patch Set 1 #
Total comments: 4
Patch Set 2 : [DevTools] Do not reset storage tree items #
Total comments: 2
Patch Set 3 : [DevTools] Do not reset storage tree items #
Total comments: 18
Patch Set 4 : [DevTools] Do not reset storage tree items #Patch Set 5 : [DevTools] Do not reset storage tree items #
Total comments: 1
Messages
Total messages: 33 (22 generated)
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...
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
Please consider
Description was changed from ========== [DevTools] Do not reset storage tree items These tree items are updated in response to storage-specific events so they should not be removed when navigation happens. BUG=707706,705237,701413 ========== to ========== [DevTools] Do not reset storage tree items These tree items are updated in response to storage-specific events so they should not be removed when navigation happens. BUG=701413 ==========
Let's add a test! https://codereview.chromium.org/2801723003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js (right): https://codereview.chromium.org/2801723003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:220: this._resetCookies(); No more _resetWebSQL? https://codereview.chromium.org/2801723003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:251: this._resetCookies(); No more _resetWebSQL?
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Thanks for the review. Code updated, please take another look. https://codereview.chromium.org/2801723003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js (right): https://codereview.chromium.org/2801723003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:220: this._resetCookies(); On 2017/04/06 00:12:23, dgozman wrote: > No more _resetWebSQL? Done. Thanks for catching this! https://codereview.chromium.org/2801723003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:251: this._resetCookies(); On 2017/04/06 00:12:23, dgozman wrote: > No more _resetWebSQL? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Let's add a test! https://codereview.chromium.org/2801723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js (left): https://codereview.chromium.org/2801723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:265: this._panel.resetView(); What happened to this? Should we reset view if it was a cookie view?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2801723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js (left): https://codereview.chromium.org/2801723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:265: this._panel.resetView(); On 2017/04/06 00:41:04, dgozman wrote: > What happened to this? Should we reset view if it was a cookie view? Parent tree element gets selected when a child is removed - this updates the view.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
I added a test case. Please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html (right): https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:7: <script> // window.debugTest=true; remove debugTest https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:25: function waitABit() { Why this? https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:40: function createWebSQLDatabase() { Let's move a helper in resources-test.js. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:44: InspectorTest.addConsoleSniffer(msg => { Instead of sniffing for console, take a look into InspectorTest.evaluateInPageAsync. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:52: var rtm = InspectorTest.mainTarget.model(SDK.ResourceTreeModel); InspectorTest.resourceTreeModel https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:53: rtm.dispatchEventToListeners(SDK.ResourceTreeModel.Events.FrameNavigated, rtm.mainFrame); Faking an event is usually a bad practice. Instead, do a real frame navigation in page. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:63: async function doTests() { You can inline this function into test() directly. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:74: } catch (e) { inspector-test does this for you. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:87: <p>Tests Application Panel behavious on main frame navigation.</p> typo: behaviour
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 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...
Thank for reviewing the CL. I addressed the comments, please take another look. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html (right): https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:7: <script> // window.debugTest=true; On 2017/04/11 17:27:29, dgozman wrote: > remove debugTest Done. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:25: function waitABit() { On 2017/04/11 17:27:28, dgozman wrote: > Why this? Removed https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:40: function createWebSQLDatabase() { On 2017/04/11 17:27:29, dgozman wrote: > Let's move a helper in resources-test.js. Done. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:44: InspectorTest.addConsoleSniffer(msg => { On 2017/04/11 17:27:28, dgozman wrote: > Instead of sniffing for console, take a look into > InspectorTest.evaluateInPageAsync. Done. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:52: var rtm = InspectorTest.mainTarget.model(SDK.ResourceTreeModel); On 2017/04/11 17:27:28, dgozman wrote: > InspectorTest.resourceTreeModel Done. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:53: rtm.dispatchEventToListeners(SDK.ResourceTreeModel.Events.FrameNavigated, rtm.mainFrame); On 2017/04/11 17:27:28, dgozman wrote: > Faking an event is usually a bad practice. Instead, do a real frame navigation > in page. Done. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:63: async function doTests() { On 2017/04/11 17:27:29, dgozman wrote: > You can inline this function into test() directly. Done. https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:74: } catch (e) { On 2017/04/11 17:27:28, dgozman wrote: > inspector-test does this for you. Done. (on a separate note - I don't see inspector-test surfacing those exceptions) https://codereview.chromium.org/2801723003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation.html:87: <p>Tests Application Panel behavious on main frame navigation.</p> On 2017/04/11 17:27:29, dgozman wrote: > typo: behaviour Done. https://codereview.chromium.org/2801723003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation-expected.txt (right): https://codereview.chromium.org/2801723003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/resources/resources-panel-on-navigation-expected.txt:52: top Note that there's a bug I am working on to fix that results in duplicate resources after the page refresh. For the time being I am uploading the test that expects the broken output.
Beautiful, thanks! lgtm
The CQ bit was unchecked by eostroukhov@chromium.org
The CQ bit was checked by eostroukhov@chromium.org
The CQ bit was unchecked by eostroukhov@chromium.org
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": 80001, "attempt_start_ts": 1491948399136800,
"parent_rev": "1e04e1de4401bf105333158e4cb314fa834c5fa8", "commit_rev":
"46f77da2781539ca0cefc84e7bf86b138bea7bc0"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Do not reset storage tree items These tree items are updated in response to storage-specific events so they should not be removed when navigation happens. BUG=701413 ========== to ========== [DevTools] Do not reset storage tree items These tree items are updated in response to storage-specific events so they should not be removed when navigation happens. BUG=701413 Review-Url: https://codereview.chromium.org/2801723003 Cr-Commit-Position: refs/heads/master@{#463835} Committed: https://chromium.googlesource.com/chromium/src/+/46f77da2781539ca0cefc84e7bf8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/46f77da2781539ca0cefc84e7bf8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
