deps: cherry-pick dbfcc48 from upstream V8 #22251

alexkozy · 2018-08-10T20:13:01Z

Original commit message:

[inspector] added V8InspectorClient::resourceNameToUrl

Some clients (see Node.js) use platform path as ScriptOrigin.
Reporting platform path in protocol makes using protocol much harder.
This CL introduced V8InspectorClient::resourceNameToUrl method that
is called for any reported using protocol url.
V8Inspector uses url internally as well so protocol client may generate
pattern for blackboxing with file urls only and does not need to build
complicated regexp that covers files urls and platform paths on
different platforms.

R=lushnikov@chromium.org
TBR=yangguo@chromium.org

Bug: none
Cq-Include-Trybots: luci.chromium.try:linux_chromium_headless_rel;luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iff302e7441df922fa5d689fe510f5a9bfd470b9b
Reviewed-on: https://chromium-review.googlesource.com/1164624
Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55029}

Refs: v8/v8@dbfcc48
Needed for #22223

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

nodejs-github-bot · 2018-08-10T20:13:02Z

@ak239 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/507/pipeline

alexkozy · 2018-08-10T20:13:59Z

By accident I broke my previous PR: #22224
So I created new one. Please take a look!

Trott · 2018-08-11T03:49:38Z

@nodejs/v8

TimothyGu · 2018-08-16T01:28:58Z

This LGTM by itself, but I'd prefer landing this with the actual Node.js resourceNameToUrl implementation.

alexkozy · 2018-08-21T04:44:14Z

@TimothyGu please take another look.
I pushed second commit that contains resourceNameToUrl implementation.

alexkozy · 2018-08-29T17:26:00Z

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

src/inspector_agent.cc

alexkozy · 2018-08-30T21:10:43Z

@TimothyGu I addressed your comments and added one more test. Please take another look.
CI: https://ci.nodejs.org/job/node-test-pull-request/16878/

TimothyGu

Newly added code LGTM. Some logistical comments.

TimothyGu · 2018-08-30T21:26:50Z

src/node_url.cc

@@ -2348,6 +2348,21 @@ std::string URL::ToFilePath() const {
 #endif
 }

+URL URL::FromFilePath(const std::string& file_path) {
+  URL url;
+  url.context_.scheme = "file:";


I'm a bit reluctant to touch into the internals in this way. Could something like this work?

URL url("file:///");

This will also need a cctest (see https://github.com/nodejs/node/blob/master/test/cctest/test_url.cc).

Finally could you split the changes to node_url into a separate commit?

alexkozy · 2018-08-30T22:11:51Z

Addressed comments and started another CI: https://ci.nodejs.org/job/node-test-pull-request/16880/

TimothyGu

Looks good to me.

TimothyGu

Hmm, jumped the gun there. Left a few more comments.

test/cctest/test_url.cc

src/inspector_agent.cc

Dismiss stale review

alexkozy · 2018-08-31T02:22:37Z

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

alexkozy · 2018-09-04T22:15:22Z

Failure on Windows bot looks unrelated. @TimothyGu could you take another look?

BridgeAR · 2018-09-05T20:15:01Z

This needs a rebase.

BridgeAR · 2018-09-05T20:15:23Z

@nodejs/v8-update PTAL. This needs some LGs

TimothyGu

LGTM if tests pass.

alexkozy · 2018-09-11T21:32:38Z

Rebased and started CI: https://ci.nodejs.org/job/node-test-pull-request/17120/

alexkozy · 2018-09-12T02:16:49Z

rebased and restarted CI again since I believe that failure is unrelated: https://ci.nodejs.org/job/node-test-pull-request/17126/

alexkozy · 2018-09-12T16:55:55Z

Another try: https://ci.nodejs.org/job/node-test-pull-request/17151/

alexkozy · 2018-09-13T19:58:48Z

Windows bot looks disabled on CI, based on previous CI all related tests passed, I am not sure, should I wait until windows is fixed on CI or can I land it as is?

Wired up support for calling the node-provided `resourceNameToUrl` method in the chakrashim inspector. This reverts commit 3f6ef13. PR-URL: nodejs#601 Fixes: nodejs#598 Refs: nodejs/node#22251 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com> Reviewed-By: Seth Brenith <sethb@microsoft.com>

Original commit message: ``` [inspector] added V8InspectorClient::resourceNameToUrl Some clients (see Node.js) use platform path as ScriptOrigin. Reporting platform path in protocol makes using protocol much harder. This CL introduced V8InspectorClient::resourceNameToUrl method that is called for any reported using protocol url. V8Inspector uses url internally as well so protocol client may generate pattern for blackboxing with file urls only and does not need to build complicated regexp that covers files urls and platform paths on different platforms. R=lushnikov@chromium.org TBR=yangguo@chromium.org Bug: none Cq-Include-Trybots: luci.chromium.try:linux_chromium_headless_rel;luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Iff302e7441df922fa5d689fe510f5a9bfd470b9b Reviewed-on: https://chromium-review.googlesource.com/1164624 Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-by: Alexei Filippov <alph@chromium.org> Cr-Commit-Position: refs/heads/master@{#55029} ``` Refs: v8/v8@dbfcc48 Backport-PR-URL: #22918 PR-URL: #22251 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Method returns file URL from native file path. Backport-PR-URL: #22918 PR-URL: #22251 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

This method is required by inspector to report normalized urls over the protocol. Backport-PR-URL: #22918 Fixes #22223 PR-URL: #22251 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

* nodejs/node#22251 * nodejs/node#20137

alexkozy requested a review from ofrobots August 10, 2018 20:13

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Aug 10, 2018

alexkozy added the inspector Issues and PRs related to the V8 inspector protocol label Aug 10, 2018

alexkozy requested a review from TimothyGu August 10, 2018 20:20

TimothyGu reviewed Aug 30, 2018

View reviewed changes

src/inspector_agent.cc Outdated Show resolved Hide resolved

src/inspector_agent.cc Outdated Show resolved Hide resolved

TimothyGu reviewed Aug 30, 2018

View reviewed changes

TimothyGu previously approved these changes Aug 30, 2018

View reviewed changes

TimothyGu reviewed Aug 30, 2018

View reviewed changes

test/cctest/test_url.cc Outdated Show resolved Hide resolved

test/cctest/test_url.cc Outdated Show resolved Hide resolved

test/cctest/test_url.cc Outdated Show resolved Hide resolved

src/inspector_agent.cc Outdated Show resolved Hide resolved

addaleax added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Sep 3, 2018

TimothyGu approved these changes Sep 11, 2018

View reviewed changes

eugeneo approved these changes Sep 13, 2018

View reviewed changes

alexkozy mentioned this pull request Sep 14, 2018

Debugger breakpoints using incorrect URL for a remote debugging session and therefore failing to pause the process ChromeDevTools/devtools-protocol#103

Closed

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

targos mentioned this pull request Oct 17, 2018

remote debugger unable to convert path correctly in different operating system #23688

Open

deepak1556 added a commit to electron/electron that referenced this pull request Oct 30, 2018

spec: update inspector-binding.js

1598fc2

* nodejs/node#22251 * nodejs/node#20137

niclashedam mentioned this pull request Nov 1, 2018

Coverage tool stopped working on newer node versions Siteimprove/alfa#79

Closed

deepak1556 added a commit to electron/electron that referenced this pull request Nov 9, 2018

spec: update inspector-binding.js

c966017

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Nov 14, 2018

spec: update inspector-binding.js

daa892b

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Nov 29, 2018

spec: update inspector-binding.js

1e7c765

* nodejs/node#22251 * nodejs/node#20137

zcbenz pushed a commit to electron/electron that referenced this pull request Nov 30, 2018

spec: update inspector-binding.js

e65ed45

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Dec 2, 2018

spec: update inspector-binding.js

0286a9c

* nodejs/node#22251 * nodejs/node#20137

deepak1556 added a commit to electron/electron that referenced this pull request Dec 3, 2018

spec: update inspector-binding.js

c491fa5

* nodejs/node#22251 * nodejs/node#20137

alexkozy mentioned this pull request May 26, 2020

src: add node::url::URL::href() method #22610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: cherry-pick dbfcc48 from upstream V8 #22251

deps: cherry-pick dbfcc48 from upstream V8 #22251

alexkozy commented Aug 10, 2018

nodejs-github-bot commented Aug 10, 2018

alexkozy commented Aug 10, 2018

Trott commented Aug 11, 2018

TimothyGu commented Aug 16, 2018

alexkozy commented Aug 21, 2018

alexkozy commented Aug 29, 2018

alexkozy commented Aug 30, 2018

TimothyGu left a comment

TimothyGu Aug 30, 2018

alexkozy commented Aug 30, 2018

TimothyGu left a comment

TimothyGu left a comment

alexkozy commented Aug 31, 2018 •

edited

Loading

alexkozy commented Sep 4, 2018

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 5, 2018

TimothyGu left a comment

alexkozy commented Sep 11, 2018

alexkozy commented Sep 12, 2018

alexkozy commented Sep 12, 2018

alexkozy commented Sep 13, 2018

deps: cherry-pick dbfcc48 from upstream V8 #22251

deps: cherry-pick dbfcc48 from upstream V8 #22251

Conversation

alexkozy commented Aug 10, 2018

Checklist

nodejs-github-bot commented Aug 10, 2018

alexkozy commented Aug 10, 2018

Trott commented Aug 11, 2018

TimothyGu commented Aug 16, 2018

alexkozy commented Aug 21, 2018

alexkozy commented Aug 29, 2018

alexkozy commented Aug 30, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Aug 30, 2018

Choose a reason for hiding this comment

alexkozy commented Aug 30, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

alexkozy commented Aug 31, 2018 • edited Loading

alexkozy commented Sep 4, 2018

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 5, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

alexkozy commented Sep 11, 2018

alexkozy commented Sep 12, 2018

alexkozy commented Sep 12, 2018

alexkozy commented Sep 13, 2018

alexkozy commented Aug 31, 2018 •

edited

Loading