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

Issue 2398533002: Implement search engine auto switch when entering or leaving some locale (Closed)

Created:
4 years, 2 months ago by Ian Wen
Modified:
4 years, 2 months ago
Reviewers:
Ilya Sherman, Maria
CC:
chromium-reviews, asvitkine+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement search engine auto switch when entering or leaving some locale Search engine auto switch is a feature that Chrome sets the default search engine automatically when it detects changes to the user's locale. This CL: 1. Enables Chrome to check locale info in onResume(). 2. Shows a snackbar if such auto switch is made. To prevent memory leak, LocaleMangaer only keeps a weak ref to the snackbar manager, which is only owned by the ChromeActivity. Also since onStartWithNative() might happen before snackbar manager is ready, locale manager initialization is now moved to onResumeWithNative() to guarantee that snackbar manager will be ready when we detect locale changes. BUG=638062 R=isherman@chromium.org, mariakhomenko@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/9b5bdfbaf2dc936913995b772b2dc419604297ab

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -13 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java View 5 chunks +8 lines, -4 lines 1 comment Download
M chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java View 3 chunks +96 lines, -8 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/locale/SearchEnginePromoDialog.java View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java View 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java View 3 chunks +11 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java View 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/android/locale/special_locale_handler.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/locale/special_locale_handler.cc View 1 chunk +19 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +1 line, -0 lines 1 comment Download

Depends on Patchset:

Messages

Total messages: 14 (7 generated)
Ian Wen
PTAL :)
4 years, 2 months ago (2016-10-05 04:21:10 UTC) #2
Ilya Sherman
histograms.xml lgtm https://codereview.chromium.org/2398533002/diff/1/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2398533002/diff/1/tools/metrics/histograms/histograms.xml#newcode97036 tools/metrics/histograms/histograms.xml:97036: + <int value="14" label="SPECIAL_LOCALE"/> This feels a ...
4 years, 2 months ago (2016-10-05 06:00:02 UTC) #3
Maria
Overall looks good, just one question https://codereview.chromium.org/2398533002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2398533002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java#newcode379 chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:379: mLocaleManager.setSnackbarManager(getSnackbarManager()); Your comment ...
4 years, 2 months ago (2016-10-05 17:36:40 UTC) #4
Ian Wen
On 2016/10/05 17:36:40, Maria wrote: > Overall looks good, just one question > > https://codereview.chromium.org/2398533002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java ...
4 years, 2 months ago (2016-10-05 18:12:18 UTC) #6
Maria
lgtm
4 years, 2 months ago (2016-10-05 18:15:17 UTC) #7
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/9b5bdfbaf2dc936913995b772b2dc419604297ab Cr-Commit-Position: refs/heads/master@{#423262}
4 years, 2 months ago (2016-10-05 19:58:31 UTC) #11
Ian Wen
4 years, 2 months ago (2016-10-05 20:01:22 UTC) #13
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as
9b5bdfbaf2dc936913995b772b2dc419604297ab (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698