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.

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

feat: WiP - Allow connecting to an existing Firefox instance #1122

Closed
wants to merge 1 commit into from

Conversation

kewisch
Copy link

@kewisch kewisch commented Oct 27, 2017

Hey @kumar303, this is my WiP for #1100. It is mostly unpolished because I do have a few design questions open:

  • FirefoxInfo has a FirefoxProcess member that is checked against null in some of the code. When connecting to a remote port without a process, there is nothing sensible to set FirefoxProcess to. Right now what I am doing is creating a mock runningInfo that just has the debuggerPort and the process' kill function which is used for cleanup. I can imagine better ways to fix this, but I'd appreciate your input on what you prefer. There could be some refactoring involved in this.
  • Code right now assumes the only reason a process will interrupt is if the process is closed. I think it would be good to change/adapt this to react when the connection to the devtools server is interrupted. I'm not yet sure the event for this is exposed, but I would investigate. This way if you close the Firefox you've connected to it will properly disconnect. Does this sound reasonable?
  • A feature I should add is that if e.g. Ctrl+C is pressed, the last action before the connection to the devtools server is interrupted is to uninstall the temporary add-on. The code for this would have to live in the mock kill function and would be asynchronous instead. I'd likely have to adapt the FirefoxDesktopExtensionRunner::exit function to take care of the asynchronous shutdown. Does this sound right?
  • There are a few options not compatible with the --firefox-port option, e.g. --firefox-profile, --firefox-binary, --keep-profile-changes, and --pre-install. I could reasonably make --pref, --browser-console, and --start-url work, but I think that would be bonus. Is there an existing way to flag incompatible options, and if not anything I should consider while implementing?
  • I've added some text explaining this is an advanced feature to the option description. Do you consider this enough?

@kewisch kewisch requested a review from kumar303 October 27, 2017 15:37
@kumar303 kumar303 changed the title WiP - Allow connecting to an existing Firefox instance feat: WiP - Allow connecting to an existing Firefox instance Oct 29, 2017
@kumar303
Copy link
Contributor

I think it would be good to change/adapt this to react when the connection to the devtools server is interrupted. I'm not yet sure the event for this is exposed, but I would investigate. This way if you close the Firefox you've connected to it will properly disconnect. Does this sound reasonable?

Sounds reasonable but I don't know if it's possible. If it's complicated, don't worry about it.

@kumar303
Copy link
Contributor

A feature I should add is that if e.g. Ctrl+C is pressed, the last action before the connection to the devtools server is interrupted is to uninstall the temporary add-on. The code for this would have to live in the mock kill function and would be asynchronous instead. I'd likely have to adapt the FirefoxDesktopExtensionRunner::exit function to take care of the asynchronous shutdown. Does this sound right?

I think you already added that, right?

@kumar303
Copy link
Contributor

Is there an existing way to flag incompatible options, and if not anything I should consider while implementing?

Just throw a WebExtError

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 don't fully understand why you need this feature. I left a comment on the issue asking for some clarifications on the use case.

However, I suppose we could accept the patch. I'm still thinking on it. The downsides are that there are a lot of things that can go wrong which may be hard for the user to fix. For example, the other Firefox will prompt them to accept the debugger connection but this prompt gets hidden easily. Also, it seems like a lot of features will not work in this mode.

That said, if you can make a solid case for it then it's a pretty simple patch. It will need automated tests and the usual cleanup (code needs to be indented two spaces, etc).

});
// TODO mock running info. Turn into something real or allow
// FirefoxProcess to be null
this.runningInfo = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks pretty good. We might want to rename kill in the interface to finishSession or something but that's minor.

Did you not need to add firefox.stderr and firefox.stdout? It looks like we'll need that. If so, you could just set each to an event emitter that does nothing so that at least web-ext can bind to them.

@@ -341,6 +341,15 @@ Example: $0 --help run.
demand: false,
type: 'string',
},
'firefox-port': {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name should make it more obvious that you are connecting to a remote Firefox instance. Maybe remote-firefox-port.

'firefox-port': {
alias: 'P',
describe: 'Attach to a running Firefox on this developer tools ' +
'port. To do so, open the developer toolbar and enter ' +
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 also show these instructions on the console in case a connection cannot be made

@@ -341,6 +341,15 @@ Example: $0 --help run.
demand: false,
type: 'string',
},
'firefox-port': {
alias: 'P',
describe: 'Attach to a running Firefox on this developer tools ' +
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 explain things more from the perspective of what web-ext will do. Maybe something like "Connect to this debugger port on a running Firefox instance to install the browser extension."

@rpl
Copy link
Member

rpl commented Jun 6, 2022

Closing as "status: do not merge", the related issue is still technically not covered by web-ext but this PR has been branched out quite some time ago and so it may be easier to recreate it from scratch by cherry picking the proposed changes from this PR than rebasing this branch.

@rpl rpl closed this Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants