Skip to content
This repository has been archived by the owner on Jan 4, 2019. It is now read-only.

Add API method: electron.app.getCountryName #647

Merged
merged 4 commits into from
Sep 4, 2018
Merged

Conversation

bsclifton
Copy link
Member

To be used to properly solve brave/browser-laptop#14647

  • Was unable to use the existing electron.app.getLocale method because it would return the language (not the country)
  • I also looked at existing ICU methods and methods in the l10n/i18n helpers in Chromium's base namespace, but those all returned empty string for locale (as if it wasn't initialized properly)
  • I briefly looked at node/native node modules and it looked too messy and unfruitful to go that route

This PR can be merged into C68 AND C69. ex: would be good to have a 8.0.10 and a 8.1.x build 😄

Works correctly on Windows; need to do macOS/Linux implementation

Auditors: @darkdh, @bridiver
…untry name), this returns the ISO country code
…hich also includes encoding (ex: `de_DE.UTF-8`)
@bsclifton bsclifton self-assigned this Aug 31, 2018
#elif defined(OS_LINUX)
// for more info, see:
// https://en.cppreference.com/w/cpp/locale/setlocale
setlocale(LC_ALL, "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

setLocale? That doesn't seem right

Copy link
Member Author

@bsclifton bsclifton Aug 31, 2018

Choose a reason for hiding this comment

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

According to the docs, I believe this is needed to initialize the locale for the purpose of using the C functions in locale.h. By passing "", this means to use whatever the user has configured as the locale (as opposed to what the C library defaults to or an arbitrary locale, such as es-MX)

Copy link
Member

Choose a reason for hiding this comment

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

right, but the initialization only need to run once per program startup. This will initialize it every time you call this API

// https://developer.apple.com/documentation/corefoundation/1543547-cflocalegetvalue?language=objc
// https://developer.apple.com/documentation/corefoundation/cflocalekey?language=objc
CFLocaleRef cflocale = CFLocaleCopyCurrent();
CFStringRef country_code = (CFStringRef)CFLocaleGetValue(
Copy link
Member

Choose a reason for hiding this comment

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

we need to check (!country_code) here

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// for more info, see:
// https://en.cppreference.com/w/cpp/locale/setlocale
setlocale(LC_ALL, "");
country = std::string(setlocale(LC_ALL, NULL));
Copy link
Member

Choose a reason for hiding this comment

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

you have to check the return value of setlocale, if it returns null and then you pass it to std::string. program crashes.

…ady forre-review).

- macOS checks return values before continuing; also avoids null std::string assignment
- Linux implementation was updated to save the current locale before change it to user-preferred locale (found a good example on gnu.org - see https://www.gnu.org/software/libc/manual/html_node/Setting-the-Locale.html)
- Windows version checks length before calling UTF conversion
@bsclifton
Copy link
Member Author

@bridiver @darkdh comments addressed (and tested) - ready for re-review 😄 👍

@bridiver bridiver merged commit 691257b into master Sep 4, 2018
bridiver added a commit that referenced this pull request Sep 4, 2018
Add API method: electron.app.getCountryName
@bsclifton bsclifton deleted the get-country-name branch September 4, 2018 21:14
darkdh pushed a commit that referenced this pull request Sep 7, 2018
Add API method: electron.app.getCountryName
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants