Provide target types filter configuration #623
Conversation
|
Should I provide some additional info for this PR, make some changes or do something else? |
roblourens
left a comment
There was a problem hiding this comment.
Thanks for the PR! Sorry for the slow response. I was a little conflicted about taking it because I haven't tested it against any target other than a normal Chrome page, and don't want to advertise too much about supporting scenarios that I am not testing. I might add an extra note to that effect in the readme. But I think it's better to have this than for it to be impossible to attach to anything else.
Please just add the new attribute to the package.json (and package.nls.json) following the pattern of the other attributes, then I'll merge it.
|
It is ok to debug at least pages with other target types. JFYI: here is code that determine target type in chromium: |
| extensionName: EXTENSION_NAME, | ||
| logFilePath: path.resolve(os.tmpdir(), 'vscode-chrome-debug.txt'), | ||
| targetFilter, | ||
| targetFilter: targetFilterProvider(), |
There was a problem hiding this comment.
This doesn't work. I guess I didn't review it closely enough. Did you test this PR? This also needs changes in the -core library to allow a targetFilter to be determined by the launch config. I'm reverting this PR.
There was a problem hiding this comment.
Oh, I'm really sorry, yes of course i've tested it. Maybe I didn't check all cases. I'll try to rework it and make another PR.
There was a problem hiding this comment.
Could you please help me about changes in core library?
As far as I have seen it need delegate as a filter and does not need any information about what this delegate does. So I thought that generating delegate according to launch config will be enough.
So according to your comment I missed something, could you please point me what?
There was a problem hiding this comment.
The commented line just generates the default filter for page, since targetFilterProvider doesn't take an argument.
I think that somewhere in -core it will have to read the list of targetFilterTypes and create the filter. That will mean that targetFilter won't be passed to the session here, and ChromeConnection won't take the targetFilter in its constructor. Instead it will be an argument passed to ChromeConnection.attach https://github.com/Microsoft/vscode-chrome-debug-core/blob/master/src/chrome/chromeConnection.ts#L120
There was a problem hiding this comment.
Oh shame on me, i've completely misunderstood purpose of chromeDebug.ts file and yes, now I see that I have not test it properly. So i'm sorry for this.
Ok then, we must learn from our mistakes. As far as i see, correct way should look like this:
- Make PR to
-corelib which marktargetFilterin IChromeDebugAdapterOpts as obsolete or remove it - In same PR add support for
targetFilterTypesin ICommonRequestArgs - Merge this PR into
-corelibrary if it is possible. - Come back here, remove
targetFilterusage and migrate totargetFilterTypes - Create Issue in rest of debuggers based on
-corelib.
Does it sounds like a plan?
There is no any reasons to prohibit debug of anything but 'page'. For example i'm using this extension for debugging chrome itself, and some internal pages have type equals to 'other'.
Another case is debugging shared or service workers.
Suggestion is to add new configuration parameter for type selection, witch is defaults to
['page']to save previous behavior, but can be changed to allow debugging other types of content.