New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 835504 link

Starred by 4 users
Status: Fixed
Owner:
█ ltian@chromium.org
Last visit > 30 days ago
Closed: Jun 2018
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

Refactor TemplateUrlService code

Reported by ltian@chromium.org, Apr 21 2018 
TemplateUrlService code needs to be refactored including:

1. TemplateUrl retrieve should not based on the index
2. TemplateUrl should only cache the native pointer to access the related information.
3. Custom search engine sorting and filtering should be applied on Java side.
4. Native side should not cache the list of TemplateUrl.
Project Member

Comment 2 by bugdroid1@chromium.org, May 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f0e579c6c5decd9c46227cb2adebed78973d9c4

commit 8f0e579c6c5decd9c46227cb2adebed78973d9c4
Author: Lei Tian <ltian@chromium.org>
Date: Tue May 08 19:42:05 2018

Refactor TemplateUrlService

This CL refactors TemplateUrlService code includes:
Java side:
1. Remove all functions access TemplateUrl with index
2. TemplateUrl only caches native pointer to access the related
information.
3. Move sorting and filtering TemplateUrl list from native to Java
side into SearchEngineAdapter

Native side:
1. Adapt to Java side changes include removing all index based query
functions int template_url_service_android and add corresponding native
code for template_url_android
2. Do not cache TemplateUrl list in the native side.

Bug:  835504 
Change-Id: I2ecfa57931f453e6ebd216249cbeb519c6f07974
Reviewed-on: https://chromium-review.googlesource.com/1023123
Commit-Queue: Lei Tian <ltian@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556930}
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEngineDialogHelper.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
[add] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrl.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/java_sources.gni
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/javatests/src/org/chromium/chrome/browser/locale/DefaultSearchEngineDialogHelperTest.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/javatests/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialogTest.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/android/javatests/src/org/chromium/chrome/browser/searchwidget/SearchActivityTest.java
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/browser/BUILD.gn
[add] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/browser/search_engines/template_url_android.cc
[add] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/browser/search_engines/template_url_android.h
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/browser/search_engines/template_url_service_android.cc
[modify] https://crrev.com/8f0e579c6c5decd9c46227cb2adebed78973d9c4/chrome/browser/search_engines/template_url_service_android.h
Project Member

Comment 3 by bugdroid1@chromium.org, May 9 2018 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/1390428f768483877d4bee86f4d1fc6fb5c9ec33

commit 1390428f768483877d4bee86f4d1fc6fb5c9ec33
Author: Lei Tian <ltian@google.com>
Date: Wed May 09 01:10:10 2018
Project Member

Comment 4 by bugdroid1@chromium.org, May 14 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0fc303121c5184489cc5ebb6faf125fb0b139222

commit 0fc303121c5184489cc5ebb6faf125fb0b139222
Author: Lei Tian <ltian@chromium.org>
Date: Mon May 14 21:05:56 2018

Refactor TemplateUrl code related to locale

This CL refactor TemplateUrl code related to locale includes:
1. Change all SpecialLocaleHandler to LocaleTemplateUrlLoader to be more
clear.
2. Remove getSearchEngineType from LocaleManager. TemplateUrlService
should be able to get the type directly.

Bug:  835504 
Change-Id: I16b6667371673faa749ed96ab06efb16e18556fe
Reviewed-on: https://chromium-review.googlesource.com/1043486
Commit-Queue: Lei Tian <ltian@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558457}
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
[add] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleTemplateUrlLoader.java
[delete] https://crrev.com/59d2b8a296efacdbb6c534f6942f05a40bdb16bb/chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/android/java_sources.gni
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/BUILD.gn
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/android/locale/locale_manager.cc
[rename] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/android/locale/locale_template_url_loader.cc
[rename] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/android/locale/locale_template_url_loader.h
[rename] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/android/locale/locale_template_url_loader_unittest.cc
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/search_engines/template_url_service_android.cc
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/browser/search_engines/template_url_service_android.h
[modify] https://crrev.com/0fc303121c5184489cc5ebb6faf125fb0b139222/chrome/test/BUILD.gn
Project Member

Comment 5 by bugdroid1@chromium.org, May 14 2018 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/dda312c828b6d751d2ec81001f49ca919bc395f7

commit dda312c828b6d751d2ec81001f49ca919bc395f7
Author: Lei Tian <ltian@google.com>
Date: Mon May 14 21:45:28 2018

Comment 6 by ltian@chromium.org, Jun 8 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment