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

update toMs function. fix #4894 #8495

Merged
merged 2 commits into from
Oct 24, 2018
Merged

update toMs function. fix #4894 #8495

merged 2 commits into from
Oct 24, 2018

Conversation

phablulo
Copy link
Contributor

@phablulo phablulo commented Jul 16, 2018

Old versions of Chromium (below 61.0.3163.100) formats floating pointer numbers in a locale-dependent way, using a comma instead of a dot.
This PR makes toMs function parse it correctly, preventing unexpected behaviors.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

so it can parse floating numbers with commas instead of dots
@posva
Copy link
Member

posva commented Jul 16, 2018

hmm, this bug was fixed in Chromium a while ago (https://bugs.chromium.org/p/chromium/issues/detail?id=720222). What is the current version used by Electron?
BTW, this needs a small comment saying a bug in old versions of chromium used by electron uses a comma version (locale dependent) instead of a dot

@phablulo
Copy link
Contributor Author

phablulo commented Jul 16, 2018

Electron 2.0.5 (current latest) uses Chromium 61.0.3163.100, the bug seems to be fixed on version 68.0.3409.0

I'll add the comment

@posva
Copy link
Member

posva commented Jul 16, 2018

oh lol, so it wasn't fixed as long ago as I thought it was

@phablulo
Copy link
Contributor Author

comment fixed (:

@posva
Copy link
Member

posva commented Jul 16, 2018

I meant in the code 😆

@phablulo
Copy link
Contributor Author

lol, sorry!
I fixed just commented the code

@Japangeek
Copy link

That would be awesome if this was merged. I'm currently having this issues with Electron on Mac too. (However, in Windows works fine)

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.

4 participants