Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12255042:
[Android] Introduce a Java wrapper for TemplateUrlService. (Closed)
|
Created:
7 years, 10 months ago by Yaron Modified:
7 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, nyquist Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Introduce a Java wrapper for TemplateUrlService.
The idea here is to move model/native logic from the Android
downstream port into chromium and instead provide a Java API, exposing
any necessary functions for consumption by the app. This also brings the
Android code closer to other ports so it's easier for OWNERS to both
see and review changes.
Aside: includes a small change to jni generator to support generating bindings for interfaces.
BUG=169106
TBR=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184724
Patch Set 1 #Patch Set 2 : #Patch Set 3 : comments #
Total comments: 60
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : test updates #
Total comments: 4
Patch Set 7 : rebase #
Total comments: 1
Patch Set 8 : get rid of generic map conversion #
Total comments: 12
Patch Set 9 : no more map #
Total comments: 4
Patch Set 10 : id->index #
Total comments: 7
Patch Set 11 : bulach again #Messages
Total messages: 21 (0 generated)
not ready for review
bulach: base sky: search_engine/templateurlservice bits
bulach: I got this findbugs failure: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/37... I think it's a false positive since I assert that TUS.getInstance is only called on one thread. Seem reasonable to whitelist it?
sky->pkasting
https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:19: using base::android::CheckException; Nit: Avoid using statements except where they save a number of lines https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:39: JNIEnv* env, jobject obj) Nit: One arg per line https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:42: TemplateURLServiceFactory::GetForProfile(GetOriginalProfile())) { Nit: Indent 4, not 2 https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:44: content::Source<TemplateURLService>(template_url_service_)); Nit: Lines of arguments should be horizontally aligned (several places) https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:54: DCHECK(type == chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED); Nit: DCHECK_EQ https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:64: if (!template_url_service_->loaded()) Nit: Remove conditional; Load() will check this and other conditions and short-circuit internally https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:74: selected_index >= static_cast<int>(template_urls.size())) { Nit: Given that you use selected_index as a size_t below (by using []), I suggest casting it to a size_t and storing that in a temp. This should also eliminate the need to check for "< 0" since that will turn into a failure of the ">= .size()" check in most cases; if you're worried about the risk of treating a very large negative number as a valid small positive number, however, you could do the check at conversion time. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:75: DLOG(ERROR) << "Wrong index for search engine"; Nit: In general we try to avoid logging statements, since it's hard to collect logs from most users' machines; there's no rule against them, but consider whether they could just be eliminated, or else turned into a DCHECK() or similar if this case could never happen. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:91: if (template_url_service_->GetDefaultSearchProvider() == template_urls[i]) { Nit: Move this call outside the loop, since it's not dirt-cheap https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:103: TemplateUrlServiceAndroid::GetLocalizedSearchEngines(JNIEnv* env, jobject obj) { Nit: Indent 4 https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:113: ScopedJavaLocalRef<jclass> hashmap_clazz = GetClass(env, "java/util/HashMap"); Nit: clazz? https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:124: std::stringstream ss; The Google style guide bans stringstreams outside of logging code. Here you're just converting an int to a string, so consider using something from string_number_conversions.h. If you want to avoid casting the size_t to something else, you could use StringPrintf() instead with the ("%" PRIuS) argument as the format string. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:24: Nit: Why all these blank lines between each function? https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:32: GetLocalizedSearchEngines(JNIEnv* env, jobject obj); Nit: Indent 4 https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:43: JavaObjectWeakGlobalRef weak_java_obj_; Nit: Add blank line above here https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:48: DISALLOW_COPY_AND_ASSIGN(TemplateUrlServiceAndroid); Nit: And here
https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc File base/android/jni_map.cc (right): https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:17: // Converts a std::map<std::string, string16> to a java HashMap<String,String>. Nit: insert space between Java generics types. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:67: // Loop over them. Nit: Technically you don't start looping yet, you're only getting jmethodIDs. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:93: // For profiles, jvalue can be null if not found in the contacts database. Which contacts database? What does this have to do with profiles? https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h File base/android/jni_map.h (right): https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h#new... base/android/jni_map.h:19: // Convert a std::map<std::string, string16> to a Java HashMap. Mention the generics type of the Java HashMap (<String, String>) https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h#new... base/android/jni_map.h:25: BASE_EXPORT std::map<std::string, string16> ConvertJavaMapToMap(JNIEnv* env, Since you throw out both null keys and even null values, maybe mention that in this documentation? https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:29: private static TemplateUrlService sService; Nit: Insert newline https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:31: ThreadUtils.assertOnUiThread(); Maybe add to the class-level documentation that this class should only be accessed on the UI thread? Some of the methods assume that you only access it on the UI thread, whereas others are protected by synchronized blocks (such as registering/unregistering observers for the load). For example, it is unclear whether isLoaded() can be called on a different thread (in native land it inspects the internal field, so should only be called from the correct thread). https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:59: private final int mNativeTemplateUrlServiceAndroid; Move final member fields above constructor https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:64: public static final String SEARCH_ENGINE_ID = "searchEngineId"; Move these up to the top of the class, before any member methods or fields. https://codereview.chromium.org/12255042/diff/3010/chrome/android/javatests/s... File chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java (right): https://codereview.chromium.org/12255042/diff/3010/chrome/android/javatests/s... chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:38: assertFalse(TemplateUrlService.getInstance().isLoaded()); To ensure a test fails instead of crash, you must assert on the test thread instead of the UI thread. It is possible to use AtomicBoolean for the return value of isLoaded()
yaron: whitelisting the findbugs warning seem fine in this case. I have a rather different suggestion for the jni_map, I think we can probably avoid passing non-opaque maps altogether, or at least use the jni generator itself on jni_map.. more below. apologies for the dead kitten :) https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc File base/android/jni_map.cc (right): https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:21: ScopedJavaLocalRef<jclass> map_clazz = GetClass(env, "java/util/HashMap"); a kitten has died.... :D any chance of generating the JNI for this? we fully support system classes... if there's any issues with it, create a tiny wrapper in java.... this sort of boiler plate tend to be viral, and before we know it, it's spread all over the place! it'd be great to set a better example, wdyt? also, why is it necessary to pass a map to/from java? wouldn't it be simpler, and perhaps even more efficient, to just expose either a @CalledByNative or a native "setKeyValue(String, String)" wherever this is needed? I mean, from the java side, it'd call N times "native setKeyValue(String, String)" then call "doSomething()" once... from native, it'd call "Java_setKeyValue(..)" then "doSomething()"... wdyt? https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h File base/android/jni_map.h (right): https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h#new... base/android/jni_map.h:32: nit: extra \n https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:74: synchronized (mLoadListeners) { so the listeners will be potentially called on a different thread than the one they were registered? worth a comment in 26 above.. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:108: for (size_t i = 0; i < template_urls.size(); i++) { nit: ++i (and 118 too)
https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc File base/android/jni_map.cc (right): https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:17: // Converts a std::map<std::string, string16> to a java HashMap<String,String>. On 2013/02/20 07:33:09, nyquist wrote: > Nit: insert space between Java generics types. Done. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:21: ScopedJavaLocalRef<jclass> map_clazz = GetClass(env, "java/util/HashMap"); On 2013/02/20 14:41:25, bulach wrote: > a kitten has died.... :D > > any chance of generating the JNI for this? we fully support system classes... if > there's any issues with it, create a tiny wrapper in java.... this sort of > boiler plate tend to be viral, and before we know it, it's spread all over the > place! it'd be great to set a better example, wdyt? > > also, why is it necessary to pass a map to/from java? > wouldn't it be simpler, and perhaps even more efficient, to just expose either a > @CalledByNative or a native "setKeyValue(String, String)" wherever this is > needed? > > I mean, from the java side, it'd call N times "native setKeyValue(String, > String)" then call "doSomething()" once... > from native, it'd call "Java_setKeyValue(..)" then "doSomething()"... > > wdyt? That seems suboptimal to me. I feel like what you're really trying to do is pass a map of data from native to Java or vice-versa and we should support. Exposing it as a series of setKeyValue calls is getting away from having a data structure that can be marshalled along the boundary. I'd also like to avoid duplicating the data and/or having two possible owners. In the end, I stuck with a map and converted most bindings to JNI-generated ones. The one caveat is that inner-classes generated from a jar-file don't work 'cause $ is treating as an environment variable. This required a minor tweak to jni_generator to support interfaces. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:67: // Loop over them. On 2013/02/20 07:33:09, nyquist wrote: > Nit: Technically you don't start looping yet, you're only getting jmethodIDs. Dropped most comments as with bindings, the readability is improved https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.cc#ne... base/android/jni_map.cc:93: // For profiles, jvalue can be null if not found in the contacts database. On 2013/02/20 07:33:09, nyquist wrote: > Which contacts database? What does this have to do with profiles? dropped. Copied costant from the original implementation that became a shared function. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h File base/android/jni_map.h (right): https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h#new... base/android/jni_map.h:19: // Convert a std::map<std::string, string16> to a Java HashMap. On 2013/02/20 07:33:09, nyquist wrote: > Mention the generics type of the Java HashMap (<String, String>) Done. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h#new... base/android/jni_map.h:25: BASE_EXPORT std::map<std::string, string16> ConvertJavaMapToMap(JNIEnv* env, On 2013/02/20 07:33:09, nyquist wrote: > Since you throw out both null keys and even null values, maybe mention that in > this documentation? Done. https://codereview.chromium.org/12255042/diff/3010/base/android/jni_map.h#new... base/android/jni_map.h:32: On 2013/02/20 14:41:25, bulach wrote: > nit: extra \n Done. https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:29: private static TemplateUrlService sService; On 2013/02/20 07:33:09, nyquist wrote: > Nit: Insert newline Done. https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:31: ThreadUtils.assertOnUiThread(); On 2013/02/20 07:33:09, nyquist wrote: > Maybe add to the class-level documentation that this class should only be > accessed on the UI thread? > Some of the methods assume that you only access it on the UI thread, whereas > others are protected by synchronized blocks (such as registering/unregistering > observers for the load). > > For example, it is unclear whether isLoaded() can be called on a different > thread (in native land it inspects the internal field, so should only be called > from the correct thread). I decided to keep this simple and require all access form the main thread. Turns out, it was only used from there, so prefer not to add extra threading logic until needed https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:59: private final int mNativeTemplateUrlServiceAndroid; On 2013/02/20 07:33:09, nyquist wrote: > Move final member fields above constructor Done. https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:64: public static final String SEARCH_ENGINE_ID = "searchEngineId"; On 2013/02/20 07:33:09, nyquist wrote: > Move these up to the top of the class, before any member methods or fields. Done. https://codereview.chromium.org/12255042/diff/3010/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:74: synchronized (mLoadListeners) { On 2013/02/20 14:41:25, bulach wrote: > so the listeners will be potentially called on a different thread than the one > they were registered? worth a comment in 26 above.. Changed to only ui thread. https://codereview.chromium.org/12255042/diff/3010/chrome/android/javatests/s... File chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java (right): https://codereview.chromium.org/12255042/diff/3010/chrome/android/javatests/s... chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:38: assertFalse(TemplateUrlService.getInstance().isLoaded()); On 2013/02/20 07:33:09, nyquist wrote: > To ensure a test fails instead of crash, you must assert on the test thread > instead of the UI thread. It is possible to use AtomicBoolean for the return > value of isLoaded() Done except in one case which isn't really a failure of the case but a failing of the world. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:19: using base::android::CheckException; On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Avoid using statements except where they save a number of lines Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:39: JNIEnv* env, jobject obj) On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:42: TemplateURLServiceFactory::GetForProfile(GetOriginalProfile())) { On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Indent 4, not 2 Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:44: content::Source<TemplateURLService>(template_url_service_)); On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Lines of arguments should be horizontally aligned (several places) Sorry, I thought that was only for function declarations. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:54: DCHECK(type == chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED); On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: DCHECK_EQ Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:64: if (!template_url_service_->loaded()) On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Remove conditional; Load() will check this and other conditions and > short-circuit internally Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:74: selected_index >= static_cast<int>(template_urls.size())) { On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Given that you use selected_index as a size_t below (by using []), I > suggest casting it to a size_t and storing that in a temp. > > This should also eliminate the need to check for "< 0" since that will turn into > a failure of the ">= .size()" check in most cases; if you're worried about the > risk of treating a very large negative number as a valid small positive number, > however, you could do the check at conversion time. Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:75: DLOG(ERROR) << "Wrong index for search engine"; On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: In general we try to avoid logging statements, since it's hard to collect > logs from most users' machines; there's no rule against them, but consider > whether they could just be eliminated, or else turned into a DCHECK() or similar > if this case could never happen. Converted to DCHECK. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:91: if (template_url_service_->GetDefaultSearchProvider() == template_urls[i]) { On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Move this call outside the loop, since it's not dirt-cheap Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:103: TemplateUrlServiceAndroid::GetLocalizedSearchEngines(JNIEnv* env, jobject obj) { On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Indent 4 Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:108: for (size_t i = 0; i < template_urls.size(); i++) { On 2013/02/20 14:41:25, bulach wrote: > nit: ++i (and 118 too) Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:113: ScopedJavaLocalRef<jclass> hashmap_clazz = GetClass(env, "java/util/HashMap"); On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: clazz? Customary in java but your'e right that it's wrong here. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:124: std::stringstream ss; On 2013/02/20 05:31:07, Peter Kasting wrote: > The Google style guide bans stringstreams outside of logging code. > > Here you're just converting an int to a string, so consider using something from > string_number_conversions.h. If you want to avoid casting the size_t to > something else, you could use StringPrintf() instead with the ("%" PRIuS) > argument as the format string. Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:24: On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Why all these blank lines between each function? Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:32: GetLocalizedSearchEngines(JNIEnv* env, jobject obj); On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Indent 4 Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:43: JavaObjectWeakGlobalRef weak_java_obj_; On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: Add blank line above here Done. https://codereview.chromium.org/12255042/diff/3010/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.h:48: DISALLOW_COPY_AND_ASSIGN(TemplateUrlServiceAndroid); On 2013/02/20 05:31:07, Peter Kasting wrote: > Nit: And here Done.
Had to rebase my client but patchset6 should include diffs only based on comments.
https://codereview.chromium.org/12255042/diff/10003/base/android/jni_map.h File base/android/jni_map.h (right): https://codereview.chromium.org/12255042/diff/10003/base/android/jni_map.h#ne... base/android/jni_map.h:20: BASE_EXPORT ScopedJavaLocalRef<jobject> ConvertMapToJavaMap( ..ToJavaHashMap (and below) ? also, this is converting only a <String, String>, which goes back to my point: we've survived this long without passing a "transparent" map. I still think that having a "@CalledByNative / native setKeyValue()" where needed rather than allowing java objects to be inspected (and provided by base) would probably be simpler.. I mean, this is not generic enough and the next person that needs <String, Int> or whatever other combination of key-value types, would have to hack their way around in base, plus their own code, right? matching the generic in java and the template type in c++ is probably going to be more painful than ad-hoc, type safe "setKeyValues"... I appreciate the need to "marshall a map", I'm just saying we either make it more generic, or we keep it private to the places that need them... otoh, jni_array.h has a bunch of utilities methods. granted, one dimension is easier :) so if you want to specify in here "StringString" in the name of the functions, that would probably be more inline with the array methods... wdyt?
LGTM https://codereview.chromium.org/12255042/diff/17002/chrome/browser/search_eng... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/17002/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:51: DCHECK_EQ(type, chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED); Tiny nit: We normally do (expected, actual) order with DCHECK_EQ/NE (though the DCHECK itself doesn't actually care) https://codereview.chromium.org/12255042/diff/17002/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:69: size_t selected_index_size_t = size_t(selected_index); Nit: Use static_cast<>
Ok, got rid of the psuedo-generic conversions in base and added a local helper in template_url_service_android.cc https://codereview.chromium.org/12255042/diff/17002/chrome/browser/search_eng... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/17002/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:51: DCHECK_EQ(type, chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED); On 2013/02/22 20:02:33, Peter Kasting wrote: > Tiny nit: We normally do (expected, actual) order with DCHECK_EQ/NE (though the > DCHECK itself doesn't actually care) Ya, makes sense. Done. https://codereview.chromium.org/12255042/diff/17002/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:69: size_t selected_index_size_t = size_t(selected_index); On 2013/02/22 20:02:33, Peter Kasting wrote: > Nit: Use static_cast<> Done.
lgtm
lgtm, but yaron, sorry for being a pain :) I tried to explain in more details below, and some other nits... feel free to go ahead if you disagree with the snippet below, but please let me know why :) https://codereview.chromium.org/12255042/diff/14004/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/14004/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:33: * This listener will be notified when template url service is done laoding. nit: s/lao/loading/ https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:45: i->second); nit: fits previous line https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:123: prepopulated_urls_count++; nit: ++prepopulated_urls_count https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:127: base::android::GetClass(env, "java/util/HashMap"); I think there's a handy JNI_HashMap::g_HashMap_clazz https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:143: search_engine_map[kSearchEngineUrl] = template_url->url_ref().DisplayURL(); damn ocean apart... :) sorry, I think I'm not exemplifying my thoughts as clear as I should... this looks fine, but let me try to sketch out something, let me know if the following is clearer: java: class Foo { class Item { String mA; String mB; @CalledByNative("Item") create(String a, String b, String c) { ... } } native int nativeGetItemCount(); native Item nativeGetItemAt(int n); ... Item[] doSomething() { int count = nativeGetItemCount(); Item[] ret = new Item[count]; for (int i = 0; i < count; ++count) ret[i] = nativeGetItemAt(i); return ret; } } c++: jint GetCount(...) { return template_url_service_->GetTemplateURLs()->size(); } jobject GetItemAt(..., jint i) { TemplateURL* t = template_url_service_->GetTemplateURLs()[i]; if (t->prepopulate_id() <= 0) return NULL; return Java_Foo_createItem( t->short_name, t->keyword, t->url_ref().DisplayURL()); } my summary would then be: pros: + type safety guaranteed by Java_Foo_createItem, and explicitly documented in code by the "Item" class. + no need to duplicate these consts across the board + no need to know anything about java map in c++.. it'd only know about the "Item" class, which is the only thing it cares about. + less indirections / boxing: it's clear from java code that it requires an "Item" object, and it's clearer from c++ that it provides one... + if the java-side needs a map rather than Item, then let java side do this massaging / conversion, and keep c++ only using "opaque" datatypes (i.e., it doesn't know what an item is, but it has a clean and safe API to create one).. the map is too transparent here. cons: - the JNI api now contains a "getCount()" and "getItemAt()" rather than "getAllItems()".. however, as above, I think these two APIs are simpler and self-documented... would this make any sense, specially in the context of setting this as a pattern for other classes in the future? wdyt? :) https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:149: prepopulated_url_index++; nit: ++prepopulated_url_index
Thanks for your perseverance Marcus. I did misunderstand your suggestion and like it better now. Through the additional type-safety I easily spotted an unused field too. https://codereview.chromium.org/12255042/diff/14004/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/14004/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:33: * This listener will be notified when template url service is done laoding. On 2013/02/25 12:51:19, bulach wrote: > nit: s/lao/loading/ Done. https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:45: i->second); On 2013/02/25 12:51:19, bulach wrote: > nit: fits previous line Done. https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:123: prepopulated_urls_count++; On 2013/02/25 12:51:19, bulach wrote: > nit: ++prepopulated_urls_count Done. https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:127: base::android::GetClass(env, "java/util/HashMap"); On 2013/02/25 12:51:19, bulach wrote: > I think there's a handy JNI_HashMap::g_HashMap_clazz it's in anon namesapce but sure. https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:143: search_engine_map[kSearchEngineUrl] = template_url->url_ref().DisplayURL(); On 2013/02/25 12:51:19, bulach wrote: > damn ocean apart... :) sorry, I think I'm not exemplifying my thoughts as clear > as I should... > > this looks fine, but let me try to sketch out something, let me know if the > following is clearer: > > java: > class Foo { > class Item { > String mA; > String mB; > @CalledByNative("Item") > create(String a, String b, String c) { > ... > } > } > native int nativeGetItemCount(); > native Item nativeGetItemAt(int n); > ... > Item[] doSomething() { > int count = nativeGetItemCount(); > Item[] ret = new Item[count]; > for (int i = 0; i < count; ++count) > ret[i] = nativeGetItemAt(i); > return ret; > } > } > > > c++: > jint GetCount(...) { > return template_url_service_->GetTemplateURLs()->size(); > } > > jobject GetItemAt(..., jint i) { > TemplateURL* t = > template_url_service_->GetTemplateURLs()[i]; > if (t->prepopulate_id() <= 0) > return NULL; > return Java_Foo_createItem( > t->short_name, t->keyword, t->url_ref().DisplayURL()); > } > > > my summary would then be: > pros: > + type safety guaranteed by Java_Foo_createItem, and explicitly documented in > code by the "Item" class. > + no need to duplicate these consts across the board > + no need to know anything about java map in c++.. it'd only know about the > "Item" class, which is the only thing it cares about. > + less indirections / boxing: it's clear from java code that it requires an > "Item" object, and it's clearer from c++ that it provides one... > + if the java-side needs a map rather than Item, then let java side do this > massaging / conversion, and keep c++ only using "opaque" datatypes (i.e., it > doesn't know what an item is, but it has a clean and safe API to create one).. > the map is too transparent here. > > cons: > - the JNI api now contains a "getCount()" and "getItemAt()" rather than > "getAllItems()".. however, as above, I think these two APIs are simpler and > self-documented... > > > would this make any sense, specially in the context of setting this as a pattern > for other classes in the future? > > wdyt? :) I'm convinced. I didn't realize you meant to get rid of the map entirely and create another type instead of marshalling a key-value across the border. I like it much better now, downstream code is easier to read, and no more magic constants. Thanks! https://codereview.chromium.org/12255042/diff/14004/chrome/browser/search_eng... chrome/browser/search_engines/template_url_service_android.cc:149: prepopulated_url_index++; On 2013/02/25 12:51:19, bulach wrote: > nit: ++prepopulated_url_index Done.
https://codereview.chromium.org/12255042/diff/9009/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/9009/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:47: public int getId() { Isn't this technically an index and not an ID? https://codereview.chromium.org/12255042/diff/9009/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/9009/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:107: TemplateUrlServiceAndroid::GetPrepopulatedTemplateUrlAt(JNIEnv* env, Nit: It might just be me, but the whitespace for jobject and jint looks off.
https://codereview.chromium.org/12255042/diff/9009/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/12255042/diff/9009/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:47: public int getId() { On 2013/02/25 19:39:55, nyquist wrote: > Isn't this technically an index and not an ID? Done. https://codereview.chromium.org/12255042/diff/9009/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/9009/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:107: TemplateUrlServiceAndroid::GetPrepopulatedTemplateUrlAt(JNIEnv* env, On 2013/02/25 19:39:55, nyquist wrote: > Nit: It might just be me, but the whitespace for jobject and jint looks off. Done.
lgtm! many thanks for putting up with my comments yaron, it does look much better now! :) I have a suggestion below, not sure if that will apply so feel free to go ahead anyways. https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:83: if (default_search_provider == template_urls[i]) { nit: could get rid of braces here https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:95: jobject obj) { since the java side is using an array list rather than a fixed-size array and this count is only being used for iterating the next function, I think these two can be simplified: 1. make this function just return the total count, not just the prepopulated ones (return template_url_service_->GetTemplateURLs().size()) 2. on "GetPrepopulatedAt" rather than traversing the vector, just use the N-th element, and if it's not of a Prepoulated type, return NULL.... 3. in the java side, if it's null, don't push into the arraylist. 4. profit! :) (note: I don't fully appreciate how the "prepoulated_url_index" works, given that "SetDefaultSearchProvider" just uses template_urls[selected_index_size_t] directly, is it intended?) https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:100: if (template_urls[i]->prepopulate_id() > 0) truly nit, but could do with a "IsPrepoluatedTemplate(TemplateURL*)", it'd make this one and 117 clearer (and wouldn't require any comments below), but hopefully the comment above will simplify this further. https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:121: return Java_TemplateUrl_create( lovely!!! :D
Thanks, some nice improvements again. Rebased and sending to CQ. tbr'd sky for trivial chrome/chrome_browser.gypi additions. https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:83: if (default_search_provider == template_urls[i]) { On 2013/02/26 09:59:28, bulach wrote: > nit: could get rid of braces here Done. https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:95: jobject obj) { On 2013/02/26 09:59:28, bulach wrote: > since the java side is using an array list rather than a fixed-size array and > this count is only being used for iterating the next function, I think these two > can be simplified: > > 1. make this function just return the total count, not just the prepopulated > ones (return template_url_service_->GetTemplateURLs().size()) > > 2. on "GetPrepopulatedAt" rather than traversing the vector, just use the N-th > element, and if it's not of a Prepoulated type, return NULL.... > > 3. in the java side, if it's null, don't push into the arraylist. > > 4. profit! :) > > (note: I don't fully appreciate how the "prepoulated_url_index" works, given > that "SetDefaultSearchProvider" just uses template_urls[selected_index_size_t] > directly, is it intended?) Done and it's easier to reason about now. https://codereview.chromium.org/12255042/diff/5006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_android.cc:100: if (template_urls[i]->prepopulate_id() > 0) On 2013/02/26 09:59:28, bulach wrote: > truly nit, but could do with a "IsPrepoluatedTemplate(TemplateURL*)", it'd make > this one and 117 clearer (and wouldn't require any comments below), but > hopefully the comment above will simplify this further. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/12255042/21005
Message was sent while issue was closed.
Change committed as 184724 |
