Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: initiate remote debugger with firefox runner's port #923

Merged
merged 1 commit into from
May 2, 2017
Merged

fix: initiate remote debugger with firefox runner's port #923

merged 1 commit into from
May 2, 2017

Conversation

ccarruitero
Copy link
Contributor

Fixes #821

@ccarruitero ccarruitero mentioned this pull request Apr 25, 2017
Closed
kumar303
kumar303 reviewed Apr 25, 2017
View reviewed changes
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Looks like you're on the right track. Let me know what you think of this...

src/cmd/run.js Outdated
@@ -177,6 +177,7 @@ export type CreateFirefoxClientParams = {|
|};

export function defaultFirefoxClient(
port: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

This argument should be an object key, not a positional argument

src/cmd/run.js Outdated
@@ -296,7 +297,8 @@ export default async function run(
log.debug('Not installing as temporary add-on because the ' +
'add-on was already installed');
} else if (requiresRemote) {
client = await firefoxClient();
const port = runningFirefox.spawnargs[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just tell Firefox to start the debugger server on a specific port. You would pass it like binaryArgs.push('--listen', '6005') (see fx-runner).

Copy link
Contributor Author

@ccarruitero ccarruitero Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We already started firefox at this point, runningFirefox is the process returned from fx-runner. That we need here is retrieve the port in which the debugger server was started.

I was thinking in maybe extend the process. But I don't think that will be easy. So maybe we just can move when find the port and run that before initiate the runner and then pass it to the runner and the client.

@coveralls
Copy link

coveralls commented Apr 26, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 1e7e4f1 on ccarruitero:fix821 into 542f9ce on mozilla:master.

@ccarruitero ccarruitero changed the title [WIP] fix: initiate remote debugger with firefox runner's port fix: initiate remote debugger with firefox runner's port Apr 26, 2017
kumar303
kumar303 reviewed Apr 26, 2017
View reviewed changes
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

I think by just returning the remote port from firefox.run() then this patch could be simplified. I haven't tested it though -- does that work?

src/firefox/index.js Outdated
}: FirefoxRunOptions = {}
): Promise<FirefoxProcess> {

log.debug(`Running Firefox with profile at ${profile.path()}`);

const remotePort = await findRemotePort();
const remotePort = port || await findRemotePort();
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. I just realized that the call to await findRemotePort() means that Firefox was always starting up with a unique remote debugger port. Maybe all we need to do is return the port? Currently it returns the firefox instance so maybe it needs to return {firefox, port} instead.

src/cmd/run.js Outdated
@@ -290,13 +291,14 @@ export default async function run(
installed = true;
}

const runningFirefox = await runner.run(profile);
const remotePort = await runner.findPort();
const runningFirefox = await runner.run(profile, remotePort);
Copy link
Contributor

Choose a reason for hiding this comment

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

after the change I mentioned below, this could be:

const { firefox: runningFirefox, port: remotePort } = await runner.run(profile, remotePort);

@coveralls
Copy link

coveralls commented Apr 28, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 0feada3 on ccarruitero:fix821 into 36cc19f on mozilla:master.

ccarruitero added a commit to ccarruitero/web-ext that referenced this pull request Apr 28, 2017
@ccarruitero
fix: initiate remote debugger client with firefox runner port (mozill…
78cf2f8 
…a#923)
@coveralls
Copy link

coveralls commented Apr 28, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 78cf2f8 on ccarruitero:fix821 into 36cc19f on mozilla:master.

kumar303
kumar303 suggested changes May 1, 2017
View reviewed changes
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This approach looks great. I just requested some variable renaming which means we're almost there!

src/firefox/index.js Outdated
@@ -109,6 +109,11 @@ export type FirefoxRunnerFn =
(params: FirefoxRunnerParams) => Promise<FirefoxRunnerResults>;


export type FirefoxRunResponse = {|
Copy link
Contributor

Choose a reason for hiding this comment

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

We should call this FirefoxInfo to make it more specific to the data rather than the method that returns the data.

src/firefox/index.js Outdated
@@ -109,6 +109,11 @@ export type FirefoxRunnerFn =
(params: FirefoxRunnerParams) => Promise<FirefoxRunnerResults>;


export type FirefoxRunResponse = {|
firefox: FirefoxProcess,
port: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be called debuggerPort to make it unambiguous.

tests/unit/test-cmd/test.run.js Outdated
@@ -104,6 +105,21 @@ describe('run', () => {
});
});

it('run extension in correct port', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar nitpick: it('runs the extension...

@kumar303 kumar303 mentioned this pull request May 1, 2017
Closed
@ccarruitero
Copy link
Contributor Author

@kumar303 I just amend the changes. Let me know if there are something else.

@coveralls
Copy link

coveralls commented May 2, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 3223222 on ccarruitero:fix821 into edf7f0e on mozilla:master.

kumar303
kumar303 approved these changes May 2, 2017
View reviewed changes
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! I tested it manually and the bug is fixed.

@kumar303 kumar303 merged commit bfffba3 into mozilla:master May 2, 2017
@caitmuenster
Copy link

Thanks so much, @ccarruitero! Your contribution has been added to our recognition wiki and your mozillians.org profile has been vouched for.

Welcome onboard! I look forward to seeing you around!

@ccarruitero ccarruitero deleted the fix821 branch May 4, 2017 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumar303 kumar303 kumar303 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ccarruitero @coveralls @caitmuenster @kumar303 @muffinresearch