Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502863002:
Add UI DevTools under chrome://inspect/#other (Closed)
|
Created:
4 years, 1 month ago by Sarmad Hashmi Modified:
4 years, 1 month ago CC:
chromium-reviews, arv+watch_chromium.org, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UI DevTools under chrome://inspect/#other
This CL lists all clients that are attached to the UiDevToolsServer under
chrome://inspect/#other. In order to access the active devtools server,
we need to have the current (and only instance) as a static variable so
the inspect UI can get all the client urls.
In addition, we have a separate command, inspect-ui in inspect.js and
inspect_ui. This is because UiDevTools doesn't need any DevToolsHandler
as it has a custom backend. We simply open the link in a new tab when
the user clicks 'inspect'.
BUG=648701
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6d1b1cb2d0fedff985eae5bd76fa13561262774e
Cr-Commit-Position: refs/heads/master@{#433878}
Patch Set 1 #Patch Set 2 : Add UI DevTools under chrome://inspect/#other #Patch Set 3 : Add ui_devtools to BUILD file #Patch Set 4 : Add ui_devtools to BUILD file #
Total comments: 8
Patch Set 5 : dgozman's comments #
Total comments: 8
Patch Set 6 : sadruls comments #
Total comments: 4
Patch Set 7 : feedback #
Messages
Total messages: 41 (30 generated)
Description was changed from ========== Add UI DevTools under chrome://inspect/#other This CL lists all clients that are attached to the UiDevToolsServer under chrome://inspect/#other. In order to access the active devtools server, we need to have the current (and only instance) as a static variable so the inspect UI can get all the client urls. In addition, we have a separate command, inspect-ui in inspect.js and inspect_ui. This is because UiDevTools doesn't need any DevToolsHandler as it has a custom backend. We simply open the link in a new tab when the user clicks 'inspect'. BUG=648701 ========== to ========== Add UI DevTools under chrome://inspect/#other This CL lists all clients that are attached to the UiDevToolsServer under chrome://inspect/#other. In order to access the active devtools server, we need to have the current (and only instance) as a static variable so the inspect UI can get all the client urls. In addition, we have a separate command, inspect-ui in inspect.js and inspect_ui. This is because UiDevTools doesn't need any DevToolsHandler as it has a custom backend. We simply open the link in a new tab when the user clicks 'inspect'. BUG=648701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...)
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: + dgozman@chromium.org, sadrul@chromium.org
Please take a look dgozman@ and sadrul@.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... File chrome/browser/devtools/BUILD.gn (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... chrome/browser/devtools/BUILD.gn:63: "//components/ui_devtools", Let's move this dependency to inspect_ui.cc. https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... File chrome/browser/devtools/devtools_targets_ui.cc (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... chrome/browser/devtools/devtools_targets_ui.cc:279: PopulateTargetsWithUiDevtools(list_value); Instead of adding to local targets, let's handle it separately in inspect_ui.cc. We can have a separate command 'populate-additional-targets' from cc to js similarly to inspect-additional going back. https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/inspect_ui.cc (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/inspect_ui.cc:42: const char kInspectUiCommand[] = "inspect-ui"; inspect-additional https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/inspect_ui.h (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/inspect_ui.h:86: void PopulateTargetsWithUiDevtools(const std::string& source_id, This method has no callers and no implementation.
populateTargets removes everything in others-list every time its called, hence the removeAdditionalChildren/removeChildrenExceptAdditional methods. https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... File chrome/browser/devtools/BUILD.gn (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... chrome/browser/devtools/BUILD.gn:63: "//components/ui_devtools", On 2016/11/15 23:47:08, dgozman wrote: > Let's move this dependency to inspect_ui.cc. Done. https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... File chrome/browser/devtools/devtools_targets_ui.cc (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/devtools... chrome/browser/devtools/devtools_targets_ui.cc:279: PopulateTargetsWithUiDevtools(list_value); On 2016/11/15 23:47:08, dgozman wrote: > Instead of adding to local targets, let's handle it separately in inspect_ui.cc. > We can have a separate command 'populate-additional-targets' from cc to js > similarly to inspect-additional going back. Done. https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/inspect_ui.cc (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/inspect_ui.cc:42: const char kInspectUiCommand[] = "inspect-ui"; On 2016/11/15 23:47:08, dgozman wrote: > inspect-additional Done. https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/inspect_ui.h (right): https://codereview.chromium.org/2502863002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/inspect_ui.h:86: void PopulateTargetsWithUiDevtools(const std::string& source_id, On 2016/11/15 23:47:08, dgozman wrote: > This method has no callers and no implementation. Removed.
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/2502863002/diff/80001/components/ui_devtools/... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:36: base::StringToInt(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( StringToInt is expected to leave |port| unchanged if it can't parse correctly? https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:49: if (task_runner_) You need to set |devtools_server_| before the early return. DCHECK() that devtools_server_ is nullptr in this ctor. https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.h:28: // Returns an empty unique_ptr if ui devtools flag isn't enabled. Document that this can return null if an instance has already been created.
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...
https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:36: base::StringToInt(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2016/11/17 18:40:57, sadrul wrote: > StringToInt is expected to leave |port| unchanged if it can't parse correctly? Just checked, it doesn't. Sets it to various values depending on the error and returns false (if error). We could have a DCHECK here that asserts true, or we could set to 9223 if StringToInt returns false. What do you think? https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:49: if (task_runner_) On 2016/11/17 18:40:57, sadrul wrote: > You need to set |devtools_server_| before the early return. DCHECK() that > devtools_server_ is nullptr in this ctor. Done. https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... File components/ui_devtools/devtools_server.h (right): https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.h:28: // Returns an empty unique_ptr if ui devtools flag isn't enabled. On 2016/11/17 18:40:57, sadrul wrote: > Document that this can return null if an instance has already been created. Done.
lgtm https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:36: base::StringToInt(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2016/11/21 17:18:24, Sarmad Hashmi wrote: > On 2016/11/17 18:40:57, sadrul wrote: > > StringToInt is expected to leave |port| unchanged if it can't parse correctly? > > Just checked, it doesn't. Sets it to various values depending on the error and > returns false (if error). We could have a DCHECK here that asserts true, or we > could set to 9223 if StringToInt returns false. What do you think? I think we should set to default if StringToInt() fails. So: constexprt int kDefaultPort = 9223; int port; if (!StringToInt(...)) port = kDefaultPort;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2502863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/inspect/inspect.js (right): https://codereview.chromium.org/2502863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/inspect/inspect.js:49: var elements = element.querySelectorAll('[additional=true]'); Let's use class instead of a property. https://codereview.chromium.org/2502863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/inspect/inspect.js:57: for (var i = 0; i != elements.length; i++) {} around the body
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...
https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2502863002/diff/80001/components/ui_devtools/... components/ui_devtools/devtools_server.cc:36: base::StringToInt(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2016/11/21 18:26:39, sadrul wrote: > On 2016/11/21 17:18:24, Sarmad Hashmi wrote: > > On 2016/11/17 18:40:57, sadrul wrote: > > > StringToInt is expected to leave |port| unchanged if it can't parse > correctly? > > > > Just checked, it doesn't. Sets it to various values depending on the error and > > returns false (if error). We could have a DCHECK here that asserts true, or we > > could set to 9223 if StringToInt returns false. What do you think? > > I think we should set to default if StringToInt() fails. So: > constexprt int kDefaultPort = 9223; > int port; > if (!StringToInt(...)) > port = kDefaultPort; Done. https://codereview.chromium.org/2502863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/inspect/inspect.js (right): https://codereview.chromium.org/2502863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/inspect/inspect.js:49: var elements = element.querySelectorAll('[additional=true]'); On 2016/11/21 19:32:55, dgozman wrote: > Let's use class instead of a property. Done. https://codereview.chromium.org/2502863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/inspect/inspect.js:57: for (var i = 0; i != elements.length; i++) On 2016/11/21 19:32:56, dgozman wrote: > {} around the body Removed if statement and replaced querySelectorAll parameter with .row:not(.additional).
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 dgozman@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2502863002/#ps120001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479830371595040,
"parent_rev": "c4b7f4416c51a3eb9b86c9880c44874fd10a69c7", "commit_rev":
"7607fa49df3528fea39cb246d9250e9a9d9d7d32"}
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add UI DevTools under chrome://inspect/#other This CL lists all clients that are attached to the UiDevToolsServer under chrome://inspect/#other. In order to access the active devtools server, we need to have the current (and only instance) as a static variable so the inspect UI can get all the client urls. In addition, we have a separate command, inspect-ui in inspect.js and inspect_ui. This is because UiDevTools doesn't need any DevToolsHandler as it has a custom backend. We simply open the link in a new tab when the user clicks 'inspect'. BUG=648701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add UI DevTools under chrome://inspect/#other This CL lists all clients that are attached to the UiDevToolsServer under chrome://inspect/#other. In order to access the active devtools server, we need to have the current (and only instance) as a static variable so the inspect UI can get all the client urls. In addition, we have a separate command, inspect-ui in inspect.js and inspect_ui. This is because UiDevTools doesn't need any DevToolsHandler as it has a custom backend. We simply open the link in a new tab when the user clicks 'inspect'. BUG=648701 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6d1b1cb2d0fedff985eae5bd76fa13561262774e Cr-Commit-Position: refs/heads/master@{#433878} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6d1b1cb2d0fedff985eae5bd76fa13561262774e Cr-Commit-Position: refs/heads/master@{#433878} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
