Skip to content
This repository has been archived by the owner on Dec 6, 2022. It is now read-only.

Added the extraCRDPChannelPort parameter and updated logs to be generated on %tmpdir%\vscode-chrome-debug.txt #465

Merged
merged 3 commits into from
Jul 26, 2017
Merged

Added the extraCRDPChannelPort parameter and updated logs to be generated on %tmpdir%\vscode-chrome-debug.txt #465

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e04d17
Added the extraCRDPChannelPort parameter and updated logs to be gener…
Jul 25, 2017
c6d542b
Updated core package version, and reverted some of the logfile logic
Jul 26, 2017
626d56c
Make tests pass
Jul 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/chromeDebug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import {ChromeDebugSession, logger, UrlPathTransformer, BaseSourceMapTransformer} from 'vscode-chrome-debug-core';
import * as path from 'path';
import * as os from 'os';
import * as fs from 'fs';
import {targetFilter} from './utils';

import {ChromeDebugAdapter} from './chromeDebugAdapter';
Expand All @@ -14,9 +16,10 @@ const EXTENSION_NAME = 'debugger-for-chrome';
declare let VERSION: string;
let versionWithDefault = typeof VERSION === 'undefined' ? 'unspecified' : VERSION; // Not built with webpack for tests

// non-.txt file types can't be uploaded to github
// also note that __dirname here is out/
const logFilePath = path.resolve(__dirname, '../vscode-chrome-debug.txt');
// non-.txt file types can't be uploaded to github. Generated filename is something like: vscode-chrome-debug_2017-07-19_161210.txt
const timestampForFilename = new Date().toISOString().replace(/T/, '_').replace(/:/g, '').replace(/\..*/, '');
Copy link
Member

@roblourens roblourens Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this only needed for your tests? Could the adapter process be started with an environment var giving the name of the log file, or a flag to use the timestamp? Is that reasonable?

This is fine and tmpdir is fine, but I'm still thinking of a way to keep it writing to the same file on each run for vscode.

How will you find the log file? Are you just looking for the newest one in the tmpdir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only added the timestamp to avoid losing the logs the second time we run the debugger. Would appending to the log file rather than starting from scratch work for you?

Copy link
Member

Choose a reason for hiding this comment

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

No, then it would just get huge. I just prefer overwriting one file but we can keep this if it's what you need.

Technically we could specify this in the launch config, because the file name isn't actually used until the logger is initialized. It's just bad design on my part to set up the hypothetical file name ahead of time. But I don't care enough to change it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll leave the tmpdir change in this PR (it's the most important change for us), and we might send you a new PR with other logchanges using a launch.json parameter in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember this coming up partly because of your tests running multiple instances of the adapter at once, and trampling each others' log files. If that's the case, you may not want to strip the milliseconds portion of the timestamp, because if two adapters start within the same second, they'll get the same log file name.

If I'm remembering wrong, ignore it...

const logFilePath = path.join(os.tmpdir(), `vscode-chrome-debug_${timestampForFilename}.txt`)


// Start a ChromeDebugSession configured to only match 'page' targets, which are Chrome tabs.
// Cast because DebugSession is declared twice - in this repo's vscode-debugadapter, and that of -core... TODO
Expand Down
6 changes: 3 additions & 3 deletions src/chromeDebugAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export class ChromeDebugAdapter extends CoreDebugAdapter {
});

return args.noDebug ? undefined :
this.doAttach(port, launchUrl || args.urlFilter, args.address, args.timeout);
this.doAttach(port, launchUrl || args.urlFilter, args.address, args.timeout, undefined, args.extraCRDPChannelPort);
});
}

Expand All @@ -111,8 +111,8 @@ export class ChromeDebugAdapter extends CoreDebugAdapter {
super.commonArgs(args);
}

protected doAttach(port: number, targetUrl?: string, address?: string, timeout?: number): Promise<void> {
return super.doAttach(port, targetUrl, address, timeout).then(() => {
protected doAttach(port: number, targetUrl?: string, address?: string, timeout?: number, websocketUrl?: string, extraCRDPChannelPort?: number): Promise<void> {
return super.doAttach(port, targetUrl, address, timeout, websocketUrl, extraCRDPChannelPort).then(() => {
// Don't return this promise, a failure shouldn't fail attach
this.globalEvaluate({ expression: 'navigator.userAgent', silent: true })
.then(
Expand Down