New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@GauthamBanasandra
Copy link
Contributor

GauthamBanasandra commented Jun 18, 2018

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

This comment has been minimized.

@GauthamBanasandra 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

This comment has been minimized.

Copy link
Contributor Author

GauthamBanasandra commented Jun 18, 2018

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

This comment has been minimized.

Copy link
Member

Trott commented Jun 18, 2018

@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 GauthamBanasandra force-pushed the GauthamBanasandra:master branch 4 times, most recently from 7fe9b0c to fa9da9c Jun 19, 2018

@GauthamBanasandra

This comment has been minimized.

Copy link
Contributor Author

GauthamBanasandra commented Jun 19, 2018

Is this good for merge? All checks have passed.

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jun 19, 2018

@eugeneo eugeneo requested a review from ak239 Jun 19, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jun 19, 2018

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

This comment has been minimized.

Copy link
Member

richardlau commented Jun 19, 2018

@mmarchini

This comment has been minimized.

Copy link
Member

mmarchini commented Jun 20, 2018

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 GauthamBanasandra force-pushed the GauthamBanasandra:master branch from fa9da9c to 845ed4c Jun 21, 2018

@GauthamBanasandra

This comment has been minimized.

Copy link
Contributor Author

GauthamBanasandra commented Jun 21, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

GauthamBanasandra commented Jun 22, 2018

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

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

This comment has been minimized.

@jasnell

jasnell Jun 22, 2018

Member

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

This comment has been minimized.

@GauthamBanasandra

GauthamBanasandra Jun 23, 2018

Author Contributor

Done.

```

(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>,

This comment has been minimized.

@jasnell

jasnell Jun 22, 2018

Member

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

This comment has been minimized.

@Trott

Trott Jun 23, 2018

Member

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

This comment has been minimized.

@GauthamBanasandra

GauthamBanasandra Jun 23, 2018

Author Contributor

Done.

```

(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.

This comment has been minimized.

@Trott

Trott Jun 23, 2018

Member

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.

This comment has been minimized.

@GauthamBanasandra

GauthamBanasandra Jun 23, 2018

Author Contributor

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

screen shot 2018-06-23 at 6 11 38 pm

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jun 23, 2018

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!

inspector: use js_app.html as the landing page for chrome devtools
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.

@GauthamBanasandra GauthamBanasandra force-pushed the GauthamBanasandra:master branch from 845ed4c to 93402aa Jun 23, 2018

@ak239

ak239 approved these changes Jun 25, 2018

Copy link
Member

ak239 left a comment

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

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jun 26, 2018

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

@jkrems

jkrems approved these changes Jun 26, 2018

Copy link
Contributor

jkrems left a comment

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

This comment has been minimized.

@Trott Trott added the author ready label Jun 26, 2018

@mmarchini

This comment has been minimized.

Copy link
Member

mmarchini commented Jun 28, 2018

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

@mmarchini mmarchini closed this Jun 28, 2018

mmarchini added a commit that referenced this pull request Jun 28, 2018

inspector: use js_app.html as the landing page for chrome devtools
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

This comment has been minimized.

Copy link
Contributor Author

GauthamBanasandra commented Jun 28, 2018

Thank you all :)

targos added a commit that referenced this pull request Jun 28, 2018

inspector: use js_app.html as the landing page for chrome devtools
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>

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

ns-codereview pushed a commit to couchbase/eventing that referenced this pull request Jul 4, 2018

[MB-29841] Use js_app.html as the landing page for debugger
* 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>

ns-codereview pushed a commit to couchbase/eventing that referenced this pull request Aug 3, 2018

[MB-30395] Use js_app.html as the landing page for debugger
* 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>

GauthamBanasandra added a commit to GauthamBanasandra/v8inspector that referenced this pull request Aug 10, 2018

Use js_app.html as the landing page for Chrome-Devtools
As per nodejs/node#21385 the landing
page for Chrome-Devtools has been changed to js_app.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment