Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes.

chrome/browser/android/webapps/manifest_upgrade_detector.h

+// 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.

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

I started putting C++ files in chrome/browser/android/webapk Maybe we should
move this file to that folder too.

One day we will move the WebAPK Java code to a WebAPK specific folder too

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

Missed this comment last time, moved now.

+
+#ifndef CHROME_BROWSER_ANDROID_WEBAPPS_MANIFEST_UPGRADE_DETECTOR_H_
+#define CHROME_BROWSER_ANDROID_WEBAPPS_MANIFEST_UPGRADE_DETECTOR_H_
+
+#include <vector>
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_weak_ref.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "content/public/browser/web_contents_observer.h"
+
+namespace content {
+struct Manifest;
+class WebContents;
+}  // namespace content

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

Nit: We do not add comments for forward declarations. A long time ago, I made
the mistake of writing a script to "fix" a number of forward declarations by
adding a "namespace comment"

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

Done.

+
+class GURL;
+struct ShortcutInfo;
+
+// ManifestUpgradeDetector is the C++ counterpart of
+// org.chromium.chrome.browser's ManifestUpgradeDetector in Java. The object
+// is owned by the Java object. It is created from there via a JNI (Initialize)
+// call and MUST BE DESTROYED via Destroy().

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

Nit: Remove "The object is owned by the Java object."

I think that the following sentence says the same thing

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

Done.

+class ManifestUpgradeDetector : public content::WebContentsObserver {
+ public:

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

Can the constructor be private?

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

It can't, because it is called by the jni function to initialize the c++ object.

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

Yes, I see now. I thought that Initialize() was a static member function of
ManifestUpgradeDetector but it's not.

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

Acknowledged.

+  ManifestUpgradeDetector(JNIEnv* env,
+                          jobject obj,
+                          content::WebContents* web_contents,

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

Nit: switch the order of |web_manifest_url| and |scope|

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

As discussed offline, keep this order for now:)

+                          const GURL& scope,
+                          const GURL& web_manifest_url);
+
+  // Observes a new WebContents, if necessary.

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

How about: "Replaces the WebContents that is being observed."

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

Done.

+  void ReplaceWebContents(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jobject>& jweb_contents);
+
+  // Called by the Java counterpart to destroy its native half.
+  void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+
+  // Called by the Java counterpart to start checking web manifest changes.
+  void Start(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+
+  // Registers JNI hooks.
+  static bool Register(JNIEnv* env);
+
+  // Called when the Manifest has been parsed, or if no Manifest was found.
+  void OnDidGetManifest(const GURL& manifest_url,
+                        const content::Manifest& manifest);
+ private:
+  ~ManifestUpgradeDetector() override;
+
+  // content::WebContentsObserver.

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

Nit: "content::WebContentsObserver." -> "content::WebContentsObserver:"

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

Done.

+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     const GURL& validated_url) override;
+
+  void OnDataAvailable(const ShortcutInfo& info,
+                       const std::vector<std::string>& icon_urls);
+
+  // Points to the Java object.
+  base::android::ScopedJavaGlobalRef<jobject> java_ref_;
+

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

Nit: |start_| -> |started_|

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

Done.

+  bool start_;
+
+  const GURL scope_;
+  const GURL web_manifest_url_;
+
+  base::WeakPtrFactory<ManifestUpgradeDetector> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ManifestUpgradeDetector);
+};
+
+#endif  // CHROME_BROWSER_ANDROID_WEBAPPS_MANIFEST_UPGRADE_DETECTOR_H_