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

61 add support for custom browser path #62

Merged

Conversation

SnoCold
Copy link
Collaborator

@SnoCold SnoCold commented Apr 12, 2021

Fixes #61

Summary

  • Added a 'BuildOption' to supply 'binaryPath' of the browser to be used.
  • Added unit tests for checking this feature

Other Notes

This pull request addresses the feature of using a chromium/gecko engine based browser, alternative to the currently prevalent Chrome/Firefox. The feature works as long as the browser engines are the same as the Original (Chrome/Firefox) browsers.

Copy link
Member

@ebebbington ebebbington left a comment

Choose a reason for hiding this comment

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

2 things:

  1. Have you tested this with your Brave browser? I'm not confident we should say we support other chromium based browsers because we don't actually test them (yet), so it's unsure if they will actually work - though I might do this in a separate PR to avoid the headache for you (unless you want to), i think it's just a matter of installing brave on the CI workflow and in the dockerfile, then an extra integration test that checks it works when passing in the path to brave

Though, saying that, maybe it isn't needed, maybe the fact we offer the option to use a different path is enough?

  1. Ill need to write the docs for this before we can release this feature, or you can as well if you feel like it and are familiar with the frontend/vue :)

@ebebbington ebebbington linked an issue Apr 13, 2021 that may be closed by this pull request
4 tasks
@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 13, 2021

2 things:

1. Have you tested this with your Brave browser? I'm not confident we should say we support other chromium based browsers because we don't actually test them (yet), so it's unsure if they will actually work - though I might do this in a separate PR to avoid the headache for you (unless you want to), i think it's just a matter of installing brave on the CI workflow and in the dockerfile, then an extra integration test that checks it works when passing in the path to brave

Though, saying that, maybe it isn't needed, maybe the fact we offer the option to use a different path is enough?

1. Ill need to write the docs for this before we can release this feature, or you can as well if you feel like it and are familiar with the frontend/vue :)
  1. Yes I have tested this with Brave Browser. It works seamlessly. Yes we could add Brave browser in the CI flow, but that is going to pile up as and when a new chromium browser comes out. Maybe if someone faces specific issues pertaining their chromiums, then can log an issue (which they most likely won't as they won't be modifying the engine so much that it isn't chromium anymore.)

  2. I am not really familiar with vue. But I would surely like to try, if it is ok with all. I will definitely put it down if really cant get it.

@ebebbington
Copy link
Member

Yes I have tested this with Brave Browser. It works seamlessly.

That is perfect :D That is really great news that it just works, i know you mentioned it should just work anyway, but it's nice to see it has done :)

I do see your your point bout point 1 though, I think us saying "We also support other browsers like Brave, you can set the path to do this" is enough

I am not really familiar with vue. But I would surely like to try, if it is ok with all. I will definitely put it down if really cant get it.

Feel free :) It's not a problem if not, we can discuss it on the discord

@ebebbington ebebbington merged commit f7ae095 into drashland:master Apr 14, 2021
@ebebbington ebebbington added the Type: Minor Merging this pull request results in a minor version increment label Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Minor Merging this pull request results in a minor version increment
Development

Successfully merging this pull request may close these issues.

Support for adding path to alternative browser Specify Path of Chrome To Run?
2 participants