Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428823002:
DevTools: properly restore selected DOMNode in Elements panel. (Closed)
|
Created:
4 years, 2 months ago by lushnikov Modified:
4 years, 2 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: properly restore selected DOMNode in Elements panel.
Since the http://crrev.com/b4d6bf98e, we started
sending two "documentUpdated" events for every
page reload.
It's not a bad thing by itself (but probably needs
addressing in a follow-up), but the element restoration
logic is unprepared for this.
This patch re-writes the element restoring logic in the
elements panel.
BUG=645645
Committed: https://crrev.com/247328a16045ca03a0684b76cf1f7de988550453
Cr-Commit-Position: refs/heads/master@{#426732}
Patch Set 1 #Patch Set 2 : nit #Patch Set 3 : test #
Total comments: 14
Patch Set 4 : address comments #Patch Set 5 : rebaseline test #Patch Set 6 : make test non-flaky #Patch Set 7 : fix test #
Messages
Total messages: 37 (20 generated)
The CQ bit was checked by lushnikov@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...
lushnikov@chromium.org changed reviewers: + pfeldman@chromium.org
please, take a look
Description was changed from ========== DevTools: properly restore selected element. BUG=645645 ========== to ========== DevTools: properly restore selected element. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing byitself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ==========
lushnikov@chromium.org changed reviewers: + dgozman@chromium.org
Description was changed from ========== DevTools: properly restore selected element. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing byitself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing byitself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ==========
Description was changed from ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing byitself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ==========
https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:393: .then(onNodeRestored.bind(this, this._selectedNodeOnReset)) missing semicolon https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:408: this._preselectNode(defaultSelection); Should recalc default selection. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:418: _preselectNode: function(node) Let's make this one responsible for default selection as well. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:418: _preselectNode: function(node) _setDefaultSelectedNode https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:425: this._isSettingDefaultSelection = true; nit: _isSettingDefaultSelectedNode https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:437: _restoreNode: function(domModel, staleNode) Let's make this inner function of _documentUpdated. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:442: return domModel.pushNodeByPathToFrontendPromise(nodePath); Let's maybe use callback version?
mostly done. please, take another look! https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js (right): https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:393: .then(onNodeRestored.bind(this, this._selectedNodeOnReset)) On 2016/10/19 01:10:38, dgozman wrote: > missing semicolon Done. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:408: this._preselectNode(defaultSelection); On 2016/10/19 01:10:38, dgozman wrote: > Should recalc default selection. Done. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:418: _preselectNode: function(node) The default selection seems to fit better in the onNodeRestored method; I left it there. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:418: _preselectNode: function(node) On 2016/10/19 01:10:38, dgozman wrote: > _setDefaultSelectedNode Done. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:425: this._isSettingDefaultSelection = true; On 2016/10/19 01:10:38, dgozman wrote: > nit: _isSettingDefaultSelectedNode Done. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:437: _restoreNode: function(domModel, staleNode) On 2016/10/19 01:10:38, dgozman wrote: > Let's make this inner function of _documentUpdated. Done. https://codereview.chromium.org/2428823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:442: return domModel.pushNodeByPathToFrontendPromise(nodePath); On 2016/10/19 01:10:38, dgozman wrote: > Let's maybe use callback version? Done.
lgtm
The CQ bit was checked by lushnikov@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lushnikov@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/2428823002/#ps100001 (title: "make test non-flaky")
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_...) 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 lushnikov@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.
Description was changed from ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://chromiumcodereview.appspot.com/2434343002/ by mathp@chromium.org. The reason for reverting is: inspector/elements/elements-panel-restore-selection-when-node-comes-later.html failed on two bots (and perhaps more). One example: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28....
Message was sent while issue was closed.
Description was changed from ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ==========
Thank you for the revert. Fixed the test, doesn't flake any more.
The CQ bit was checked by lushnikov@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/2428823002/#ps120001 (title: "fix test")
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.
Description was changed from ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 Committed: https://crrev.com/9cb88eeb6589fb10e8b7e87c76b54ee16d2f844e Cr-Commit-Position: refs/heads/master@{#426525} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9cb88eeb6589fb10e8b7e87c76b54ee16d2f844e Cr-Commit-Position: refs/heads/master@{#426525}
Message was sent while issue was closed.
Description was changed from ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 Committed: https://crrev.com/9cb88eeb6589fb10e8b7e87c76b54ee16d2f844e Cr-Commit-Position: refs/heads/master@{#426525} ========== to ========== DevTools: properly restore selected DOMNode in Elements panel. Since the http://crrev.com/b4d6bf98e, we started sending two "documentUpdated" events for every page reload. It's not a bad thing by itself (but probably needs addressing in a follow-up), but the element restoration logic is unprepared for this. This patch re-writes the element restoring logic in the elements panel. BUG=645645 Committed: https://crrev.com/247328a16045ca03a0684b76cf1f7de988550453 Cr-Commit-Position: refs/heads/master@{#426732} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/247328a16045ca03a0684b76cf1f7de988550453 Cr-Commit-Position: refs/heads/master@{#426732}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://chromiumcodereview.appspot.com/2446163004/ by lushnikov@chromium.org. The reason for reverting is: Reverting this patch since it doesn't eliminate issue.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
