Introduce ManifestUpgradeDetector for WebAPK to detect web manifest changes.

chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc

Index: chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc
diff --git a/chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc b/chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..330140e1ad5522de27223800802f05c872f51999
--- /dev/null
+++ b/chrome/browser/android/webapk/manifest_upgrade_detector_unittest.cc
@@ -0,0 +1,20 @@
+// 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/webapk/manifest_upgrade_detector.h"
+#include "content/public/common/manifest.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ManifestUpgradeDetectorTest = testing::Test;
+
+TEST_F(ManifestUpgradeDetectorTest,
+       OnDidGetManifestReturnsFalseWhenTheFetchedManifestUrlIsEmpty) {

[pkotwicz, 2016/07/20 20:57:04]

This is on the border of what should be unit tested. The logic in
ManifestUpgradeDetector::DidFinishLoad() is simple but non obvious. I did not
realize when I asked you to write the unit tests that the JNI call would be a
problem.

You could add a method ManifestUpgradeDetector::ShouldStop(const GURL&
manifest_url, const content::Manifest& manifest) and unit test that

I will defer to Yaron as to whether manifest_upgrade_detector\.cc should be unit
tested. Right now I am leaning towards no. Sorry for asking you to write all of
this code :(

[pkotwicz, 2016/07/22 20:13:05]

Still deferring to Yaron

[pkotwicz, 2016/07/25 17:35:52]

Yaron bump!

[Yaron, 2016/07/25 18:00:21]

Not sure why JNI code is a problem - the Java code should be registered with the
test fixture and if it's not, that's a bug.

I think this testing is pretty minimal - by itself it's not interesting but if
you covered the other cases (e.g. manifest doesn't match, then it might). You
could actually close the feedback loop - by having the java
ManifestUpgradeDetector notify native whether or not upgrade is necessary, you
could test the variations from here and possibly not add 
ManifestUpgradeDetectorTest? 

[Xi Han, 2016/07/25 20:01:37]

I would prefer to simply to remove this test. For the case of having a non-empty
web_manifest_url but doesn't match the current one, we can't really tell if this
is another manifest url (within the webapp's scope) or the old url is changed.
So we just return here, won't continue fetching until get the result of if
upgrade or not.

+  ManifestUpgradeDetector detector(nullptr, jobject(), nullptr, GURL(), GURL());
+  EXPECT_EQ(ManifestUpgradeDetector::NoErrorDetected, detector.error_code());
+
+  content::Manifest manifest;
+  detector.OnDidGetManifest(GURL(), manifest);
+  EXPECT_EQ(ManifestUpgradeDetector::FetchedManifestEmpty,
+            detector.error_code());
+}