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

[rb] remove unnecessary code from Firefox::Profile class #7376

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

titusfortner
Copy link
Member

Ways that Mozilla supports customizing these things are a little varied.

To point to an existing profile:

{"args": ["-profile", "/path/to/your/profile"]}

To only specify prefs:

{"prefs": {"foo": "bar"}}

To only specify extensions, we have an endpoint:

/session/{session id}/addon/install

(I requested that we be able to add extensions independently as part of options without needing an endpoint, but haven't seen progress: mozilla/geckodriver#1476)

So the only way to set certificates at all, or extensions before initialization is with profile:

{"profile": Base64_ENCODED_ZIP}

Firefox::Profile should only need to generate a default profile and allow for

  1. Custom certificates
  2. Adding Extensions
  3. Specifying preferences (actually, I'm not certain if profile will override prefs or if they merge?)

So... This PR.

  1. Pulls out the extra prefs we were adding for legacy driver
  2. Removes legacy driver preferences from class

I'm not sure the best way to go about testing this in an automated fashion. I think someone needs to sit down with extensions and preferences and certificates to make sure everything does what we expect for all the different methods outlined above, and I'm not currently volunteering for that. :)

Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

I agree most of Firefox::Profile code is being gradually replaced by Firefox::Options so I'm fine with the changes. I'm also not volunteering to do the proper testing of this, so I'll leave that and the decision to merge this to you :)

@titusfortner titusfortner force-pushed the firefox_profile branch 2 times, most recently from d5db5af to 590b20d Compare July 17, 2019 20:06
@titusfortner
Copy link
Member Author

Since I don't think the current code actually works the way it appears, I'm going to go ahead and merge this. I've created an issue to track the need to validate the correct behavior and that this code properly implements it. I'm creating a separate issue to track that part of it.

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.

2 participants