inspector: use js_app.html as the landing page for chrome devtools #21385

GauthamBanasandra · 2018-06-18T16:12:26Z

As of this commit in chromium -
https://chromium-review.googlesource.com/c/chromium/src/+/905450
a new landing page (js_app.html) has been added and the existing
inspector.html has been made as the fallback page.

Another motivation for this patch is the following bug in
chromium -
https://bugs.chromium.org/p/chromium/issues/detail?id=846642
due to which, source maps won't get applied with inspector.html,
but works with js_app.html

In order to maintain compatibility, this patch adds a URL
"devtoolsFrontendUrlCompat" to the response of /json/list REST API
so that those using Chrome browsers older than 66.0.3345.0
could use this to open DevTools.

Refs: https://bugs.chromium.org/p/chromium/issues/detail?id=846642
Refs: https://chromium-review.googlesource.com/c/chromium/src/+/905450

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

vsemozhetbyt · 2018-06-18T16:42:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15507/

GauthamBanasandra · 2018-06-18T17:47:53Z

I had a look at the run node-test-commit-windows-fanned. I see that the tests have passed -

[==========] 74 tests from 9 test cases ran. (245 ms total)
[  PASSED  ] 74 tests.

But it says -

There are failed test cases and the job is configured to mark the build as failure. Marking build as FAILURE

I'm not sure why it's failing. Anything for me to fix for this patch?

Trott · 2018-06-18T23:18:25Z

@GauthamBanasandra It's a bit buried, but the test failure is in https://ci.nodejs.org/job/node-test-binary-windows/18091/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=2/console. Here it is:

not ok 296 parallel/test-message-channel
  ---
  duration_ms: 120.96
  severity: fail
  exitcode: 1
  stack: |-
    timeout

I suspect it is unrelated to your change here.

Re-running CI via "Resume Build" (so it just re-runs the subtasks that failed rather than a full CI re-run): https://ci.nodejs.org/job/node-test-pull-request/15519/

GauthamBanasandra · 2018-06-19T15:31:16Z

Is this good for merge? All checks have passed.

vsemozhetbyt · 2018-06-19T15:57:58Z

сс @nodejs/v8-inspector

Trott · 2018-06-19T18:59:20Z

Is this good for merge? All checks have passed.

@GauthamBanasandra It needs to remain open for 48 hours and be approved by at least one Collaborator before it can be merged.

richardlau · 2018-06-19T20:34:39Z

cc @nodejs/diagnostics

mmarchini · 2018-06-20T19:02:18Z

Wouldn't this make node inspect open a page that doesn't exist on previous versions of Chrome? After which Chrome version is this page available (based on the date I'm assuming 66)?

GauthamBanasandra · 2018-06-21T17:52:35Z

@mmarchini Thanks for the catch. I don't think Google officially supports downloading previous versions of the Chrome browser.

However, one could do so with Chromium. So, here's the change that I've made to address this -
I've added a new field to the response of /json/list REST API called devtoolsFrontendUrlCompat, which could be used for those who are using Chromium browsers older than 66.0.3345.0
The response of GET /json/list looks like this now -

[ {
  "description": "node.js instance",
  "devtoolsFrontendUrl": "chrome-devtools://devtools/bundled/js_app.html?experiments=true&v8only=true&ws=localhost:9229/3b418c47-4d5c-4e61-bf83-9f63866be5dc",
  "devtoolsFrontendUrlCompat": "chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=localhost:9229/3b418c47-4d5c-4e61-bf83-9f63866be5dc",
  "faviconUrl": "https://nodejs.org/static/favicon.ico",
  "id": "3b418c47-4d5c-4e61-bf83-9f63866be5dc",
  "title": "input.js",
  "type": "node",
  "url": "file:///Users/gautham/projects/github/chromium/debug-srcmap/input.js",
  "webSocketDebuggerUrl": "ws://localhost:9229/3b418c47-4d5c-4e61-bf83-9f63866be5dc"
} ]

So, devtoolsFrontendUrlCompat opens inspector.html instead of js_app.html

GauthamBanasandra · 2018-06-22T14:17:47Z

Folks, does my change look ok? Please let me know if any more changes need to be done for this patch.

jasnell · 2018-06-22T20:42:13Z

src/inspector_socket_server.cc

+    const std::string &formatted_address) {
+  std::ostringstream frontend_url;
+  frontend_url << "chrome-devtools://devtools/bundled/";
+  frontend_url << (is_compat?"inspector":"js_app");


nit: spaces between the ternary operators (e.g. is_compat ? "inspector" : "js_app")

jasnell · 2018-06-22T20:44:27Z

doc/api/debugger.md

 ```

 (In the example above, the UUID dc9010dd-f8b8-4ac5-a510-c1a114ec7d29
 at the end of the URL is generated on the fly, it varies in different
 debugging sessions.)

+> If the Chrome browser is older than <strong>66.0.3345.0</strong>,


nit: There's no need for the leading > here.

Also no need for <strong>. Would prefer markdown for emphasis, but honestly, no emphasis is needed here.

Trott · 2018-06-23T04:52:13Z

doc/api/debugger.md

 ```

 (In the example above, the UUID dc9010dd-f8b8-4ac5-a510-c1a114ec7d29
 at the end of the URL is generated on the fly, it varies in different
 debugging sessions.)

+> If the Chrome browser is older than <strong>66.0.3345.0</strong>,
+>use `inspector.html` instead of `js_app.html` in the above URL.


Would chrome://inspect work in all versions? If so, maybe it's easier to recommend using that URL than doing string substitution on the URL.

In Chromium 51.0.2704.0, chrome://inspect doesn't provide the link to open inspector.html.

Trott · 2018-06-23T04:55:28Z

Folks, does my change look ok? Please let me know if any more changes need to be done for this patch.

Left some comments on the docs, but nothing that should be considered blocking. Would be good to get a review or two in from @nodejs/diagnostics and/or @nodejs/v8-inspector. Ping!

As of this commit in chromium - https://chromium-review.googlesource.com/c/chromium/src/+/905450 a new landing page (js_app.html) has been added and inspector.html has been made as the fallback page. Another motivation for this patch is the following bug in chromium - https://bugs.chromium.org/p/chromium/issues/detail?id=846642 due to which, source maps won't get applied with inspector.html, but works with js_app.html In order to maintain compatibility, this patch adds a URL "devtoolsFrontendUrlCompat" to the response of /json/list REST API so that those using Chrome browsers older than 66.0.3345.0 could use this to open DevTools.

alexkozy

On chrome://inspect page we already use js_app.html for Node.js targets so this PR looks good to me.

Trott · 2018-06-26T05:13:50Z

/ping @jkrems Looks good from the node-inspect side of things?

jkrems

Wouldn't this make node inspect open a page that doesn't exist on previous versions of Chrome?

node inspect doesn't really care about this URL, it only cares about the websocket URL. This change LGTM.

Trott · 2018-06-26T18:40:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15637/

mmarchini · 2018-06-28T12:25:11Z

Landed in 026d279, thanks for your contribution! 🎉🎉🎉🎉🎉🎉

As of this commit in chromium - https://chromium-review.googlesource.com/c/chromium/src/+/905450 a new landing page (js_app.html) has been added and inspector.html has been made as the fallback page. Another motivation for this patch is the following bug in chromium - https://bugs.chromium.org/p/chromium/issues/detail?id=846642 due to which, source maps won't get applied with inspector.html, but works with js_app.html In order to maintain compatibility, this patch adds a URL "devtoolsFrontendUrlCompat" to the response of /json/list REST API so that those using Chrome browsers older than 66.0.3345.0 could use this to open DevTools. PR-URL: #21385 Refs: https://bugs.chromium.org/p/chromium/issues/detail?id=846642 Refs: https://chromium-review.googlesource.com/c/chromium/src/+/905450 Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

GauthamBanasandra · 2018-06-28T14:11:55Z

Thank you all :)

As of this commit in chromium - https://chromium-review.googlesource.com/c/chromium/src/+/905450 a new landing page (js_app.html) has been added and inspector.html has been made as the fallback page. Another motivation for this patch is the following bug in chromium - https://bugs.chromium.org/p/chromium/issues/detail?id=846642 due to which, source maps won't get applied with inspector.html, but works with js_app.html In order to maintain compatibility, this patch adds a URL "devtoolsFrontendUrlCompat" to the response of /json/list REST API so that those using Chrome browsers older than 66.0.3345.0 could use this to open DevTools. PR-URL: #21385 Refs: https://bugs.chromium.org/p/chromium/issues/detail?id=846642 Refs: https://chromium-review.googlesource.com/c/chromium/src/+/905450 Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

* As per nodejs/node#21385 and https://chromium.googlesource.com/chromium/src/+/f7efc442c8dc9108f057144983987e74ddc6a0d4 we need to use js_app.html instead of inspector.html * This patch also fixes the inconsistency in symbol mapping caused due to adding handler headers. Change-Id: I1427742f5305a0c71b722c3e6d699ea4b5c0ebf0 Reviewed-on: http://review.couchbase.org/96315 Reviewed-by: Sriram Melkote <siri@couchbase.com> Reviewed-by: CI Bot Tested-by: Gautham B A <gautham.bangalore@gmail.com>

* As per nodejs/node#21385 and https://chromium.googlesource.com/chromium/src/+/f7efc442c8dc9108f057144983987e74ddc6a0d4 we need to use js_app.html instead of inspector.html * This patch also fixes the inconsistency in symbol mapping caused due to adding handler headers. Change-Id: I23884175d7e8ce9fa798869659b44ba2a0ee0dca Reviewed-on: http://review.couchbase.org/97722 Well-Formed: Build Bot <build@couchbase.com> Reviewed-by: Sriram Melkote <siri@couchbase.com> Tested-by: Gautham B A <gautham.bangalore@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Jun 18, 2018

GauthamBanasandra changed the title ~~Use js_app.html as the landing page for debugger in chrome browser~~ inspector: use js_app.html as the landing page for chrome devtools Jun 18, 2018

GauthamBanasandra force-pushed the master branch 4 times, most recently from 7fe9b0c to fa9da9c Compare June 19, 2018 01:56

eugeneo requested a review from alexkozy June 19, 2018 16:36

GauthamBanasandra force-pushed the master branch from fa9da9c to 845ed4c Compare June 21, 2018 17:41

jasnell reviewed Jun 22, 2018

View reviewed changes

Trott reviewed Jun 23, 2018

View reviewed changes

GauthamBanasandra force-pushed the master branch from 845ed4c to 93402aa Compare June 23, 2018 13:58

alexkozy approved these changes Jun 25, 2018

View reviewed changes

jkrems approved these changes Jun 26, 2018

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2018

mmarchini approved these changes Jun 28, 2018

View reviewed changes

mmarchini closed this Jun 28, 2018

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

GauthamBanasandra mentioned this pull request Jul 12, 2018

Use js_app.html as the landing page for debugger hsharsha/v8inspector#2

Merged

GauthamBanasandra mentioned this pull request Aug 10, 2018

Use js_app.html as the landing page for Chrome-Devtools hsharsha/v8inspector#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector: use js_app.html as the landing page for chrome devtools #21385

inspector: use js_app.html as the landing page for chrome devtools #21385

GauthamBanasandra commented Jun 18, 2018 •

edited

Loading

vsemozhetbyt commented Jun 18, 2018

GauthamBanasandra commented Jun 18, 2018

Trott commented Jun 18, 2018

GauthamBanasandra commented Jun 19, 2018

vsemozhetbyt commented Jun 19, 2018

Trott commented Jun 19, 2018

richardlau commented Jun 19, 2018

mmarchini commented Jun 20, 2018

GauthamBanasandra commented Jun 21, 2018

GauthamBanasandra commented Jun 22, 2018

jasnell Jun 22, 2018

GauthamBanasandra Jun 23, 2018

jasnell Jun 22, 2018

Trott Jun 23, 2018

GauthamBanasandra Jun 23, 2018

Trott Jun 23, 2018

GauthamBanasandra Jun 23, 2018 •

edited

Loading

Trott commented Jun 23, 2018

alexkozy left a comment

Trott commented Jun 26, 2018

jkrems left a comment

Trott commented Jun 26, 2018

mmarchini commented Jun 28, 2018

GauthamBanasandra commented Jun 28, 2018

inspector: use js_app.html as the landing page for chrome devtools #21385

inspector: use js_app.html as the landing page for chrome devtools #21385

Conversation

GauthamBanasandra commented Jun 18, 2018 • edited Loading

Checklist

vsemozhetbyt commented Jun 18, 2018

GauthamBanasandra commented Jun 18, 2018

Trott commented Jun 18, 2018

GauthamBanasandra commented Jun 19, 2018

vsemozhetbyt commented Jun 19, 2018

Trott commented Jun 19, 2018

richardlau commented Jun 19, 2018

mmarchini commented Jun 20, 2018

GauthamBanasandra commented Jun 21, 2018

GauthamBanasandra commented Jun 22, 2018

jasnell Jun 22, 2018

Choose a reason for hiding this comment

GauthamBanasandra Jun 23, 2018

Choose a reason for hiding this comment

jasnell Jun 22, 2018

Choose a reason for hiding this comment

Trott Jun 23, 2018

Choose a reason for hiding this comment

GauthamBanasandra Jun 23, 2018

Choose a reason for hiding this comment

Trott Jun 23, 2018

Choose a reason for hiding this comment

GauthamBanasandra Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Trott commented Jun 23, 2018

alexkozy left a comment

Choose a reason for hiding this comment

Trott commented Jun 26, 2018

jkrems left a comment

Choose a reason for hiding this comment

Trott commented Jun 26, 2018

mmarchini commented Jun 28, 2018

GauthamBanasandra commented Jun 28, 2018

GauthamBanasandra commented Jun 18, 2018 •

edited

Loading

GauthamBanasandra Jun 23, 2018 •

edited

Loading