Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes.

chrome/browser/android/webapps/manifest_upgrade_detector.cc

Index: chrome/browser/android/webapps/manifest_upgrade_detector.cc
diff --git a/chrome/browser/android/webapps/manifest_upgrade_detector.cc b/chrome/browser/android/webapps/manifest_upgrade_detector.cc
new file mode 100644
index 0000000000000000000000000000000000000000..74f36ada08fe65d2b987bf35ec774c3922b869e6
--- /dev/null
+++ b/chrome/browser/android/webapps/manifest_upgrade_detector.cc
@@ -0,0 +1,147 @@
+// Copyright 2016 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/webapps/manifest_upgrade_detector.h"
+
+#include <jni.h>
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
+#include "chrome/browser/android/shortcut_helper.h"
+#include "chrome/browser/banners/app_banner_manager.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/manifest.h"
+#include "jni/ManifestUpgradeDetector_jni.h"
+#include "ui/gfx/android/java_bitmap.h"
+#include "url/gurl.h"
+
+using content::Manifest;
+
+jlong Initialize(JNIEnv* env,
+                 const JavaParamRef<jobject>& obj,
+                 const JavaParamRef<jobject>& java_web_contents) {
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(java_web_contents);
+  ManifestUpgradeDetector* manifest_upgrade_detector =
+      new ManifestUpgradeDetector(env, obj, web_contents);
+  return reinterpret_cast<intptr_t>(manifest_upgrade_detector);
+}
+
+ManifestUpgradeDetector::ManifestUpgradeDetector(
+    JNIEnv* env,
+    jobject obj,
+    content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents),
+      start_fetching_(false),
+      data_fetcher_(nullptr) {
+  java_ref_.Reset(env, obj);
+}
+
+ManifestUpgradeDetector::~ManifestUpgradeDetector() {
+  data_fetcher_->set_weak_observer(nullptr);
+  data_fetcher_ = nullptr;
+}
+
+// static
+bool ManifestUpgradeDetector::Register(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+void ManifestUpgradeDetector::DidFinishLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url) {

[pkotwicz, 2016/07/12 01:23:49]

Random thought: I wonder how this works with 301 HTTP redirects.

For instance:
WebAPK is minted with start URL www.example.com/home1.html
www.example.com/home1.html is changed to HTTP 301 redirect to
www.example.com/home2.html

www.example.com/home2.html uses the same web manifest URL as
www.example.com/home1.html uses to

[Xi Han, 2016/07/12 20:53:41]

If both urls link to the same manifest, no matter which url is loaded and points
us to the manifest, should be considered as the manifest is found, and we are
ok. But if they don't point to the same manifest, then we have to check whether
the manifest is the one that we look for. Do I get your point?

[pkotwicz, 2016/07/12 21:04:28]

My concern is that if the start URL is a 301 HTTP redirect we will get
ManifestUpgradeDetector::DidFinishLoad() but fetching the manifest will fail.

Do we even get ManifestUpgradeDetector::DidFinishLoad() for 301 HTTP redirects?

+  if (render_frame_host->GetParent())
+    return;
+  if (start_fetching_)
+    RequestDetector(validated_url);
+}
+
+void ManifestUpgradeDetector::ReplaceWebContents(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& jweb_contents) {
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(jweb_contents);
+  content::WebContentsObserver::Observe(web_contents);
+  if (data_fetcher_.get())
+    data_fetcher_.get()->ReplaceWebContents(web_contents);
+}
+
+void ManifestUpgradeDetector::RequestDetector(const GURL& validated_url) {
+  if (data_fetcher_.get() && banners::AppBannerManager::URLsAreForTheSamePage(
+      data_fetcher_->shortcut_info().url, validated_url))
+    return;
+
+  if (data_fetcher_.get())
+    data_fetcher_->set_weak_observer(nullptr);
+  data_fetcher_ = CreateDataFetcher();
+}
+
+AddToHomescreenDataFetcher* ManifestUpgradeDetector::CreateDataFetcher() {
+    return new AddToHomescreenDataFetcher(web_contents(),
+          ShortcutHelper::GetIdealHomescreenIconSizeInDp(),
+          ShortcutHelper::GetMinimumHomescreenIconSizeInDp(),
+          ShortcutHelper::GetIdealSplashImageSizeInDp(),
+          ShortcutHelper::GetMinimumSplashImageSizeInDp(),
+          true, this);

[pkotwicz, 2016/07/12 01:23:49]

For the sake of landing something this week, I think that it makes sense to
revert the image-related checking. (Don't delete it, keep it in a separate git
branch)

For the image-related checking we should extract the useful parts out of
AddToHomescreenDataFetcher into a separate class

Without the image related checking, this class needs to just call:
content::WebContents::GetManifest()
(which is simple enough that it should not interfere with dominickn@'s work)
I think that it should be simple enough that you do not need a dedicated fetcher
class

I do not think that it is worth calling WebContents::HasManifest(). Calling
WebContents::HasManifest() prior to WebContents::GetManifest() does not save
much/any computation.

[Xi Han, 2016/07/12 20:53:41]

That is definitely a good idea.

+}
+
+void ManifestUpgradeDetector::OnDidHasManifest(bool has_manifest) {
+  start_fetching_ = !has_manifest;
+}
+
+void ManifestUpgradeDetector::OnDataAvailable(
+    const ShortcutInfo& info,
+    const SkBitmap& icon,
+    const std::vector<std::string>& icons) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+
+  start_fetching_ = false;
+  ScopedJavaLocalRef<jstring> java_url =
+      base::android::ConvertUTF8ToJavaString(env, info.url.spec());
+  ScopedJavaLocalRef<jstring> java_user_title =
+      base::android::ConvertUTF16ToJavaString(env, info.user_title);
+  ScopedJavaLocalRef<jstring> java_name =
+      base::android::ConvertUTF16ToJavaString(env, info.name);
+  ScopedJavaLocalRef<jstring> java_short_name =
+      base::android::ConvertUTF16ToJavaString(env, info.short_name);
+  ScopedJavaLocalRef<jobject> java_bitmap;
+  if (icon.getSize())
+    java_bitmap = gfx::ConvertToJavaBitmap(&icon);
+  ScopedJavaLocalRef<jobjectArray> java_icons =
+            base::android::ToJavaArrayOfStrings(env, icons);
+

[pkotwicz, 2016/07/12 01:23:49]

We should check whether the manifest URL is the same one as in the WebAPK
manifest.

If
1) a user minted a WebAPK for funnysite.com
2) the developer for funnysite.com makes their home page do an HTTP redirect to
google.com
This logic should not cause a OneOffWebAPK for funnysite.com to be updated into
a OneOffWebAPK for google.com

[Xi Han, 2016/07/12 20:53:41]

That was what I thought. Create a
CL(https://codereview.chromium.org/2140273002/) to plumb web_manifest_url to
webappInfo. We will check whether the manifest url loaded matches this baked
one.

+  Java_ManifestUpgradeDetector_onDataAvailable(
+    env, java_ref_.obj(),
+    java_url.obj(),
+    java_name.obj(),
+    java_short_name.obj(),
+    java_bitmap.obj(),
+    java_icons.obj(),
+    info.display,
+    info.orientation,
+    info.theme_color,
+    info.background_color);
+}
+
+SkBitmap ManifestUpgradeDetector::FinalizeLauncherIcon(
+      const SkBitmap& bitmap,
+      const GURL& url,
+      bool* is_generated) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  return SkBitmap();
+}
+
+void ManifestUpgradeDetector::Destroy(JNIEnv* env,
+                                      const JavaParamRef<jobject>& obj) {
+  delete this;
+}
+
+void ManifestUpgradeDetector::Start(JNIEnv* env,
+                                      const JavaParamRef<jobject>& obj) {
+  start_fetching_ = true;

[pkotwicz, 2016/07/12 01:23:49]

Nit: Rename to |started_|

[Xi Han, 2016/07/12 20:53:41]

Done.

+}