Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466073003:
Push updates to inspector when windows are added, destroyed or reorganized (Closed)
|
Created:
4 years, 1 month ago by Sarmad Hashmi Modified:
4 years, 1 month ago Reviewers:
sadrul CC:
chromium-reviews, kalyank, sadrul, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPush updates to inspector when windows are added, destroyed or reorganized
This CL pushes updates to the connected DevTools inspector whenever windows
are updated in any way. We add the DOM agent as an observer to the root
window and simply listen for any window tree changes. Depending on whether
there is a new parent or an old parent for the target window, we push
updates for removing and adding the window DOM node.
This adds two events to the protocol: childNodeInserted and childNodeRemoved.
BUG=648701
Committed: https://crrev.com/584115bcc2a6724daf09482a199ddb179f1a9cc2
Cr-Commit-Position: refs/heads/master@{#430102}
Patch Set 1 #Patch Set 2 : Push updates to inspector when windows are added, destroyed or reorganized #Patch Set 3 : Push updates to inspector when windows are added, destroyed or reorganized #Patch Set 4 : Push updates to inspector when windows are added, destroyed or reorganized #
Total comments: 14
Patch Set 5 : sadruls comments, implement different methods of observer #Patch Set 6 : use new devtools style #
Total comments: 4
Patch Set 7 : Document previous sibling id #
Depends on Patchset: Messages
Total messages: 34 (26 generated)
The CQ bit was checked by mhashmi@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 mhashmi@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...
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
Will add tests in follow up CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mhashmi@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.
https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:95: void AshDevToolsDOMAgent::OnWindowTreeChanging(WmWindow* window, I don't think this gets called if sibling windows are reordered. For example, if you have two browser windows open, and you click on one to activate it, then click on the other to activate, that changes the order of the two windows. Is that reflected in devtools? If not, it may be necessary to also override OnWindowStackingChanged(). https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:132: frontend()->childNodeInserted(window_to_node_id_map_[parent], 0, Should 'prev_sibling' be correctly computed? (e.g. you would look at parent->children() and figure out what the prev-sibling window is?) Maybe this needs to happen in OnWindowTreeChanged() callback (instead of OnWindowTreeChanging())? https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:142: window->GetParent() ? window_to_node_id_map_[window->GetParent()] : 0; window->GetParent() should never be null here, right? It is getting called from OnWindowTreeChanging(), only when params.old_parent is non-null. That would imply window->GetParent() should also be non-null? Can this be a DCHECK()? https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:152: window->AddObserver(this); No {} https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:157: for (ash::WmWindow* window : shell_->GetAllRootWindows()) { ditto https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.h:47: typedef std::unordered_map<WmWindow*, int> WindowToNodeIdMap; Use 'using WindowToNodeIdMap = ...'; https://codereview.chromium.org/2466073003/diff/60001/components/ui_devtools/... File components/ui_devtools/protocol.json (right): https://codereview.chromium.org/2466073003/diff/60001/components/ui_devtools/... components/ui_devtools/protocol.json:38: }, Are there some special values for this? (e.g. 0 means at the beginning/end?)
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Kind of a big change since the last patch because sadrul@ brought OnWindowStackingChanged to my attention. We can no longer just add observers to the root windows because OnWindowStackingChanged is called on individual windows as opposed to OnWindowTreeChange* callbacks, which are called on every window that is an ancestor of the target window. Every window now has an observer. OnWindowTreeChanged handles reorganizing and adding of nodes. OnWindowDestroying handles destruction of nodes (as the parent pointer becomes null in OnWindowDestroyed). Finally, OnWindowStackingChanged simply destroys and adds the node again so that it is in the correct order. This leads to the previous sibling change. Now, the nodes will be added in the order that they are in for the parent window (i.e: the same order as parent->GetChildren()). This change works both with and without mustash. https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:95: void AshDevToolsDOMAgent::OnWindowTreeChanging(WmWindow* window, On 2016/11/02 02:04:36, sadrul wrote: > I don't think this gets called if sibling windows are reordered. For example, if > you have two browser windows open, and you click on one to activate it, then > click on the other to activate, that changes the order of the two windows. Is > that reflected in devtools? If not, it may be necessary to also override > OnWindowStackingChanged(). Done. https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:132: frontend()->childNodeInserted(window_to_node_id_map_[parent], 0, On 2016/11/02 02:04:36, sadrul wrote: > Should 'prev_sibling' be correctly computed? (e.g. you would look at > parent->children() and figure out what the prev-sibling window is?) Maybe this > needs to happen in OnWindowTreeChanged() callback (instead of > OnWindowTreeChanging())? Done. https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:142: window->GetParent() ? window_to_node_id_map_[window->GetParent()] : 0; On 2016/11/02 02:04:36, sadrul wrote: > window->GetParent() should never be null here, right? It is getting called from > OnWindowTreeChanging(), only when params.old_parent is non-null. That would > imply window->GetParent() should also be non-null? Can this be a DCHECK()? Discussed in-person. It can be null for root windows. https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:152: window->AddObserver(this); On 2016/11/02 02:04:36, sadrul wrote: > No {} Done. https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:157: for (ash::WmWindow* window : shell_->GetAllRootWindows()) { On 2016/11/02 02:04:36, sadrul wrote: > ditto Done. https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2466073003/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.h:47: typedef std::unordered_map<WmWindow*, int> WindowToNodeIdMap; On 2016/11/02 02:04:36, sadrul wrote: > Use 'using WindowToNodeIdMap = ...'; Done. https://codereview.chromium.org/2466073003/diff/60001/components/ui_devtools/... File components/ui_devtools/protocol.json (right): https://codereview.chromium.org/2466073003/diff/60001/components/ui_devtools/... components/ui_devtools/protocol.json:38: }, On 2016/11/02 02:04:36, sadrul wrote: > Are there some special values for this? (e.g. 0 means at the beginning/end?) From what I've noticed, 0 means the beginning.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 mhashmi@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_TIMED_OUT, no build URL)
lgtm https://codereview.chromium.org/2466073003/diff/100001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2466073003/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_unittest.cc:150: dom_agent()->disable(); When |widget| is destroyed, it should clean up the state from the appropriate callbacks, right? Why is it necessary to explicitly disable() here? https://codereview.chromium.org/2466073003/diff/100001/components/ui_devtools... File components/ui_devtools/protocol.json (right): https://codereview.chromium.org/2466073003/diff/100001/components/ui_devtools... components/ui_devtools/protocol.json:35: "description": "Id of the previous sibling.", Document in the description that 0 means at the beginning. e.g. "Id if the previous sibling (0 if this is at the beginning of the list)"
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2466073003/diff/100001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2466073003/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_unittest.cc:150: dom_agent()->disable(); On 2016/11/04 15:39:17, sadrul wrote: > When |widget| is destroyed, it should clean up the state from the appropriate > callbacks, right? Why is it necessary to explicitly disable() here? The tear down of the |windows| on windows trybots causes the test to crash in the generated files. Specifically, the backtrace crashes inside the Frontend methods, which are all generated. The crash is very similar to the one we saw before for the tests. I'll investigate more a bit later, but the tests work if we disable the agents since the Frontend methods are no longer called on tear down. https://codereview.chromium.org/2466073003/diff/100001/components/ui_devtools... File components/ui_devtools/protocol.json (right): https://codereview.chromium.org/2466073003/diff/100001/components/ui_devtools... components/ui_devtools/protocol.json:35: "description": "Id of the previous sibling.", On 2016/11/04 15:39:17, sadrul wrote: > Document in the description that 0 means at the beginning. e.g. > > "Id if the previous sibling (0 if this is at the beginning of the list)" Done.
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.
The CQ bit was checked by mhashmi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2466073003/#ps120001 (title: "Document previous sibling id")
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Push updates to inspector when windows are added, destroyed or reorganized This CL pushes updates to the connected DevTools inspector whenever windows are updated in any way. We add the DOM agent as an observer to the root window and simply listen for any window tree changes. Depending on whether there is a new parent or an old parent for the target window, we push updates for removing and adding the window DOM node. This adds two events to the protocol: childNodeInserted and childNodeRemoved. BUG=648701 ========== to ========== Push updates to inspector when windows are added, destroyed or reorganized This CL pushes updates to the connected DevTools inspector whenever windows are updated in any way. We add the DOM agent as an observer to the root window and simply listen for any window tree changes. Depending on whether there is a new parent or an old parent for the target window, we push updates for removing and adding the window DOM node. This adds two events to the protocol: childNodeInserted and childNodeRemoved. BUG=648701 Committed: https://crrev.com/584115bcc2a6724daf09482a199ddb179f1a9cc2 Cr-Commit-Position: refs/heads/master@{#430102} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/584115bcc2a6724daf09482a199ddb179f1a9cc2 Cr-Commit-Position: refs/heads/master@{#430102} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
