Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(171)

Issue 628263004: [Android] Simplify logic for grabbing initial country-code. (Closed)

Created:
6 years, 2 months ago by Yaron
Modified:
6 years, 2 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

[Android] Simplify logic for grabbing initial country-code. The old way involved start-up code reading a value and setting a global in template_url_prepopulate_data. Since the value can be computed on-demand from device-information, just fetch it as needed instead. BUG=169106 TBR=thakis Committed: https://crrev.com/38b46138112e7077a41ab8dd4815a22a15ff0527 Cr-Commit-Position: refs/heads/master@{#300317}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : rebase #

Total comments: 14

Patch Set 4 : #

Patch Set 5 : #

Total comments: 4

Patch Set 6 : #

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -71 lines) Patch
M android_webview/browser/aw_browser_main_parts.cc View 1 2 3 4 5 3 chunks +2 lines, -2 lines 0 comments Download
M android_webview/browser/aw_content_browser_client.cc View 1 2 3 4 5 4 chunks +3 lines, -3 lines 0 comments Download
M android_webview/browser/aw_form_database_service.cc View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M android_webview/native/aw_contents.cc View 1 2 3 4 5 3 chunks +2 lines, -2 lines 0 comments Download
M base/BUILD.gn View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M base/android/base_jni_registrar.cc View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download
M base/android/java/src/org/chromium/base/BaseSwitches.java View 1 chunk +3 lines, -0 lines 0 comments Download
M base/android/java/src/org/chromium/base/LocaleUtils.java View 1 2 3 4 5 2 chunks +14 lines, -0 lines 0 comments Download
A base/android/locale_utils.h View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
A base/android/locale_utils.cc View 1 2 3 4 5 1 chunk +31 lines, -0 lines 0 comments Download
M base/base.gyp View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/android/shell/chrome_main_delegate_chrome_shell_android.cc View 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/android/sync_shell/chrome_main_delegate_chrome_sync_shell_android.cc View 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/search_engines/search_provider_install_data_unittest.cc View 1 2 3 4 5 2 chunks +0 lines, -5 lines 0 comments Download
M components/omnibox/autocomplete_result_unittest.cc View 1 2 3 4 5 2 chunks +0 lines, -5 lines 0 comments Download
M components/search_engines/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M components/search_engines/template_url_prepopulate_data.h View 1 chunk +0 lines, -6 lines 0 comments Download
M components/search_engines/template_url_prepopulate_data.cc View 1 2 3 4 5 3 chunks +8 lines, -16 lines 0 comments Download
M testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
M ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java View 1 2 3 4 5 2 chunks +0 lines, -11 lines 0 comments Download
M ui/base/l10n/l10n_util.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M ui/base/l10n/l10n_util_android.h View 1 2 3 4 5 1 chunk +0 lines, -3 lines 0 comments Download
M ui/base/l10n/l10n_util_android.cc View 1 2 3 4 5 1 chunk +0 lines, -7 lines 0 comments Download

Messages

Total messages: 24 (5 generated)
Yaron
jshin: ui/base/l10n/l10n_util_android.cc ui/base/l10n/l10n_util_android.h pkasting: chrome/browser/search_engines/search_provider_install_data_unittest.cc components/omnibox/autocomplete_result_unittest.cc components/search_engines/DEPS components/search_engines/template_url_prepopulate_data.cc components/search_engines/template_url_prepopulate_data.h newt: ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java
6 years, 2 months ago (2014-10-06 18:21:30 UTC) #2
Yaron
oops. looks like i need to rebase.
6 years, 2 months ago (2014-10-06 18:22:55 UTC) #3
newt (away)
https://codereview.chromium.org/628263004/diff/40001/ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java File ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java (right): https://codereview.chromium.org/628263004/diff/40001/ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java#newcode65 ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java:65: context.getResources().getConfiguration().locale.getCountry(); Is context.getResources().getConfiguration().locale the same as Locale.getDefault()? (I'd hope ...
6 years, 2 months ago (2014-10-06 18:34:03 UTC) #4
newt (away)
https://codereview.chromium.org/628263004/diff/40001/components/search_engines/template_url_prepopulate_data.cc File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/628263004/diff/40001/components/search_engines/template_url_prepopulate_data.cc#newcode629 components/search_engines/template_url_prepopulate_data.cc:629: const std::string& country_code = l10n_util::GetDefaultCountryCode(); Is it worth caching ...
6 years, 2 months ago (2014-10-06 18:36:48 UTC) #5
Peter Kasting
LGTM https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_engines/search_provider_install_data_unittest.cc File chrome/browser/search_engines/search_provider_install_data_unittest.cc (right): https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_engines/search_provider_install_data_unittest.cc#newcode20 chrome/browser/search_engines/search_provider_install_data_unittest.cc:20: #include "components/search_engines/template_url_prepopulate_data.h" This #include can probably be removed. ...
6 years, 2 months ago (2014-10-06 20:28:27 UTC) #6
Yaron
+sky to approve the DEP on ui/base https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_engines/search_provider_install_data_unittest.cc File chrome/browser/search_engines/search_provider_install_data_unittest.cc (right): https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_engines/search_provider_install_data_unittest.cc#newcode20 chrome/browser/search_engines/search_provider_install_data_unittest.cc:20: #include "components/search_engines/template_url_prepopulate_data.h" ...
6 years, 2 months ago (2014-10-08 00:08:54 UTC) #8
Peter Kasting
https://codereview.chromium.org/628263004/diff/40001/components/search_engines/template_url_prepopulate_data.cc File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/628263004/diff/40001/components/search_engines/template_url_prepopulate_data.cc#newcode631 components/search_engines/template_url_prepopulate_data.cc:631: DLOG(ERROR) << "Invalid country code: " << country_code; On ...
6 years, 2 months ago (2014-10-08 00:23:08 UTC) #9
newt (away)
lgtm
6 years, 2 months ago (2014-10-08 00:31:07 UTC) #10
sky
Don't you need to update gyp/gn too? Presumably they aren't linking against ui. https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_android.h File ...
6 years, 2 months ago (2014-10-08 03:08:03 UTC) #11
Yaron
https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_android.h File ui/base/l10n/l10n_util_android.h (right): https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_android.h#newcode18 ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string GetDefaultLocale(); On 2014/10/08 03:08:03, sky wrote: > ...
6 years, 2 months ago (2014-10-09 18:09:19 UTC) #12
sky
That seems better to me. On Thu, Oct 9, 2014 at 11:09 AM, <yfriedman@chromium.org> wrote: ...
6 years, 2 months ago (2014-10-10 02:51:08 UTC) #13
jungshik at Google
https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_android.h File ui/base/l10n/l10n_util_android.h (right): https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_android.h#newcode18 ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string GetDefaultLocale(); On 2014/10/09 18:09:19, Yaron wrote: > ...
6 years, 2 months ago (2014-10-13 21:46:18 UTC) #14
Yaron
boliu: aw https://codereview.chromium.org/628263004/diff/40001/components/search_engines/template_url_prepopulate_data.cc File components/search_engines/template_url_prepopulate_data.cc (right): https://codereview.chromium.org/628263004/diff/40001/components/search_engines/template_url_prepopulate_data.cc#newcode631 components/search_engines/template_url_prepopulate_data.cc:631: DLOG(ERROR) << "Invalid country code: " << ...
6 years, 2 months ago (2014-10-17 18:24:00 UTC) #16
boliu
android_webview lgtm
6 years, 2 months ago (2014-10-17 19:41:27 UTC) #17
sky
ui LGTM
6 years, 2 months ago (2014-10-20 16:13:37 UTC) #18
Yaron
tbr thakis for trivial base gyp/gn changes
6 years, 2 months ago (2014-10-20 18:00:35 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628263004/150001
6 years, 2 months ago (2014-10-20 18:01:56 UTC) #22
commit-bot: I haz the power
Committed patchset #7 (id:150001)
6 years, 2 months ago (2014-10-20 19:40:28 UTC) #23
commit-bot: I haz the power
6 years, 2 months ago (2014-10-20 19:41:05 UTC) #24
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/38b46138112e7077a41ab8dd4815a22a15ff0527
Cr-Commit-Position: refs/heads/master@{#300317}

Powered by Google App Engine
This is Rietveld 408576698