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

chrome/browser/android/banners/app_banner_manager_android.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/banners/app_banner_manager_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #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"
 #include "content/public/common/frame_navigate_params.h"
 #include "jni/AppBannerManager_jni.h"
 
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::ConvertUTF16ToJavaString;
 
 namespace {
 
 const char kPlayPlatform[] = "play";
 const char kReferrerName[] = "referrer";
 const char kIdName[] = "id";
 const char kPlayInlineReferrer[] = "playinline=chrome_inline";
 
 }  // anonymous namespace
 
 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) {
 }
 
 AppBannerManagerAndroid::~AppBannerManagerAndroid() {
 }
 
 void AppBannerManagerAndroid::Destroy(JNIEnv* env, jobject obj) {
   delete this;
 }
 
@@ skipping 35 matching lines @@
     referrer = kPlayInlineReferrer;
   else
     referrer.append("&").append(kPlayInlineReferrer);
 
   ScopedJavaLocalRef<jstring> jurl(
       ConvertUTF8ToJavaString(env, data_fetcher()->validated_url().spec()));
   ScopedJavaLocalRef<jstring> jpackage(
       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;
 }
 
 bool AppBannerManagerAndroid::CheckPlatformAndId(const std::string& platform,
                                                  const std::string& id) {
   if (platform != kPlayPlatform) {
     banners::OutputDeveloperDebugMessage(
         web_contents(), platform + banners::kIgnoredNotSupportedOnAndroid);
     return false;
   }
@@ skipping 27 matching lines @@
   while (url::ExtractQueryKeyValue(url_spec, &query, &key, &value)) {
     std::string key_str(url_spec, key.begin, key.len);
     std::string value_str(url_spec, value.begin, value.len);
     if (key_str == name)
       return value_str;
   }
   return "";
 }
 
 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,
                                                     jobject obj,
                                                     jobject japp_data,
                                                     jstring japp_title,
                                                     jstring japp_package,
                                                     jstring jicon_url) {
   if (!CheckFetcherMatchesContents())
     return false;
@@ skipping 13 matching lines @@
 bool AppBannerManagerAndroid::IsFetcherActive(JNIEnv* env, jobject obj) {
   return AppBannerManager::IsFetcherActive();
 }
 
 // static
 bool AppBannerManagerAndroid::Register(JNIEnv* env) {
   return RegisterNativesImpl(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);
 }
 
 void SetTimeDeltaForTesting(JNIEnv* env,
                             const JavaParamRef<jclass>& clazz,
                             jint days) {
   AppBannerDataFetcher::SetTimeDeltaForTesting(days);
 }
 
 void DisableSecureSchemeCheckForTesting(JNIEnv* env,
                                         const JavaParamRef<jclass>& clazz) {
   AppBannerManager::DisableSecureSchemeCheckForTesting();
 }
 
 void SetEngagementWeights(JNIEnv* env,
                           const JavaParamRef<jclass>& clazz,
                           jdouble direct_engagement,
                           jdouble indirect_engagement) {
   AppBannerManager::SetEngagementWeights(direct_engagement,
                                          indirect_engagement);
 }
 
 jboolean IsEnabled(JNIEnv* env, const JavaParamRef<jclass>& clazz) {
   return base::FieldTrialList::FindFullName("AppBanners") == "Enabled";
 }
 
 }  // namespace banners