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

Now working on Windows 10 latest Chrome #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Now working on Windows 10 latest Chrome #3

wants to merge 1 commit into from

Conversation

ww9
Copy link

@ww9 ww9 commented Nov 5, 2018

Windows 10, Google Chrome 70.0.3538.77

took 2 minutes to make it work with my rusty python ¯\_(ツ)_/¯

@ww9 ww9 changed the title Now working on Windows 10 latest chrome Now working on Windows 10 latest Chrome Nov 5, 2018
Copy link

@aslafy-z aslafy-z left a comment

Choose a reason for hiding this comment

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

Get chrome path from registry for Windows

CHROME_CMD = "chrome.exe"
USER_DATA_DIR = r"%LOCALAPPDATA%\Google\Chrome\User Data"
elif sys.platform.startswith("win"):
CHROME_CMD = "\"C:\\Program Files (x86)\\Google\\Chrome\Application\\chrome.exe\""
Copy link

Choose a reason for hiding this comment

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

I think you should use this kind of discovery method to ensure the path is good:
https://stackoverflow.com/a/44588190

Copy link
Author

Choose a reason for hiding this comment

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

Completely agree, hardcoded path is bad. However I'm short in time and just wanted to get it working on my pretty standard Windows 10 installation. There's also the potential problem of having to escalate privileges to fetch this registry. But don't quote me on that one.

This patch also fixes other problems when running on Windows so I was hopping for it to land and be further improved upon by someone else with a bit more time.

@ww9 ww9 mentioned this pull request Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants