|
|
DescriptionImplement native functions to query custom search engines for Android
1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document.
2. Add native function to retrieve recent visited search engines.
3. Allow recent visited search engine to be selected as default search engine.
BUG=348360
Committed: https://crrev.com/aa36cbaa608461565b798d847494d9be25cd15c8
Cr-Commit-Position: refs/heads/master@{#419569}
Patch Set 1 #
Total comments: 8
Patch Set 2 : delete Java file change because it is only related to frontend #Patch Set 3 : merge native functions to return generic template url and let Java side to decide which list it sho… #
Total comments: 8
Patch Set 4 : update based on ian's comments #
Total comments: 4
Patch Set 5 : run "git cl format" to format the commit #
Dependent Patchsets: Messages
Total messages: 30 (12 generated)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ltian@chromium.org changed reviewers: + ianwen@chromium.org, tedchoc@chromium.org
Description was changed from ========== 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document 2. Add native function to retrieve recent visited search engines 3. Allow recent visited search engine to be selected as default search engine BUG=348360 ========== to ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document 2. Add native function to retrieve recent visited search engines 3. Allow recent visited search engine to be selected as default search engine BUG=348360 ==========
Description was changed from ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document 2. Add native function to retrieve recent visited search engines 3. Allow recent visited search engine to be selected as default search engine BUG=348360 ========== to ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document. 2. Add native function to retrieve recent visited search engines. 3. Allow recent visited search engine to be selected as default search engine. BUG=348360 ==========
ltian@chromium.org changed reviewers: - tedchoc@chromium.org
https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:125: //search engine to adapt consistent design with desktop. Remove this comment? In a jni bridge, function description should be written in java. Also I think the code below is self descriptive enough that you don't need to add the comment. https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:141: TemplateUrlServiceAndroid::GetRecentTemplateUrlAt( How would you use this method in Java? How would the java side know which index in the GetTemplateURLs() is recent template urls? I think what you need to do is change the method above to getTemplateUrlsAt(), and in TemplateUrlService.java, you can create two java list that store the prepopulated engines and recent engines. This means both types of engines will be get from the same method, and it's your job to distinguish them in java. https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.h:42: base::android::ScopedJavaLocalRef<jobject> GetRecentTemplateUrlAt( I would remove this function. Reason stated below. https://codereview.chromium.org/2349473002/diff/1/chrome/browser/ui/tab_helpe... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2349473002/diff/1/chrome/browser/ui/tab_helpe... chrome/browser/ui/tab_helpers.cc:49: #include "chrome/browser/ui/search_engines/search_engine_tab_helper.h" Move this into the ANDROID_JAVA_UI part down below.
https://codereview.chromium.org/2349473002/diff/1/chrome/browser/ui/tab_helpe... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2349473002/diff/1/chrome/browser/ui/tab_helpe... chrome/browser/ui/tab_helpers.cc:49: #include "chrome/browser/ui/search_engines/search_engine_tab_helper.h" On 2016/09/15 22:26:03, Ian Wen wrote: > Move this into the ANDROID_JAVA_UI part down below. Ah nvm. It should be here.
BTW I would rename the CL to something like "Enable web-fetched search engines on Android". This is not strictly "custom" engine, isn't it?
ltian@chromium.org changed reviewers: + pkasting@chromium.org
I got added as a reviewer with no comment. Please clarify what you're looking for me to review: OWNER for chrome/browser/ui/? If so, LGTM on that.
On 2016/09/15 22:50:50, Peter Kasting wrote: > I got added as a reviewer with no comment. Please clarify what you're looking > for me to review: OWNER for chrome/browser/ui/? If so, LGTM on that. Sorry for not ping you first. And yes I find that you are the owner of some code in my changelist and also had a meeting with you asking for guidance about it last week, so I feel it might be appropriate to add you as reviewer.
On 2016/09/15 23:16:52, ltian wrote: > On 2016/09/15 22:50:50, Peter Kasting wrote: > > I got added as a reviewer with no comment. Please clarify what you're looking > > for me to review: OWNER for chrome/browser/ui/? If so, LGTM on that. > > Sorry for not ping you first. And yes I find that you are the owner of some code > in my changelist and also had a meeting with you asking for guidance about it > last week, so I feel it might be appropriate to add you as reviewer. There's never a problem with adding someone as a reviewer, and no need to ask first, just always say explicitly what you want each person in a multi-person review to do, or anyone brought on as an additional reviewer later.
https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:125: //search engine to adapt consistent design with desktop. On 2016/09/15 22:26:03, Ian Wen wrote: > Remove this comment? > > In a jni bridge, function description should be written in java. Also I think > the code below is self descriptive enough that you don't need to add the > comment. Done. https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.cc:141: TemplateUrlServiceAndroid::GetRecentTemplateUrlAt( On 2016/09/15 22:26:03, Ian Wen wrote: > How would you use this method in Java? How would the java side know which index > in the GetTemplateURLs() is recent template urls? > > I think what you need to do is change the method above to getTemplateUrlsAt(), > and in TemplateUrlService.java, you can create two java list that store the > prepopulated engines and recent engines. This means both types of engines will > be get from the same method, and it's your job to distinguish them in java. That makes sense. I will change that part. https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2349473002/diff/1/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_android.h:42: base::android::ScopedJavaLocalRef<jobject> GetRecentTemplateUrlAt( On 2016/09/15 22:26:03, Ian Wen wrote: > I would remove this function. Reason stated below. Done.
https://chromiumcodereview.appspot.com/2349473002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://chromiumcodereview.appspot.com/2349473002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:50: private boolean mIsDefault; s/mIsDefault/mIsPrepopulated https://chromiumcodereview.appspot.com/2349473002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:133: TemplateUrl templateUrl = nativeGetTemplateUrlAt( Have a boolean guard so that you don't create regression for trunk. https://chromiumcodereview.appspot.com/2349473002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:312: long nativeTemplateUrlServiceAndroid, int i); Can you squash the two lines into one line? https://chromiumcodereview.appspot.com/2349473002/diff/40001/chrome/browser/s... File chrome/browser/search_engines/template_url_service_android.cc (right): https://chromiumcodereview.appspot.com/2349473002/diff/40001/chrome/browser/s... chrome/browser/search_engines/template_url_service_android.cc:130: return Java_TemplateUrl_create( You could say return Java_templateUrl_create(env, index, ..., template_url->show_in_default_list()), and get rid of the if/else. BTW, even if you use the if/else, you should wrap them in brackets.
https://codereview.chromium.org/2349473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2349473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:50: private boolean mIsDefault; On 2016/09/16 20:43:04, Ian Wen wrote: > s/mIsDefault/mIsPrepopulated Done. https://codereview.chromium.org/2349473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:133: TemplateUrl templateUrl = nativeGetTemplateUrlAt( On 2016/09/16 20:43:04, Ian Wen wrote: > Have a boolean guard so that you don't create regression for trunk. Done. https://codereview.chromium.org/2349473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:312: long nativeTemplateUrlServiceAndroid, int i); On 2016/09/16 20:43:04, Ian Wen wrote: > Can you squash the two lines into one line? Done. https://codereview.chromium.org/2349473002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2349473002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:130: return Java_TemplateUrl_create( On 2016/09/16 20:43:04, Ian Wen wrote: > You could say return Java_templateUrl_create(env, index, ..., > template_url->show_in_default_list()), and get rid of the if/else. > > BTW, even if you use the if/else, you should wrap them in brackets. Done.
https://codereview.chromium.org/2349473002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2349473002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:54: int id, String shortName, boolean isPrepopulated) { nit: move #54 to #53 (if possible) https://codereview.chromium.org/2349473002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2349473002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:129: return Java_TemplateUrl_create( I think you don't need to check the policy anymore. If you want to be extra safe, you might check extension and filter them out. However on Android this is not feasible. IMO, in #132 you only need to check IsPrepopulatedTemplate(). Also, #129 to #132 are not properly formatted. Try to run git cl format before submitting.
https://codereview.chromium.org/2349473002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2349473002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:54: int id, String shortName, boolean isPrepopulated) { On 2016/09/16 21:54:06, Ian Wen wrote: > nit: move #54 to #53 (if possible) Done. https://codereview.chromium.org/2349473002/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2349473002/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:129: return Java_TemplateUrl_create( On 2016/09/16 21:54:06, Ian Wen wrote: > I think you don't need to check the policy anymore. If you want to be extra > safe, you might check extension and filter them out. However on Android this is > not feasible. > > IMO, in #132 you only need to check IsPrepopulatedTemplate(). > > Also, #129 to #132 are not properly formatted. Try to run git cl format before > submitting. Done.
ianwen@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm. Passing it to dfalcantara@ for owner stamp.
lgtm
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2349473002/#ps80001 (title: "run "git cl format" to format the commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document. 2. Add native function to retrieve recent visited search engines. 3. Allow recent visited search engine to be selected as default search engine. BUG=348360 ========== to ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document. 2. Add native function to retrieve recent visited search engines. 3. Allow recent visited search engine to be selected as default search engine. BUG=348360 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document. 2. Add native function to retrieve recent visited search engines. 3. Allow recent visited search engine to be selected as default search engine. BUG=348360 ========== to ========== Implement native functions to query custom search engines for Android 1. Enable SearchEngineTabHelper on Android to auto-detect and fetch OpenSearch description document. 2. Add native function to retrieve recent visited search engines. 3. Allow recent visited search engine to be selected as default search engine. BUG=348360 Committed: https://crrev.com/aa36cbaa608461565b798d847494d9be25cd15c8 Cr-Commit-Position: refs/heads/master@{#419569} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/aa36cbaa608461565b798d847494d9be25cd15c8 Cr-Commit-Position: refs/heads/master@{#419569} |