Skip to content

Conversation

@alexkozy
Copy link
Member

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. [email protected][email protected] 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 <[email protected]> Reviewed-by: Alexei Filippov <[email protected]> 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
Copy link
Collaborator

@alexkozyalexkozy requested a review from ofrobotsAugust 10, 2018 20:13
@nodejs-github-botnodejs-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
@alexkozyalexkozy added the inspector Issues and PRs related to the V8 inspector protocol label Aug 10, 2018
@alexkozy
Copy link
MemberAuthor

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

@Trott
Copy link
Member

@nodejs/v8

@TimothyGu
Copy link
Member

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

@alexkozy
Copy link
MemberAuthor

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

@alexkozy
Copy link
MemberAuthor

@alexkozy
Copy link
MemberAuthor

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

Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newly added code LGTM. Some logistical comments.

src/node_url.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
MemberAuthor

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

TimothyGu
TimothyGu previously approved these changes Aug 30, 2018
Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@TimothyGuTimothyGu dismissed their stale reviewAugust 30, 2018 23:37

Dismiss stale review

@alexkozy
Copy link
MemberAuthor

alexkozy commented Aug 31, 2018

@addaleaxaddaleax added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Sep 3, 2018
@alexkozy
Copy link
MemberAuthor

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

@BridgeAR
Copy link
Member

This needs a rebase.

@BridgeAR
Copy link
Member

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

Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if tests pass.

@alexkozy
Copy link
MemberAuthor

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

@alexkozy
Copy link
MemberAuthor

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

@alexkozy
Copy link
MemberAuthor

@alexkozy
Copy link
MemberAuthor

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?

kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Sep 19, 2018
Wired up support for calling the node-provided `resourceNameToUrl` method in the chakrashim inspector. This reverts commit 3f6ef13. PR-URL: nodejs#601Fixes: nodejs#598 Refs: nodejs/node#22251 Reviewed-By: Jimmy Thomson <[email protected]> Reviewed-By: Seth Brenith <[email protected]>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Sep 19, 2018
Wired up support for calling the node-provided `resourceNameToUrl` method in the chakrashim inspector. This reverts commit 3f6ef13. PR-URL: nodejs#601Fixes: nodejs#598 Refs: nodejs/node#22251 Reviewed-By: Jimmy Thomson <[email protected]> Reviewed-By: Seth Brenith <[email protected]>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Sep 20, 2018
Wired up support for calling the node-provided `resourceNameToUrl` method in the chakrashim inspector. This reverts commit 3f6ef13. PR-URL: nodejs#601Fixes: nodejs#598 Refs: nodejs/node#22251 Reviewed-By: Jimmy Thomson <[email protected]> Reviewed-By: Seth Brenith <[email protected]>
targos pushed a commit that referenced this pull request Sep 25, 2018
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. [email protected][email protected] 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 <[email protected]> Reviewed-by: Alexei Filippov <[email protected]> Cr-Commit-Position: refs/heads/master@{#55029} ``` Refs: v8/v8@dbfcc48 Backport-PR-URL: #22918 PR-URL: #22251 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
targos pushed a commit that referenced this pull request Sep 25, 2018
Method returns file URL from native file path. Backport-PR-URL: #22918 PR-URL: #22251 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
targos pushed a commit that referenced this pull request Sep 25, 2018
This method is required by inspector to report normalized urls over the protocol. Backport-PR-URL: #22918Fixes#22223 PR-URL: #22251 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@targostargos mentioned this pull request Oct 7, 2018
deepak1556 added a commit to electron/electron that referenced this pull request Oct 30, 2018
deepak1556 added a commit to electron/electron that referenced this pull request Nov 9, 2018
deepak1556 added a commit to electron/electron that referenced this pull request Nov 14, 2018
deepak1556 added a commit to electron/electron that referenced this pull request Nov 29, 2018
zcbenz pushed a commit to electron/electron that referenced this pull request Nov 30, 2018
deepak1556 added a commit to electron/electron that referenced this pull request Dec 2, 2018
deepak1556 added a commit to electron/electron that referenced this pull request Dec 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.inspectorIssues and PRs related to the V8 inspector protocolv8 engineIssues and PRs related to the V8 dependency.whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@alexkozy@nodejs-github-bot@Trott@TimothyGu@BridgeAR@targos@addaleax@eugeneo