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..e0d5538d13fdf1e6741f919394505f8a0dc45d76
--- /dev/null
+++ b/chrome/browser/android/webapps/manifest_upgrade_detector.cc
@@ -0,0 +1,148 @@
+// 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_info.h"
+#include "chrome/browser/banners/app_banner_manager.h"

[pkotwicz, 2016/07/13 21:01:15]

Nit: Remove include :)

[Xi Han, 2016/07/15 19:30:01]

Done.

+#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 "url/gurl.h"
+
+using content::Manifest;

[pkotwicz, 2016/07/13 21:01:15]

In my opinion (others will probably disagree), a high threshold of usefulness
must be attained before using the "using" keyword.

In functions like ManifestUpgradeDetector::OnDidGetManifest() you are including
the "content" namespace. Maybe just remove the "using" statement?

[Xi Han, 2016/07/15 19:30:01]

Sure, just forgot to remove it when some functions that used it got removed.

+
+namespace {

[pkotwicz, 2016/07/13 21:01:15]

Nit: new line

[Xi Han, 2016/07/15 19:30:01]

Done.

+// Returns whether the given |url| is within the scope of the |scope| url.

[pkotwicz, 2016/07/13 21:01:15]

Super nit: I think that it is clearer if you switch the order of the |scope| and
|url| parameters. That way "Is" refers to the first function parameter.

[Xi Han, 2016/07/15 19:30:01]

Done.

+bool IsInScope(const GURL& scope, const GURL& url) {
+  const std::string& scope_str = scope.spec().c_str();
+  const std::string& url_str = url.spec().c_str();
+  auto res = std::mismatch(scope_str.begin(), scope_str.end(), url_str.begin());
+  return res.first == scope_str.end();

[pkotwicz, 2016/07/13 21:01:15]

Can you use base::StartsWith() ?

[Xi Han, 2016/07/15 19:30:01]

Have been looking for this function but haven't found. Updated.

+}
+
+} // namespace

[pkotwicz, 2016/07/13 21:01:15]

Nit: 2 spaces before //

[Xi Han, 2016/07/15 19:30:01]

Done.

+
+jlong Initialize(JNIEnv* env,
+                 const JavaParamRef<jobject>& obj,
+                 const JavaParamRef<jobject>& java_web_contents,
+                 const JavaParamRef<jstring>& java_scope,
+                 const JavaParamRef<jstring>& java_web_manifest_url) {
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(java_web_contents);
+  GURL scope(base::android::ConvertJavaStringToUTF8(env, java_scope));
+  GURL web_manifest_url(base::android::ConvertJavaStringToUTF8(
+      env, java_web_manifest_url));
+  ManifestUpgradeDetector* manifest_upgrade_detector =
+      new ManifestUpgradeDetector(env, obj, web_contents, scope,
+                                  web_manifest_url);
+  return reinterpret_cast<intptr_t>(manifest_upgrade_detector);
+}
+
+ManifestUpgradeDetector::ManifestUpgradeDetector(
+    JNIEnv* env,
+    jobject obj,
+    content::WebContents* web_contents,
+    const GURL& scope,
+    const GURL& web_manifest_url)
+    : content::WebContentsObserver(web_contents),
+      start_(false),
+      scope_(scope),
+      web_manifest_url_(web_manifest_url),
+      weak_ptr_factory_(this) {
+  java_ref_.Reset(env, obj);
+}
+
+ManifestUpgradeDetector::~ManifestUpgradeDetector() {
+}
+
+// static
+bool ManifestUpgradeDetector::Register(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+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);
+}
+
+void ManifestUpgradeDetector::DidFinishLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url) {
+  if (render_frame_host->GetParent())
+    return;
+  if (start_ && IsInScope(scope_, validated_url)) {

[pkotwicz, 2016/07/13 21:01:15]

Nit: For the sake of consistency early return

if (!started_ || !IsInScope(validated_url, scope_))
  return;

[Xi Han, 2016/07/15 19:30:01]

Done.

+    content::WebContents* web_contents =
+        content::WebContentsObserver::web_contents();
+    web_contents->GetManifest(
+        base::Bind(&ManifestUpgradeDetector::OnDidGetManifest,
+                  weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void ManifestUpgradeDetector::OnDidGetManifest(
+    const GURL& manifest_url,
+    const content::Manifest& manifest) {

[pkotwicz, 2016/07/13 21:01:15]

Can you please add a comment why we are ignoring the manifest if it is empty.
i.e. why isn't |started_| set to false on line 98? I think that it is the right
thing to do but the reason why should be commented

[Xi Han, 2016/07/15 19:30:01]

Done.

+  if (manifest.IsEmpty() || web_manifest_url_ != manifest_url)
+    return;
+
+  start_ = false;
+
+  ShortcutInfo info(GURL::EmptyGURL());

[pkotwicz, 2016/07/13 21:01:15]

Can you use GURL() instead of GURL::EmptyGURL() ?

[Xi Han, 2016/07/15 19:30:01]

No, we will end up with compiler error, since the constructor of ShortcutInfo
needs a const GURL&.

[pkotwicz, 2016/07/16 00:56:45]

I am willing to bet that we will not have a compiler error. Nothing special
needs to be done to pass variables by reference.

This is valid:
void main() {
  foo(GURL());
}

void foo(const GURL& url) {
  ...
}

This is a bug:
std::pair<const GURL&, int> pair_;
void bar() {
  pair_ = std::make_pair(GURL(), 0);
}

This is also a bug:
const GURL& rod() {
  return GURL();
}

[Xi Han, 2016/07/18 21:49:38]

When passing a reference, the variable should be something reachable when the
function is done. It cannot be just an arbitrary variable within the function.
This is my understanding.

[pkotwicz, 2016/07/19 18:02:37]

From offline discussion, I believe that the error you were getting is
https://en.wikipedia.org/wiki/Most_vexing_parse

There are a lot of places in the Chromium code which do things similar to this.
e.g. RectTest, ManhattanDistanceToPoint in ui/gfx/geometry/rect_unittest.cc
Notice that Rect::ManhattanDistanceToPoint() takes a const ref to a gfx::Point

[Xi Han, 2016/07/20 18:54:59]

Yes, an extra "()" works: ShortcutInfo info((GURL())).

+  info.UpdateFromManifest(manifest);
+  info.manifest_url = manifest_url;
+
+  std::vector<std::string> icon_urls;
+  for (const auto& icon : manifest.icons)
+    icon_urls.push_back(icon.src.spec());
+
+  OnDataAvailable(info, icon_urls);
+}
+
+void ManifestUpgradeDetector::OnDataAvailable(
+    const ShortcutInfo& info,
+    const std::vector<std::string>& icons) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+
+  ScopedJavaLocalRef<jstring> java_url =
+      base::android::ConvertUTF8ToJavaString(env, info.url.spec());
+  ScopedJavaLocalRef<jstring> java_name =
+      base::android::ConvertUTF16ToJavaString(env, info.name);
+  ScopedJavaLocalRef<jstring> java_short_name =
+      base::android::ConvertUTF16ToJavaString(env, info.short_name);
+  ScopedJavaLocalRef<jobjectArray> java_icons =
+            base::android::ToJavaArrayOfStrings(env, icons);
+
+  Java_ManifestUpgradeDetector_onDataAvailable(
+    env, java_ref_.obj(),
+    java_url.obj(),
+    java_name.obj(),
+    java_short_name.obj(),
+    java_icons.obj(),
+    info.display,
+    info.orientation,
+    info.theme_color,
+    info.background_color);
+}
+
+void ManifestUpgradeDetector::Destroy(JNIEnv* env,
+                                      const JavaParamRef<jobject>& obj) {
+  delete this;
+}
+
+void ManifestUpgradeDetector::Start(JNIEnv* env,
+                                      const JavaParamRef<jobject>& obj) {

[pkotwicz, 2016/07/13 21:01:15]

Nit: Order the functions in this file to match the order in the .h file

[Xi Han, 2016/07/15 19:30:01]

Done.

+  start_ = true;
+}