Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2374513002:
Add ui devtools server (Closed)
|
Created:
4 years, 2 months ago by Sarmad Hashmi Modified:
4 years, 1 month 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, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ui devtools server
Implement a basic devtools server for mus/ash in order to inspect the window
server tree hierarchy from a chrome devtools client. This can be enabled
by starting chrome with the flag --enable-ui-devtools=<port>.
With this implementation, a basic server that only communicates with web
sockets is started. It responds with the hierarchy when the devtools
client connects.
BUG=648701
CQ-DEPEND=CL:2365283003
Committed: https://crrev.com/1ec338a7f29b26c0e6ae4c5c276d6014e00b77c9
Cr-Commit-Position: refs/heads/master@{#427506}
Patch Set 1 #Patch Set 2 : Initial commit for ui devtools server #Patch Set 3 : Initial commit for ui devtools server #Patch Set 4 : Initial commit for ui devtools server #Patch Set 5 : Initial commit for ui devtools server #
Total comments: 4
Patch Set 6 : Initial commit for ui devtools server #Patch Set 7 : Initial commit for ui devtools server #
Total comments: 39
Patch Set 8 : Address @sadrul's comments #
Total comments: 8
Patch Set 9 : Fix all header if directives #
Total comments: 30
Patch Set 10 : Add comments #
Total comments: 4
Patch Set 11 : Move everything to components/ui_devtools #
Total comments: 12
Patch Set 12 : Add OWNERS file and fix headers #Patch Set 13 : Add component as dependency #Patch Set 14 : Add component as dependency #Patch Set 15 : Add net to DEPS and Build #Patch Set 16 : Update BUILD file #Patch Set 17 : Add ui devtools placeholder test action in build file so it builds on win/linux trybots #Patch Set 18 : Add ui devtools placeholder test action in build file so it builds on win/linux trybots #Patch Set 19 : Revert to old build files, try with more specific target #Patch Set 20 : Add dep under is_win conditional #Patch Set 21 : Add to ash BUILD file #Patch Set 22 : Fix int to size_t warning #Patch Set 23 : Ignore int to bool conversion warning #Patch Set 24 : Remove ui_devtools dependency from Ash (will add back in next CL) #Patch Set 25 : Add inspector_protocol to DEPS #Messages
Total messages: 99 (69 generated)
Description was changed from ========== Add devtools server for mus Implement a basic devtools server for mus in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-mus-remote-debugging=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 ========== to ========== Add devtools server for mus Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 ==========
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
Description was changed from ========== Add devtools server for mus Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 ========== to ========== Add devtools server for mus Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 ==========
Description was changed from ========== Add devtools server for mus Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 ========== to ========== Add ui devtools server Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 ==========
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_...)
https://codereview.chromium.org/2374513002/diff/80001/ui/DEPS File ui/DEPS (right): https://codereview.chromium.org/2374513002/diff/80001/ui/DEPS#newcode3 ui/DEPS:3: "+net", This should go in ui/devtools/DEPS https://codereview.chromium.org/2374513002/diff/80001/ui/devtools/protocol.json File ui/devtools/protocol.json (right): https://codereview.chromium.org/2374513002/diff/80001/ui/devtools/protocol.js... ui/devtools/protocol.json:25: "domain": "DOM", We can probably call this UI? Or does the front-end require DOM?
https://codereview.chromium.org/2374513002/diff/80001/ui/DEPS File ui/DEPS (right): https://codereview.chromium.org/2374513002/diff/80001/ui/DEPS#newcode3 ui/DEPS:3: "+net", On 2016/09/27 18:14:50, sadrul wrote: > This should go in ui/devtools/DEPS Done. https://codereview.chromium.org/2374513002/diff/80001/ui/devtools/protocol.json File ui/devtools/protocol.json (right): https://codereview.chromium.org/2374513002/diff/80001/ui/devtools/protocol.js... ui/devtools/protocol.json:25: "domain": "DOM", On 2016/09/27 18:14:50, sadrul wrote: > We can probably call this UI? Or does the front-end require DOM? Front-end calls it DOM. I mentioned yesterday that there was a code generator for the front-end as well and since we aren't using that we'll have to use the names of the chrome built in front-end.
https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/BUILD.gn File ui/devtools/BUILD.gn (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/BUILD.gn#n... ui/devtools/BUILD.gn:58: "StringUtil.h", Do these files have to be called ProtocolPlatform StringUtil? Or can we follow the chromium naming rules (i.e. protocol_platform, string_util) for these? https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/ProtocolPl... File ui/devtools/ProtocolPlatform.h (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/ProtocolPl... ui/devtools/ProtocolPlatform.h:6: #define UI_PROTOCOLPLATFORM_H_ UI_DEVTOOLS_PROTOCOL_PLATFORM_H_ https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/ProtocolPl... ui/devtools/ProtocolPlatform.h:20: #endif // UI_PROTOCOLPLATFORM_H_ newline before https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/StringUtil.cc File ui/devtools/StringUtil.cc (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/StringUtil... ui/devtools/StringUtil.cc:14: return parseJSON(reinterpret_cast<const uint8_t*>(&string[0]), Hm, we probably need to check that this is safe. How about a DCHECK(base::IsStringUTF8(string)) before doing this? And a TODO to work correctly for 16-bit strings too. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:5: #ifndef UI_DEVTOOLS_CLIENT_H UI_DEVTOOLS_DEVTOOLS_CLIENT_H_ https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:19: class UiDevToolsClient : public protocol::FrontendChannel { Document. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:21: UiDevToolsClient(String name, UiDevToolsServer* server); const String& name (to avoid unnecessary copy) https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:39: int connection_id; connection_id_ https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:44: }; DISALLOOW_COPY_AND_ASSIGN https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... File ui/devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:33: if (thread_ && thread_->IsRunning()) { No {} https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:44: } // namespace // namespace should move up above (in line 25) https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:51: return; No {} The '1' in the param-list is a little bit confusing. It'd be more readable if you do something like this: constexpr int kBacklog = 1; if (socket->ListenWithAddressAndPort(... kBacklog) != OK) { ... } https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:53: server_.reset(new net::HttpServer(std::move(socket), this)); server_ = base::MakeUnique<net::HttpServer>(std::move(socket), this); https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:123: if (it == connections_.end()) Should this be a DCHECK() that it != connections_.end()? https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:143: if (it == connections_.end()) Should this be a DCHECK that it != connections_.end()? https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... File ui/devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:30: friend class UiDevToolsClient; Why does UiDevToolsClient need to be friend? Can you just make 'SendOverWebSocket()' a public method instead? https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:47: std::unique_ptr<base::Thread> thread_; If the user already has an IO thread, then it's probably not necessary to create one here. Leave a TODO to allow injecting a task-runner for the IO thread instead of always creating+owning the IO thread from here. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/inspector_... File ui/devtools/inspector_protocol_config.json (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/inspector_... ui/devtools/inspector_protocol_config.json:1: { Does the scripts that use this file understand comments? If it does, can you add some links (if there's any) to docs/guides that explains this protocol here in a comment? https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/protocol.json File ui/devtools/protocol.json (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/protocol.j... ui/devtools/protocol.json:1: { Same comment about link to doc
https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/BUILD.gn File ui/devtools/BUILD.gn (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/BUILD.gn#n... ui/devtools/BUILD.gn:58: "StringUtil.h", On 2016/10/17 16:05:08, sadrul wrote: > Do these files have to be called ProtocolPlatform StringUtil? Or can we follow > the chromium naming rules (i.e. protocol_platform, string_util) for these? Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/ProtocolPl... File ui/devtools/ProtocolPlatform.h (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/ProtocolPl... ui/devtools/ProtocolPlatform.h:6: #define UI_PROTOCOLPLATFORM_H_ On 2016/10/17 16:05:08, sadrul wrote: > UI_DEVTOOLS_PROTOCOL_PLATFORM_H_ Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/ProtocolPl... ui/devtools/ProtocolPlatform.h:20: #endif // UI_PROTOCOLPLATFORM_H_ On 2016/10/17 16:05:08, sadrul wrote: > newline before Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/StringUtil.cc File ui/devtools/StringUtil.cc (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/StringUtil... ui/devtools/StringUtil.cc:14: return parseJSON(reinterpret_cast<const uint8_t*>(&string[0]), On 2016/10/17 16:05:08, sadrul wrote: > Hm, we probably need to check that this is safe. How about a > DCHECK(base::IsStringUTF8(string)) before doing this? And a TODO to work > correctly for 16-bit strings too. Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:5: #ifndef UI_DEVTOOLS_CLIENT_H On 2016/10/17 16:05:08, sadrul wrote: > UI_DEVTOOLS_DEVTOOLS_CLIENT_H_ Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:19: class UiDevToolsClient : public protocol::FrontendChannel { On 2016/10/17 16:05:08, sadrul wrote: > Document. Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:21: UiDevToolsClient(String name, UiDevToolsServer* server); On 2016/10/17 16:05:08, sadrul wrote: > const String& name (to avoid unnecessary copy) Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:39: int connection_id; On 2016/10/17 16:05:08, sadrul wrote: > connection_id_ Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:44: }; On 2016/10/17 16:05:08, sadrul wrote: > DISALLOOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... File ui/devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:33: if (thread_ && thread_->IsRunning()) { On 2016/10/17 16:05:08, sadrul wrote: > No {} Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:44: } // namespace On 2016/10/17 16:05:08, sadrul wrote: > // namespace should move up above (in line 25) Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:51: return; On 2016/10/17 16:05:08, sadrul wrote: > No {} > > The '1' in the param-list is a little bit confusing. It'd be more readable if > you do something like this: > > constexpr int kBacklog = 1; > if (socket->ListenWithAddressAndPort(... kBacklog) != OK) { ... } Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:53: server_.reset(new net::HttpServer(std::move(socket), this)); On 2016/10/17 16:05:08, sadrul wrote: > server_ = base::MakeUnique<net::HttpServer>(std::move(socket), this); Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:123: if (it == connections_.end()) On 2016/10/17 16:05:08, sadrul wrote: > Should this be a DCHECK() that it != connections_.end()? Done. Makes sense since a connection_id that never "connected" to the server shouldn't be sending messages. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:143: if (it == connections_.end()) On 2016/10/17 16:05:08, sadrul wrote: > Should this be a DCHECK that it != connections_.end()? Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... File ui/devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:30: friend class UiDevToolsClient; On 2016/10/17 16:05:08, sadrul wrote: > Why does UiDevToolsClient need to be friend? Can you just make > 'SendOverWebSocket()' a public method instead? Done. Wasn't sure whether it was necessary to make SendOverWebSocket since nothing else would not (and should not) be using SendOverWebSocket but the UiDevToolsClient. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:47: std::unique_ptr<base::Thread> thread_; On 2016/10/17 16:05:08, sadrul wrote: > If the user already has an IO thread, then it's probably not necessary to create > one here. Leave a TODO to allow injecting a task-runner for the IO thread > instead of always creating+owning the IO thread from here. Done. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/inspector_... File ui/devtools/inspector_protocol_config.json (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/inspector_... ui/devtools/inspector_protocol_config.json:1: { On 2016/10/17 16:05:09, sadrul wrote: > Does the scripts that use this file understand comments? If it does, can you add > some links (if there's any) to docs/guides that explains this protocol here in a > comment? There's no specific doc regarding these files AFAIK. There are bits and pieces in various places. Also, can't put comments in json files. https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/protocol.json File ui/devtools/protocol.json (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/protocol.j... ui/devtools/protocol.json:1: { On 2016/10/17 16:05:09, sadrul wrote: > Same comment about link to doc Copied from other comment: There's no specific doc regarding these files AFAIK. There are bits and pieces in various places. Also, can't put comments in json files.
https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/protocol.json File ui/devtools/protocol.json (right): https://codereview.chromium.org/2374513002/diff/120001/ui/devtools/protocol.j... ui/devtools/protocol.json:1: { On 2016/10/17 16:58:21, mhashmi wrote: > On 2016/10/17 16:05:09, sadrul wrote: > > Same comment about link to doc > > Copied from other comment: There's no specific doc regarding these files AFAIK. > There are bits and pieces in various places. Also, can't put comments in json > files. OK. In that case, would it be possible to add a README.md file in here to document some of these? (not necessarily in this CL though) Perhaps we can get some links/help from the devtools folks? https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:5: #ifndef UI_DEVTOOLS_DEVTOOLS_CLIENT_H There should be a _ at the end. https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_s... File ui/devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:6: #define UI_DEVTOOLS_SERVER_H_ UI_DEVTOOLS_DEVTOOLS_SERVER_H_ https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/protocol_p... File ui/devtools/protocol_platform.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/protocol_p... ui/devtools/protocol_platform.h:6: #define UI_PROTOCOL_PLATFORM_H_ UI_DEVTOOLS_PROTOCOL_PLATFORM_H_ https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/string_util.h File ui/devtools/string_util.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/string_uti... ui/devtools/string_util.h:6: #define WS_STRINGUTIL_H_ UI_DEVTOOLS_STRING_UTIL_H_
https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:5: #ifndef UI_DEVTOOLS_DEVTOOLS_CLIENT_H On 2016/10/17 17:44:36, sadrul wrote: > There should be a _ at the end. Done. https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_s... File ui/devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:6: #define UI_DEVTOOLS_SERVER_H_ On 2016/10/17 17:44:36, sadrul wrote: > UI_DEVTOOLS_DEVTOOLS_SERVER_H_ Done. https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/protocol_p... File ui/devtools/protocol_platform.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/protocol_p... ui/devtools/protocol_platform.h:6: #define UI_PROTOCOL_PLATFORM_H_ On 2016/10/17 17:44:36, sadrul wrote: > UI_DEVTOOLS_PROTOCOL_PLATFORM_H_ Done. https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/string_util.h File ui/devtools/string_util.h (right): https://codereview.chromium.org/2374513002/diff/140001/ui/devtools/string_uti... ui/devtools/string_util.h:6: #define WS_STRINGUTIL_H_ On 2016/10/17 17:44:36, sadrul wrote: > UI_DEVTOOLS_STRING_UTIL_H_ Done.
This looks great! A couple of questions: - Do you think we could place the client and the server classes into more accessible location for other services to be able to use it? - retro-fitting the DOM domain backend is probably fine as you test the waters, but I wonder if you should go ahead and introduce your own domain here + your own panel in Devtools longer term.
pfeldman@chromium.org changed reviewers: + beng@chromium.org, pfeldman@chromium.org
pfeldman@chromium.org changed reviewers: + ben@chromium.org - beng@chromium.org, pfeldman@chromium.org
> - retro-fitting the DOM domain backend is probably fine as you test the waters, > but I wonder if you should go ahead and introduce your own domain here + your > own panel in Devtools longer term. For now, we are hoping to be able to use the devtools front-end as is, especially since the DOM panels provide all the functionality we want to expose right now. However, I think this would be good to do in the long-term. mhashmi@ Mind filing a bug to track this?
https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/BUILD.gn File ui/devtools/BUILD.gn (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/BUILD.gn#n... ui/devtools/BUILD.gn:14: "DOM.cpp", Sort https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:34: friend class UiDevToolsServer; This should not need to be a friend. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:36: bool connected(); Make it a const method. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... File ui/devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:45: &port); Use some default port if the flag is there, but it doesn't give a port#? (e.g. user just uses --enable-ui-devtools) https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:53: clients_.push_back(std::move(client)); It looks like we are never removing the client from this list? Is this expected? https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:82: if (socket->ListenWithAddressAndPort(address_string, port, kBacklog) != Define kBacklog just before this line here. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:132: client->connection_id_ = connection_id; client->set_connection_id(connection_id) instead, so this doesn't need to friend the client. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:145: if (!client) { |client| should never be null here, right? https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:163: if (client) ditto re client being non-null. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... File ui/devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:26: static std::unique_ptr<UiDevToolsServer> Create(); Document that this can return null. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/string_util.h File ui/devtools/string_util.h (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/string_uti... ui/devtools/string_util.h:28: CustomStringBuilder() : s_("") {} You shouldn't need s_(""). https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/string_uti... ui/devtools/string_util.h:33: void append(const char*, unsigned int length) { return; } This is incomplete? https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/switches.cc File ui/devtools/switches.cc (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/switches.c... ui/devtools/switches.cc:10: const char kEnableUiDevTools[] = "enable-ui-devtools"; Document that the value should be the port to use (and document the default port#, if no port-number is provided in the cmd flag).
> Do you think we could place the client and the server > classes into more accessible location for other services > to be able to use it? Do you have any suggestions as to where it could go? > retro-fitting the DOM domain backend is probably fine as > you test the waters, but I wonder if you should go ahead > and introduce your own domain here + your own panel in > Devtools longer term. In order to do that, we'd probably have to write our own front-end so that it can actually understand the protocol. By using the existing protocols, we can use every single aspect of the inspector by simply providing our data in the form that it wants, and it'll display it. I guess the disadvantage here is that we can't customize the way the data is presented, but for now it should be enough. In the long-term we could definitely consider this, I've filed a bug for this here crbug.com/656867. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/BUILD.gn File ui/devtools/BUILD.gn (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/BUILD.gn#n... ui/devtools/BUILD.gn:14: "DOM.cpp", On 2016/10/18 01:09:15, sadrul wrote: > Sort Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:34: friend class UiDevToolsServer; On 2016/10/18 01:09:15, sadrul wrote: > This should not need to be a friend. Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:36: bool connected(); On 2016/10/18 01:09:15, sadrul wrote: > Make it a const method. Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... File ui/devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:45: &port); On 2016/10/18 01:09:15, sadrul wrote: > Use some default port if the flag is there, but it doesn't give a port#? (e.g. > user just uses --enable-ui-devtools) Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:53: clients_.push_back(std::move(client)); On 2016/10/18 01:09:15, sadrul wrote: > It looks like we are never removing the client from this list? Is this expected? It would be possible if in the future we detach clients when they are destroyed. The main reason for the list was so that the server would own all the DT clients. Is there a better way to approach this? https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:82: if (socket->ListenWithAddressAndPort(address_string, port, kBacklog) != On 2016/10/18 01:09:15, sadrul wrote: > Define kBacklog just before this line here. Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:132: client->connection_id_ = connection_id; On 2016/10/18 01:09:15, sadrul wrote: > client->set_connection_id(connection_id) instead, so this doesn't need to friend > the client. Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:145: if (!client) { On 2016/10/18 01:09:15, sadrul wrote: > |client| should never be null here, right? Can't it be null if the client is destroyed but the server is still up? I guess this would only be possible when we turn the server into a service. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:163: if (client) On 2016/10/18 01:09:15, sadrul wrote: > ditto re client being non-null. Replied above. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... File ui/devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.h:26: static std::unique_ptr<UiDevToolsServer> Create(); On 2016/10/18 01:09:15, sadrul wrote: > Document that this can return null. Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/string_util.h File ui/devtools/string_util.h (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/string_uti... ui/devtools/string_util.h:28: CustomStringBuilder() : s_("") {} On 2016/10/18 01:09:15, sadrul wrote: > You shouldn't need s_(""). Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/string_uti... ui/devtools/string_util.h:33: void append(const char*, unsigned int length) { return; } On 2016/10/18 01:09:15, sadrul wrote: > This is incomplete? Was going to keep this file simple until we figured out how to deal with 8/16 bit strings. For now, added simple implementation. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/switches.cc File ui/devtools/switches.cc (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/switches.c... ui/devtools/switches.cc:10: const char kEnableUiDevTools[] = "enable-ui-devtools"; On 2016/10/18 01:09:15, sadrul wrote: > Document that the value should be the port to use (and document the default > port#, if no port-number is provided in the cmd flag). Done.
sadrul@chromium.org changed reviewers: + sky@chromium.org
Looks good. Just a few more comments. +sky@ for additional owner review. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... File ui/devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:53: clients_.push_back(std::move(client)); On 2016/10/18 02:40:48, mhashmi wrote: > On 2016/10/18 01:09:15, sadrul wrote: > > It looks like we are never removing the client from this list? Is this > expected? > > It would be possible if in the future we detach clients when they are destroyed. > The main reason for the list was so that the server would own all the DT > clients. Is there a better way to approach this? I think this is OK for now. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:145: if (!client) { On 2016/10/18 02:40:49, mhashmi wrote: > On 2016/10/18 01:09:15, sadrul wrote: > > |client| should never be null here, right? > > Can't it be null if the client is destroyed but the server is still up? I guess > this would only be possible when we turn the server into a service. Since UiDevToolsServer owns the client (and also the |connections_| map), it should not become null. If the server becomes a service at some point, then it will need mechanism to allow clients to unregister themselves, and that should involve tearing down the sockets etc., which means we should stop receiving messages for those non-existent clients too. So I think a DCHECK that client is not null here (and below) would still make sense. https://codereview.chromium.org/2374513002/diff/180001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/180001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:28: UiDevToolsClient(const String& name, UiDevToolsServer* server); Let's use const std::string& here. (you probably don't need the strint_util.h include above) https://codereview.chromium.org/2374513002/diff/180001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:36: std::string name() const; return a const-ref as well, i.e.: const std::string& name() const { return name_; }
Sadrul, are there specific things you want me to review here? I'm ok with just you reviewing this. One meta comment though. In hopes of making ui less of a dumping ground we're trying to only add things to ui that are lower level foundation pieces used for building ui features. ui/aura and ui/views fit this bill, but ui/keyboard doesn't. I think this feature isn't a building block either, but more and end feature. Can we move it to components?
On 2016/10/18 21:42:19, sky wrote: > Sadrul, are there specific things you want me to review here? I'm ok with just > you reviewing this. OK, cool. I wanted to make sure that you are aware that the code for this is going to start landing, and get feedback (if any) about code location, naming etc. > > One meta comment though. In hopes of making ui less of a dumping ground we're > trying to only add things to ui that are lower level foundation pieces used for > building ui features. ui/aura and ui/views fit this bill, but ui/keyboard > doesn't. I think this feature isn't a building block either, but more and end > feature. Can we move it to components? Moving to //components/ makes sense, yeah. Although not sure if it should be //components/devtools, or //components/ui_devtools. Thoughts?
components/ui_devtools SGTM On Wed, Oct 19, 2016 at 9:37 AM, <sadrul@chromium.org> wrote: > On 2016/10/18 21:42:19, sky wrote: >> Sadrul, are there specific things you want me to review here? I'm ok with >> just >> you reviewing this. > > OK, cool. I wanted to make sure that you are aware that the code for this is > going to start landing, and get feedback (if any) about code location, > naming > etc. > >> >> One meta comment though. In hopes of making ui less of a dumping ground >> we're >> trying to only add things to ui that are lower level foundation pieces >> used > for >> building ui features. ui/aura and ui/views fit this bill, but ui/keyboard >> doesn't. I think this feature isn't a building block either, but more and >> end >> feature. Can we move it to components? > > Moving to //components/ makes sense, yeah. Although not sure if it should be > //components/devtools, or //components/ui_devtools. Thoughts? > > https://codereview.chromium.org/2374513002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
components/ui_devtools SGTM On Wed, Oct 19, 2016 at 9:37 AM, <sadrul@chromium.org> wrote: > On 2016/10/18 21:42:19, sky wrote: >> Sadrul, are there specific things you want me to review here? I'm ok with >> just >> you reviewing this. > > OK, cool. I wanted to make sure that you are aware that the code for this is > going to start landing, and get feedback (if any) about code location, > naming > etc. > >> >> One meta comment though. In hopes of making ui less of a dumping ground >> we're >> trying to only add things to ui that are lower level foundation pieces >> used > for >> building ui features. ui/aura and ui/views fit this bill, but ui/keyboard >> doesn't. I think this feature isn't a building block either, but more and >> end >> feature. Can we move it to components? > > Moving to //components/ makes sense, yeah. Although not sure if it should be > //components/devtools, or //components/ui_devtools. Thoughts? > > https://codereview.chromium.org/2374513002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... File ui/devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:145: if (!client) { On 2016/10/18 20:29:37, sadrul wrote: > On 2016/10/18 02:40:49, mhashmi wrote: > > On 2016/10/18 01:09:15, sadrul wrote: > > > |client| should never be null here, right? > > > > Can't it be null if the client is destroyed but the server is still up? I > guess > > this would only be possible when we turn the server into a service. > > Since UiDevToolsServer owns the client (and also the |connections_| map), it > should not become null. If the server becomes a service at some point, then it > will need mechanism to allow clients to unregister themselves, and that should > involve tearing down the sockets etc., which means we should stop receiving > messages for those non-existent clients too. So I think a DCHECK that client is > not null here (and below) would still make sense. Done. https://codereview.chromium.org/2374513002/diff/160001/ui/devtools/devtools_s... ui/devtools/devtools_server.cc:163: if (client) On 2016/10/18 02:40:48, mhashmi wrote: > On 2016/10/18 01:09:15, sadrul wrote: > > ditto re client being non-null. > > Replied above. Done. https://codereview.chromium.org/2374513002/diff/180001/ui/devtools/devtools_c... File ui/devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/180001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:28: UiDevToolsClient(const String& name, UiDevToolsServer* server); On 2016/10/18 20:29:37, sadrul wrote: > Let's use const std::string& here. (you probably don't need the strint_util.h > include above) Done. https://codereview.chromium.org/2374513002/diff/180001/ui/devtools/devtools_c... ui/devtools/devtools_client.h:36: std::string name() const; On 2016/10/18 20:29:37, sadrul wrote: > return a const-ref as well, i.e.: > const std::string& name() const { return name_; } Done.
lgtm https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_client.cc (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_client.cc:58: } The order should be the same as the order in the header files. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_client.h:31: void Dispatch(const String& data); const std::string& https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_server.cc:35: // Returns an empty unique_ptr if ui devtools flag isn't enabled. Remove this comment. The comment in the header is sufficient. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_server.h:5: #ifndef UI_DEVTOOLS_DEVTOOLS_SERVER_H_ Update the include guards in all the header files.
sadrul@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman@ would you mind reviewing this CL as well? Thank you! Can you also add an OWNER file, with dgozman@ and myself in the OWNERS file for now?
I looked at generator usage, and it LGTM. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_client.h:23: class UiDevToolsClient : public protocol::FrontendChannel { Maybe I misunderstand something, but this doesn't look like a "client", but rather a real backend/connection/channel implementation. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/string_util.cc (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/string_util.cc:16: // TODO(mhashmi): 16-bit strings need to be handled If ui subsystem works with utf8-encoded std::strings, there is no need to support 16 bit.
Added OWNERS file. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_client.cc (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_client.cc:58: } On 2016/10/20 16:56:29, sadrul wrote: > The order should be the same as the order in the header files. Done. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_client.h (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_client.h:23: class UiDevToolsClient : public protocol::FrontendChannel { On 2016/10/20 20:45:58, dgozman wrote: > Maybe I misunderstand something, but this doesn't look like a "client", but > rather a real backend/connection/channel implementation. You are correct in thinking that. This is a UI client in the sense that it is a client of the UI server. It has all the backend implementation but in the end, it still receives messages from the server and responds through it. The inspector can therefore inspect attached "clients" through the server. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_client.h:31: void Dispatch(const String& data); On 2016/10/20 16:56:29, sadrul wrote: > const std::string& Done. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_server.cc:35: // Returns an empty unique_ptr if ui devtools flag isn't enabled. On 2016/10/20 16:56:29, sadrul wrote: > Remove this comment. The comment in the header is sufficient. Done. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/devtools_server.h:5: #ifndef UI_DEVTOOLS_DEVTOOLS_SERVER_H_ On 2016/10/20 16:56:29, sadrul wrote: > Update the include guards in all the header files. Done. https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... File components/ui_devtools/string_util.cc (right): https://codereview.chromium.org/2374513002/diff/200001/components/ui_devtools... components/ui_devtools/string_util.cc:16: // TODO(mhashmi): 16-bit strings need to be handled On 2016/10/20 20:45:58, dgozman wrote: > If ui subsystem works with utf8-encoded std::strings, there is no need to > support 16 bit. Will look into this, thanks!
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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...
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_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 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...
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: 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...
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, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2374513002/#ps460001 (title: "Remove ui_devtools dependency from Ash (will add back in next CL)")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mhashmi@chromium.org changed reviewers: + thakis@chromium.org
+thakis@ for DEPS owner review
lgtm I think you'll need a net owner too
mhashmi@chromium.org changed reviewers: + juliatuttle@chromium.org
+juliatuttle@ for net DEPS owner review
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...
On 2016/10/25 20:59:26, Sarmad Hashmi wrote: > +juliatuttle@ for net DEPS owner review net DEP lgtm
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, thakis@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2374513002/#ps480001 (title: "Add inspector_protocol to DEPS")
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 ui devtools server Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 ========== to ========== Add ui devtools server Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Add ui devtools server Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 ========== to ========== Add ui devtools server Implement a basic devtools server for mus/ash in order to inspect the window server tree hierarchy from a chrome devtools client. This can be enabled by starting chrome with the flag --enable-ui-devtools=<port>. With this implementation, a basic server that only communicates with web sockets is started. It responds with the hierarchy when the devtools client connects. BUG=648701 CQ-DEPEND=CL:2365283003 Committed: https://crrev.com/1ec338a7f29b26c0e6ae4c5c276d6014e00b77c9 Cr-Commit-Position: refs/heads/master@{#427506} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/1ec338a7f29b26c0e6ae4c5c276d6014e00b77c9 Cr-Commit-Position: refs/heads/master@{#427506} |
