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

fix: correct Chrome path on Windows #3

Closed

Conversation

@JoeDoyle23
Copy link

@JoeDoyle23 JoeDoyle23 commented Aug 13, 2013

Checks for Chrome in the default location on windows taking into account
32 and 64 bit machines. Falls back to the version in the user's
profile.

Closes #2

Checks for Chrome in the default location on windows taking into account
32 and 64 bit machines.  Falls back to the version in the user's
profile.

Closes #2
},
ENV_CMD: 'CHROME_CANARY_BIN'
};

ChromeCanaryBrowser.$inject = ['baseBrowserDecorator', 'args'];

function windowsChromePath(chromeExe) {

This comment has been minimized.

@vojtajina

vojtajina Aug 17, 2013
Contributor

Make it so that this is only executed if you are on windows.

Now, there are two fs.existsSync even if you are on Mac/Linux.

@vojtajina
Copy link
Contributor

@vojtajina vojtajina commented Aug 17, 2013

Also, can you please change the commit msg to just fix: correct Chrome path on Windows.

if(os.platform()!=='win32')
return '';

var globalInstall = os.arch()==='x64' ? process.env['ProgramFiles(x86)'] : process.env.ProgramFiles;

This comment has been minimized.

@mgol

mgol Sep 9, 2013
Contributor

This is wrong... You load 32-bit Chrome on a 64-bit OS nad vice-versa. But even if you swapped it, it would still be wrong as Chrome is currently 32-bit even on 64-bit Windows. My approach is more future-proof: #6.

cc @vojtajina

This comment has been minimized.

@JoeDoyle23

JoeDoyle23 Sep 9, 2013
Author

I think you might be misreading this. This code loads Chrome from the correct location depending on the bitness of the OS. Since there isn't a 64bit version of Chrome (and there may never be) I'm not too worried about not handling that scenario. We're using this code on several machines without issue.

This comment has been minimized.

@mgol

mgol Sep 10, 2013
Contributor

@JoeDoyle23 Ahh, you're right, I've read it wrong. I still think it's better to be future-proof, especially that it doesn't have a lot of overhead but I'm sorry for incorrectly claiming your solution is wrong.

This comment has been minimized.

@mgol

mgol Nov 13, 2013
Contributor

@JoeDoyle23 Surely some day there will be a 64-bit Chrome version, the only question is when.

@JoeDoyle23
Copy link
Author

@JoeDoyle23 JoeDoyle23 commented Nov 13, 2013

Was there something else that you wanted me to change before accepting this?

@vojtajina
Copy link
Contributor

@vojtajina vojtajina commented Nov 22, 2013

@JoeDoyle23 Thanks a bunch! I corrected some styling issues and squashed both commits. It's merged as 9ebd997.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants