webapps: allow callers of icon downloader/selector to specify a minimum size

chrome/browser/android/banners/app_banner_manager_android.cc

Index: chrome/browser/android/banners/app_banner_manager_android.cc
diff --git a/chrome/browser/android/banners/app_banner_manager_android.cc b/chrome/browser/android/banners/app_banner_manager_android.cc
index 9caa50eb07218140752acf6179c80fb7f43c4a46..c5ece7d4b0fa2b1d5ea7f0dc435de528cb636763 100644
--- a/chrome/browser/android/banners/app_banner_manager_android.cc
+++ b/chrome/browser/android/banners/app_banner_manager_android.cc
@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "chrome/browser/android/banners/app_banner_data_fetcher_android.h"
+#include "chrome/browser/android/shortcut_helper.h"
 #include "chrome/browser/banners/app_banner_metrics.h"
 #include "chrome/common/chrome_constants.h"
 #include "content/public/browser/web_contents.h"
@@ -33,11 +34,8 @@ namespace banners {
 
 AppBannerManagerAndroid::AppBannerManagerAndroid(
     JNIEnv* env,
-    jobject obj,
-    int ideal_splash_image_size_in_dp,
-    int ideal_icon_size_in_dp)
-    : AppBannerManager(ideal_icon_size_in_dp),
-      ideal_splash_image_size_in_dp_(ideal_splash_image_size_in_dp),
+    jobject obj)
+    : AppBannerManager(),
       weak_java_banner_view_manager_(env, obj) {
 }
 
@@ -93,9 +91,10 @@ bool AppBannerManagerAndroid::HandleNonWebApp(const std::string& platform,
       ConvertUTF8ToJavaString(env, id));
   ScopedJavaLocalRef<jstring> jreferrer(
       ConvertUTF8ToJavaString(env, referrer));
-  Java_AppBannerManager_fetchAppDetails(env, jobj.obj(), jurl.obj(),
-                                        jpackage.obj(), jreferrer.obj(),
-                                        ideal_icon_size_in_dp());
+  Java_AppBannerManager_fetchAppDetails(
+      env, jobj.obj(), jurl.obj(),
+      jpackage.obj(), jreferrer.obj(),
+      ShortcutHelper::GetIdealIconSizeInDp());
   return true;
 }
 
@@ -143,11 +142,14 @@ std::string AppBannerManagerAndroid::ExtractQueryValueForName(
 }
 
 AppBannerDataFetcher* AppBannerManagerAndroid::CreateAppBannerDataFetcher(
-    base::WeakPtr<Delegate> weak_delegate,
-    const int ideal_icon_size_in_dp) {
-  return new AppBannerDataFetcherAndroid(web_contents(), weak_delegate,
-                                         ideal_splash_image_size_in_dp_,
-                                         ideal_icon_size_in_dp);
+    base::WeakPtr<Delegate> weak_delegate) {
+  return new AppBannerDataFetcherAndroid(
+      web_contents(),
+      weak_delegate,
+      ShortcutHelper::GetIdealSplashImageSizeInDp(),
+      ShortcutHelper::GetMinimumSplashImageSizeInDp(),
+      ShortcutHelper::GetIdealIconSizeInDp(),
+      ShortcutHelper::GetMinimumIconSizeInDp());

[gone, 2015/09/08 21:01:57]

Doing it like this means that doing 4 JNI calls when you could have just passed
them in from the Java side.  That's terrible from a performance standpoint.  Do
these values ever change?  Does it make sense to initialize them once from Java,
then send and cache them on the native side?

[gone, 2015/09/08 21:02:47]

Cache as native-side statics, I mean.

[Lalit Maganti, 2015/09/11 13:06:00]

The problem would be where this initialization would be done. There is no sane
place for that to happen. Because of that, I've switched back to storing the
variables in Android so we don't do JNI calls.

[gone, 2015/09/11 16:48:48]

You should really turn down the hyperbole and investigate potential options,
instead.  What's stopping you, for example, from checking if the static
variables I suggested have been initialized at all from C++, making a single JNI
call to tell Java code that it needs to be initialized, then making a Java->C++
call with the four values?

[Lalit Maganti, 2015/09/11 17:09:09]

I did consider that but I really didn't like the boilerplate of that approach -
it seems to be more boilerplaty than simply passing the variables in when the
C++ objects are fist created.

[gone, 2015/09/11 17:17:54]

How about caching the values when the first calls are to
ShortcutHelper::Get*SizeInDp()?

[Lalit Maganti, 2015/09/11 17:26:03]

That is another possibility yes but would still have the performance penalty of
4 JNI calls on the first try. If you think that's acceptable, I would be happy
enough to proceed with that. Another option is we have a single method which
retrieves all 4 and caches it. Would involve the creation of a Java object and
passing it to C++ but would save 4 JNI calls. We could even cache that for
greater efficiency.

[gone, 2015/09/11 17:33:03]

A single 4 JNI call hit where you cache the values is not as good as the 2 JNI
calls I was suggesting, but I'd still find it acceptable over making these four
JNI calls every single time the user navigates to a new page (which triggers the
creation of a new AppBannerDataFetcher).

I'm not sure why you need to create a Java object, though.  You can make calls
to static Java methods without creating objects; look at
AwContents::DocumentHasImagesCallback() and
AwContents#onDocumentHasImagesResponse().

[Lalit Maganti, 2015/09/11 17:38:49]

No I don't mean create an object to call it. I meant to do 1 JNI call where the
Java side returned an object containing all 4 ints. This could even be an array
I guess (although that would be ugly and have questions about which position
holds which value)

Ahhhhh now I understand why you were so concerned about the performance
implication of this - this happens on every page. Me and Mounir were unsure as
we thought that this wasn't a performance sensitive codepath but if it happens
every page load we definitely don't want too many JNI calls.

In summary, I think either caching with 1 JNI call retrieving all 4 ints at the
same time or the caching of 4 JNI calls is fine by me. Your call which one to
pick.

[gone, 2015/09/11 17:42:10]

Sorry :/  Should have explained why this was less than ideal more clearly.

If you can find a way to return a four int array, go for it.  Otherwise, the
one-time 4 JNI call hit is fine by me.

I get the feeling that the array path should be possible, but I don't know how
to do it off the top of my head.

[Lalit Maganti, 2015/09/14 11:35:19]

Implemented the array solution in latest PS.

 }
 
 bool AppBannerManagerAndroid::OnAppDetailsRetrieved(JNIEnv* env,
@@ -181,12 +183,9 @@ bool AppBannerManagerAndroid::Register(JNIEnv* env) {
 }
 
 jlong Init(JNIEnv* env,
-           const JavaParamRef<jobject>& obj,
-           jint ideal_splash_image_size_in_dp,
-           jint ideal_icon_size_in_dp) {
+           const JavaParamRef<jobject>& obj) {
   AppBannerManagerAndroid* manager =
-      new AppBannerManagerAndroid(env, obj,
-          ideal_splash_image_size_in_dp, ideal_icon_size_in_dp);
+      new AppBannerManagerAndroid(env, obj);
   return reinterpret_cast<intptr_t>(manager);
 }