Split fetcher logic out of ManifestUpgradeDetector

Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
TEST=ManifestUpgradeDetectorTest.* ManifestUpgradeFetcherTest.*

Committed: https://crrev.com/7689d29f5d5d7d1cf4937c8d22ee7795c843b26b
Cr-Commit-Position: refs/heads/master@{#409808}

[pkotwicz, 2016-07-20 18:12:13]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2016-07-20 18:12:13]

Xi can you please take a look?

I do not mind if you integrate this CL into
https://codereview.chromium.org/2124513002/
Otherwise, we handle this CL as a separate CL which depends on
https://codereview.chromium.org/2124513002/

[pkotwicz, 2016-07-20 18:12:46]

Xi can you please take a look?

I do not mind if you integrate this CL into
https://codereview.chromium.org/2124513002/
Otherwise, we handle this CL as a separate CL which depends on
https://codereview.chromium.org/2124513002/

I will work on improving the comments in this CL once
https://codereview.chromium.org/2124513002/ has landed

[pkotwicz, 2016-07-20 18:12:47]

Xi can you please take a look?

I do not mind if you integrate this CL into
https://codereview.chromium.org/2124513002/
Otherwise, we handle this CL as a separate CL which depends on
https://codereview.chromium.org/2124513002/

I will work on improving the comments in this CL once
https://codereview.chromium.org/2124513002/ has landed

[Xi Han, 2016-07-29 15:12:37]

Since in CL https://codereview.chromium.org/2124513002/, we are able to destroy
the native pointer of ManifestUpgradeDetector when the fetching is done, so I am
not sure whether we still need these changes. Feel free to pass to other
reviewers to get their opinions.

[pkotwicz, 2016-08-02 00:08:52]

Description was changed from

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantage of the split is that we can now destroy the native fetcher once
the Web Manifest has been fetched. Previously, we kept the native
ManifetsUpgradeDetector alive for the lifetime of the Java object.

BUG=624834
==========

to

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
==========

[pkotwicz, 2016-08-02 00:23:47]

Xi can you please take another look? This CL makes testing a whole lot easier

[pkotwicz, 2016-08-02 02:21:51]

Description was changed from

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
==========

to

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
TEST=ManifestUpgradeDetectorTest.* ManifestUpgradeFetcherTest.*
==========

[Xi Han, 2016-08-02 14:07:25]

lgtm with nits.

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:146:
protected void onComplete() {}
Please add @VisibleForTesting since this function is only used by tests.

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:44:
private static final String WEBAPK_MANIFEST_URL = "/manifest.java";
manifest.json?

[pkotwicz, 2016-08-03 01:40:54]

pkotwicz@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[pkotwicz, 2016-08-03 01:40:54]

dfalcantara@ for OWNERS

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:146:
protected void onComplete() {}
I am unsure whether this function needs @VisibleForTesting.

The function is overridden by ManifestUpgradeDetectorFetcher.
It is called by onGotManifestData()

I tried doing a release build of chrome_junit_tests and the tests still pass
without @VisibleForTesting. What else should I try?

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:44:
private static final String WEBAPK_MANIFEST_URL = "/manifest.java";
On 2016/08/02 14:07:25, Xi Han wrote:
> manifest.json?

Done.

[gone, 2016-08-03 18:31:40]

lgtm

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:66:
* Creates ManifestUpgradeDataFetcher. In dedicated function for the sake of
tests.
Nit: Don't know if you gain anything by declaring that this function exists for
tests.

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java
(right):

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:18:
/** Pointer to the native side ManifestUpgradeDetector. */
Can you add a comment about the Java side owning the native side?

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:25:
* Called once the Web Manifest has been downloaded.
Put this public interface above all the private variables.  It randomly splits
apart the private variables in its current location.

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
(right):

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:29:

Nit: Group PAGE_URLs together up top?

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:42:
// CallbackHelper which blocks till the {@link
ManifestUpgradeDetectorFetcher.Callback} callback
Until or til

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:64:
final Context context = getInstrumentation().getTargetContext();
Remove the final.

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:127:
tabLoadObserver.fullyLoadUrl(mTestServer.getURL(PAGE_URL1));
Does this actually test what you say in the comment?  It loads both URLs and
doesn't check that the callback is called until after both finish.  It could
have triggered for the first one and seems like it would pass this.

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://chromiumcodereview.appspot.com/2161163004/diff/40001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:72:
public boolean mIsUpgraded;
Nit: Public over private.

[gone, 2016-08-03 18:32:15]

Er hit the wrong button.  Need the question about the test answered before you
submit.

[Xi Han, 2016-08-03 18:46:03]

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:146:
protected void onComplete() {}
On 2016/08/03 01:40:54, pkotwicz wrote:
> I am unsure whether this function needs @VisibleForTesting.
> 
> The function is overridden by ManifestUpgradeDetectorFetcher.
> It is called by onGotManifestData()
> 
> I tried doing a release build of chrome_junit_tests and the tests still pass
> without @VisibleForTesting. What else should I try?

Maybe my understanding was wrong, I thought @VisibleForTesting meant the
function was used by testing only. If it works as to increase visibility to
testing, then I am ok to not add the comments.

[gone, 2016-08-03 18:48:12]

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2161163004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:146:
protected void onComplete() {}
On 2016/08/03 18:46:03, Xi Han wrote:
> On 2016/08/03 01:40:54, pkotwicz wrote:
> > I am unsure whether this function needs @VisibleForTesting.
> > 
> > The function is overridden by ManifestUpgradeDetectorFetcher.
> > It is called by onGotManifestData()
> > 
> > I tried doing a release build of chrome_junit_tests and the tests still pass
> > without @VisibleForTesting. What else should I try?
> 
> Maybe my understanding was wrong, I thought @VisibleForTesting meant the
> function was used by testing only. If it works as to increase visibility to
> testing, then I am ok to not add the comments. 

@VisibleForTesting is meant to make sure that ProGuard doesn't strip the
function if the function is used only for testing (i.e. not called by anything
except tests).  If you don't have it in that case, the bots will throw a fit and
fail.  If you add it when it isn't necessary, it basically adds a little
clutter.

[pkotwicz, 2016-08-03 20:36:24]

dfalcantara@ can you please take another look?

https://codereview.chromium.org/2161163004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
(right):

https://codereview.chromium.org/2161163004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:127:
tabLoadObserver.fullyLoadUrl(mTestServer.getURL(PAGE_URL1));
I am unsure how to properly test that an asynchronous operation does not occur.

I check the "name" from the Web Manifest data to make sure that the
ManifestUpgradeDetectorFetcher returned data for the correct Web Manifest.

Do you think that I should delete the test?

[pkotwicz, 2016-08-03 20:36:44]

dfalcantara@ can you please take another look? I think i have addressed all of
your comments

[gone, 2016-08-03 20:42:14]

https://codereview.chromium.org/2161163004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
(right):

https://codereview.chromium.org/2161163004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java:127:
tabLoadObserver.fullyLoadUrl(mTestServer.getURL(PAGE_URL1));
On 2016/08/03 20:36:23, pkotwicz wrote:
> I am unsure how to properly test that an asynchronous operation does not
occur.
> 
> I check the "name" from the Web Manifest data to make sure that the
> ManifestUpgradeDetectorFetcher returned data for the correct Web Manifest.
> 
> Do you think that I should delete the test?

You check the name, but you don't check that the name was overwritten.  You'll
probably be in better shape if you assertNull(mName) or something before setting
mName in the CallbackWaiter.

[pkotwicz, 2016-08-03 20:48:26]

dfalcantara@ can you please take another look?

I have added assertNull(mName) as you have suggested

[gone, 2016-08-03 21:05:23]

lgtm

[pkotwicz, 2016-08-04 15:53:40]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-04 15:53:41]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2161163004/#ps100001
(title: "Merge branch 'master' into webapk_updater_images0")

[commit-bot: I haz the power, 2016-08-04 15:54:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 16:43:53]

Description was changed from

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
TEST=ManifestUpgradeDetectorTest.* ManifestUpgradeFetcherTest.*
==========

to

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
TEST=ManifestUpgradeDetectorTest.* ManifestUpgradeFetcherTest.*
==========

[commit-bot: I haz the power, 2016-08-04 16:43:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-04 16:45:22]

Description was changed from

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
TEST=ManifestUpgradeDetectorTest.* ManifestUpgradeFetcherTest.*
==========

to

==========
Split fetcher logic out of ManifestUpgradeDetector

This CL introduces ManifestUpgradeDetectorFetcher which calls back to
ManifestUpgradeDetector once:
- The user has navigated to a page which uses the specified Web Manifest URL
- The Web Manifest has been downloaded

The advantages of the split are:
- The lifetime of the native fetcher now matches the lifetime of the Java
  fetcher.
- This CL makes JUnit testing easier. JUnit testing of ManifestUpgradeDetector
  is desireable because it avoids having to create a lot of test pages with
  different Web Manifests.

BUG=624834
TEST=ManifestUpgradeDetectorTest.* ManifestUpgradeFetcherTest.*

Committed: https://crrev.com/7689d29f5d5d7d1cf4937c8d22ee7795c843b26b
Cr-Commit-Position: refs/heads/master@{#409808}
==========

[commit-bot: I haz the power, 2016-08-04 16:45:23]

Patchset 6 (id:??) landed as
https://crrev.com/7689d29f5d5d7d1cf4937c8d22ee7795c843b26b
Cr-Commit-Position: refs/heads/master@{#409808}