Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372843002:
Add ash devtools client (Closed)
|
Created:
4 years, 2 months ago by Sarmad Hashmi Modified:
4 years, 1 month ago CC:
chromium-reviews, kalyank, sadrul, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ash devtools client
The wm shell starts the devtools server and attaches a client which
implements the Backend interface provided by the devtools protocol. The
current implementation simply generates a tree with the windows,
widgets and views.
BUG=648701
Committed: https://crrev.com/93903d4af14a7922e2e7c4e1f44026daa8cfc99f
Cr-Commit-Position: refs/heads/master@{#428548}
Patch Set 1 #Patch Set 2 : Add ash devtools client #Patch Set 3 : Add ash devtools client #Patch Set 4 : Add ash devtools client #Patch Set 5 : Add ash devtools client #
Total comments: 24
Patch Set 6 : Address @sadrul's comments #Patch Set 7 : Rename AshDevToolsBackend to AshDevToolsClientDOM #Patch Set 8 : Changed imports to components/ui_devtools #Patch Set 9 : Fix header definition #Patch Set 10 : Add ui_devtools dep to ash #Patch Set 11 : Add OWNERS file #
Total comments: 14
Patch Set 12 : sadrul's feedback and renamed to dom_agent #
Total comments: 4
Patch Set 13 : Use MakeUnique #
Messages
Total messages: 67 (52 generated)
Description was changed from ========== Add ash devtools client The wm shell starts the devtools server and attaches a client which implements the Backend interface provided by the devtools protocol. The current implementation simply generates a tree with the windows, widgets and views. BUG=648701 ========== to ========== Add ash devtools client The wm shell starts the devtools server and attaches a client which implements the Backend interface provided by the devtools protocol. The current implementation simply generates a tree with the windows, widgets and views. BUG=648701 ==========
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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...
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... File ash/common/devtools/devtools_client_backend.cc (right): https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:23: namespace { This anon-namespace block should go before the AshDevToolsBackend ctor/dtor https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:26: DOM::NodeId node_ids = 0; Move this into BuildNode below, as 'static DOM::NodeId node_ids = 0;' https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:28: std::unique_ptr<DOM::Node> buildNode( BuildNode https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:30: std::unique_ptr<Array<String>> attributes, Where is Array defined? https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:35: .setNodeType(1) What does '1' mean here? https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:42: std::unique_ptr<Array<String>> getAttributes(const ash::WmWindow* window) { GetAttributes https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... File ash/common/devtools/devtools_client_backend.h (right): https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:4: Add include guard https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:13: AshDevToolsBackend(ash::WmShell* shell); explicit https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:17: std::unique_ptr<protocol::DOM::Node> buildInitialTree(); BuildInitialTree() https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:21: void disable(protocol::ErrorString* error) override {} When are enable/disable called? https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:26: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2372843002/diff/80001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2372843002/diff/80001/ash/common/wm_shell.cc#... ash/common/wm_shell.cc:110: } Can some of this code be moved into ui/devtools/? For example, the code here could look like: devtools_server_ = ui::UiDevToolsServer::Create(); if (devtools_server_) { ... register backend, client etc. } ui::UiDevToolsServer::Create() would look at the flag to decide whether or not to start the server, and what port to use etc.
Renamed files to devtools_client_backend.* to devtools_client_dom.* since we'll eventually have devtools_client_css and devtools_client_network as well. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... File ash/common/devtools/devtools_client_backend.cc (right): https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:23: namespace { On 2016/10/17 16:18:59, sadrul wrote: > This anon-namespace block should go before the AshDevToolsBackend ctor/dtor Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:26: DOM::NodeId node_ids = 0; On 2016/10/17 16:18:59, sadrul wrote: > Move this into BuildNode below, as 'static DOM::NodeId node_ids = 0;' Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:28: std::unique_ptr<DOM::Node> buildNode( On 2016/10/17 16:18:59, sadrul wrote: > BuildNode Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:30: std::unique_ptr<Array<String>> attributes, On 2016/10/17 16:18:59, sadrul wrote: > Where is Array defined? protocol::Array (generated) https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:35: .setNodeType(1) On 2016/10/17 16:18:59, sadrul wrote: > What does '1' mean here? https://www.w3.org/TR/1998/REC-DOM-Level-1-19981001/level-one-core.html#ID-19... https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.cc:42: std::unique_ptr<Array<String>> getAttributes(const ash::WmWindow* window) { On 2016/10/17 16:18:59, sadrul wrote: > GetAttributes Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... File ash/common/devtools/devtools_client_backend.h (right): https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:4: On 2016/10/17 16:18:59, sadrul wrote: > Add include guard Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:13: AshDevToolsBackend(ash::WmShell* shell); On 2016/10/17 16:18:59, sadrul wrote: > explicit Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:17: std::unique_ptr<protocol::DOM::Node> buildInitialTree(); On 2016/10/17 16:18:59, sadrul wrote: > BuildInitialTree() Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:21: void disable(protocol::ErrorString* error) override {} On 2016/10/17 16:18:59, sadrul wrote: > When are enable/disable called? The client (inspector) sends an enable message initially and it won't request anything else (such as getDocument) until the Backend responds to that enable request. Disable is for, as you can probably guess, disabling the inspector tabs. https://codereview.chromium.org/2372843002/diff/80001/ash/common/devtools/dev... ash/common/devtools/devtools_client_backend.h:26: }; On 2016/10/17 16:18:59, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2372843002/diff/80001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2372843002/diff/80001/ash/common/wm_shell.cc#... ash/common/wm_shell.cc:110: } On 2016/10/17 16:19:00, sadrul wrote: > Can some of this code be moved into ui/devtools/? For example, the code here > could look like: > > devtools_server_ = ui::UiDevToolsServer::Create(); > if (devtools_server_) { > ... register backend, client etc. > } > > ui::UiDevToolsServer::Create() would look at the flag to decide whether or not > to start the server, and what port to use etc. This is a great idea. Done!
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...
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 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...
Please take a look sadrul@!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... File ash/common/devtools/devtools_client_dom.cc (right): https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:21: const String& name, Is this std::string? If so, use std::string in all the places in here. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:28: .setNodeType(1) Let's give this a name so it's easier to read: constexpr int kDomElementNodeType = 1; return DOM::Node::create()....setNodeType(kDomElementNodeType)... https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:46: attributes->addItem(base::IntToString(window->GetBounds().width())); A number of these attributes (e.g. bounds, visibility, opacity etc.) will be done differently, right? Would it make sense to leave those out from here for now? https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:78: std::unique_ptr<DOM::Node> BuildTreeForRootView(views::View* view) { BuildTreeForView, since this is called for all Views in the tree. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:86: std::unique_ptr<DOM::Node> BuildTreeForRootWidget(views::Widget* widget) { BuildTreeForWidget https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:92: std::unique_ptr<DOM::Node> BuildTreeForRootWindow(ash::WmWindow* window) { BuildTreeForWindow https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... File ash/common/devtools/devtools_client_dom.h (right): https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.h:11: namespace ui { Should be namespace ash
Renamed from devtools_client_dom to devtools_dom_agent. I realized that we should keep the naming consistent with what they have for DevTools. They call all their "backends/frontends", agents. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... File ash/common/devtools/devtools_client_dom.cc (right): https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:21: const String& name, On 2016/10/26 19:20:41, sadrul wrote: > Is this std::string? If so, use std::string in all the places in here. Done. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:28: .setNodeType(1) On 2016/10/26 19:20:41, sadrul wrote: > Let's give this a name so it's easier to read: > > constexpr int kDomElementNodeType = 1; > return DOM::Node::create()....setNodeType(kDomElementNodeType)... Done. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:46: attributes->addItem(base::IntToString(window->GetBounds().width())); On 2016/10/26 19:20:41, sadrul wrote: > A number of these attributes (e.g. bounds, visibility, opacity etc.) will be > done differently, right? Would it make sense to leave those out from here for > now? Yep you're right. Done. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:78: std::unique_ptr<DOM::Node> BuildTreeForRootView(views::View* view) { On 2016/10/26 19:20:41, sadrul wrote: > BuildTreeForView, since this is called for all Views in the tree. Done. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:86: std::unique_ptr<DOM::Node> BuildTreeForRootWidget(views::Widget* widget) { On 2016/10/26 19:20:41, sadrul wrote: > BuildTreeForWidget Done. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.cc:92: std::unique_ptr<DOM::Node> BuildTreeForRootWindow(ash::WmWindow* window) { On 2016/10/26 19:20:41, sadrul wrote: > BuildTreeForWindow Done. https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... File ash/common/devtools/devtools_client_dom.h (right): https://codereview.chromium.org/2372843002/diff/200001/ash/common/devtools/de... ash/common/devtools/devtools_client_dom.h:11: namespace ui { On 2016/10/26 19:20:41, sadrul wrote: > Should be namespace ash Done.
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.
sadrul@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman@ too lgtm
mhashmi@chromium.org changed reviewers: + sky@chromium.org
+sky@ for OWNERS review
I don't know about views and widgets, but dispatcher usage looks good.
https://codereview.chromium.org/2372843002/diff/220001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2372843002/diff/220001/ash/BUILD.gn#newcode859 ash/BUILD.gn:859: "//components/ui_devtools", What are all the dependencies of this? I want to make sure we don't end up pulling in content/blink. How many additional build files do this pull in? https://codereview.chromium.org/2372843002/diff/220001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2372843002/diff/220001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:95: std::unique_ptr<ui::devtools::protocol::DOM::Backend> backend( MakeUnique where possible.
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2372843002/diff/220001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2372843002/diff/220001/ash/BUILD.gn#newcode859 ash/BUILD.gn:859: "//components/ui_devtools", On 2016/10/28 21:27:10, sky wrote: > What are all the dependencies of this? I want to make sure we don't end up > pulling in content/blink. How many additional build files do this pull in? Dependencies are base, net, net:http_server, and an action which generates the protocol using files from here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/inspe... https://codereview.chromium.org/2372843002/diff/220001/ash/common/wm_shell.cc File ash/common/wm_shell.cc (right): https://codereview.chromium.org/2372843002/diff/220001/ash/common/wm_shell.cc... ash/common/wm_shell.cc:95: std::unique_ptr<ui::devtools::protocol::DOM::Backend> backend( On 2016/10/28 21:27:10, sky wrote: > MakeUnique where possible. 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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM
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/2372843002/#ps240001 (title: "Use MakeUnique")
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 ========== Add ash devtools client The wm shell starts the devtools server and attaches a client which implements the Backend interface provided by the devtools protocol. The current implementation simply generates a tree with the windows, widgets and views. BUG=648701 ========== to ========== Add ash devtools client The wm shell starts the devtools server and attaches a client which implements the Backend interface provided by the devtools protocol. The current implementation simply generates a tree with the windows, widgets and views. BUG=648701 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add ash devtools client The wm shell starts the devtools server and attaches a client which implements the Backend interface provided by the devtools protocol. The current implementation simply generates a tree with the windows, widgets and views. BUG=648701 ========== to ========== Add ash devtools client The wm shell starts the devtools server and attaches a client which implements the Backend interface provided by the devtools protocol. The current implementation simply generates a tree with the windows, widgets and views. BUG=648701 Committed: https://crrev.com/93903d4af14a7922e2e7c4e1f44026daa8cfc99f Cr-Commit-Position: refs/heads/master@{#428548} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/93903d4af14a7922e2e7c4e1f44026daa8cfc99f Cr-Commit-Position: refs/heads/master@{#428548} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
