Merge WebappInfo and WebApkMetaData part 2/2

Merge WebappInfo and WebApkMetaData part 2/2

This CL:
- Deletes WebApkMetaData
- Adds parameters in WebApkMetaData but not in WebApkInfo to WebApkInfo.
- Moves code from WebApkMetaDataUtils to WebApkInfo

BUG=659845
Committed: https://crrev.com/cb48ebabe16d969057fc583c496729547a1b10d5
Cr-Commit-Position: refs/heads/master@{#434594}

[pkotwicz, 2016-10-28 14:45:13]

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

[pkotwicz, 2016-10-28 14:45:14]

Dominick, can you please take a look? This CL deletes WebApkMetaData

[pkotwicz, 2016-10-28 14:46:30]

Patchset #1 (id:1) has been deleted

[dominickn, 2016-10-31 04:29:43]

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:91:
public ManifestUpgradeDetector(Tab tab, WebApkInfo info, Callback callback) {
There's a lot of Uri -> String conversion going on in here. Maybe you could have
String mScopeUrl and mManifestUrl members, and initialize them as
mInfo.scopeUri()/toString() in the constructor? That will save you making
multiple copies of it everywhere (since it's Java every call to toString() is
going to heap-allocate anyway so you may as well keep it on this object). It
also means you can avoid changing ManifestUpgradeDetectorFetcher's constructor
signature.

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:76:
int displayMode = displayModeFromString(
Just to make this easier to read, can you stick in newlines:

 - after scope
 - after backgroundColor
 - after shellApkVersion

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:94:
resources = packageManager.getResourcesForApplication(webApkPackageName);
Nit: perhaps move this up into the first try/catch? Both result in a null return
with no side effects, so you may as well do it all first.

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:145:
protected void updateAsync(String startUrl, String scopeUrl, String name, String
shortName,
It would be super nice if updateAsync could just take a WebApkInfo parameter and
unpack that internally rather than the huge list of arguments (it means you only
have the super long list in one place rather than three). I know you'd prefer to
keep FetchedManifestData separate from WebApkInfo; I just want to point out that
it would make this a lot nicer if that sways you. :)

That also means you can get rid of updateAsyncUsingAndroidManifestMetaData,
because that's just updateAsync(mInfo).

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:71:
private static class CreationData {
Instead of having this class, can't you just call WebApkInfo.create directly in
defaultCreationData() instead of in startManifestUpgradeDetector?

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:182:
private TestManifestUpgradeDetector createDetector(WebappInfoCreationData
oldData,
This test would also be nicer if FetchedManifestData and WebappInfoCreationData
could just be replaced by WebApkInfo.

[pkotwicz, 2016-11-01 01:32:20]

Patchset #3 (id:60001) has been deleted

[pkotwicz, 2016-11-01 01:32:28]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2016-11-01 01:32:43]

Dominick, can you please take another look?

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:91:
public ManifestUpgradeDetector(Tab tab, WebApkInfo info, Callback callback) {
On 2016/10/31 04:29:43, dominickn wrote:
> There's a lot of Uri -> String conversion going on in here. Maybe you could
have
> String mScopeUrl and mManifestUrl members, and initialize them as
> mInfo.scopeUri()/toString() in the constructor? That will save you making
> multiple copies of it everywhere (since it's Java every call to toString() is
> going to heap-allocate anyway so you may as well keep it on this object). It
> also means you can avoid changing ManifestUpgradeDetectorFetcher's constructor
> signature.

Done.

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:76:
int displayMode = displayModeFromString(
On 2016/10/31 04:29:43, dominickn wrote:
> Just to make this easier to read, can you stick in newlines:
> 
>  - after scope
>  - after backgroundColor
>  - after shellApkVersion

Done.

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:94:
resources = packageManager.getResourcesForApplication(webApkPackageName);
I like having short try{} statements. The alternative is to have a try{}
statement spanning > 20 lines

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:71:
private static class CreationData {
The tests mutate CreationData. WebApkInfo cannot be mutated after creation (A
good thing - except in tests)

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:182:
private TestManifestUpgradeDetector createDetector(WebappInfoCreationData
oldData,
I have completely rewritten the tests in this file in
https://codereview.chromium.org/2460253002/ That CL merges
WebappInfoCreationData and FetchedManifestData (and moves the tests to
WebApkUpdateManagerTest)

The nice thing about WebappInfoCreationData and FetchedManifestData is that they
can be easily mutated after being constructed. WebApkInfo cannot be mutated
after being constructed. If I were to pass in WebApkInfo into this function,
each test would have to call the 17 parameter version of WebApkInfo#create()

[dominickn, 2016-11-01 02:48:50]

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:94:
resources = packageManager.getResourcesForApplication(webApkPackageName);
On 2016/11/01 01:32:42, pkotwicz wrote:
> I like having short try{} statements. The alternative is to have a try{}
> statement spanning > 20 lines

Why would it be >20 lines? I meant just move the one line

resources = packageManager.getResourcesForApplication(webApkPackageName);

into the previous try statement, move the variable declaration up, and delete
this try statement. You already have the webApkPackageName in the previous try
which is all you need. It would be a 2 line try statement.

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:182:
private TestManifestUpgradeDetector createDetector(WebappInfoCreationData
oldData,
You could add setters to facilitate this rather than another whole class (why I
suggested making the properties in Webappinfo protected so subclasses can access
them). You're only changing 5-6 of the fields in tests. I don't think it's
really worth having the almost-but-not-quite-the-same class just for that.
Ideally WebApkInfo should be used everywhere.

The alternate approach is to have a createForTesting method which takes the 5/6
arguments which you use and sets everything else to null values. That explicitly
sets out the fields that you care about.

Ultimately, there should be just one method (WebApInfo.create) that requires
that many arguments. Otherwise, classes create and pass around a WebApkInfo that
contains all the required data.

[pkotwicz, 2016-11-02 02:25:32]

I am going to put this CL on ice for a bit in order to focus on other WebAPK
related bugs for the Chrome Developer Summit

[pkotwicz, 2016-11-22 05:09:53]

Patchset #3 (id:100001) has been deleted

[pkotwicz, 2016-11-22 05:18:36]

Dominick can you please take another look?

- I have rebased the CL to tip of tree (including Xi's changes)
- I have merged WebApkInfo, WebApkMetaData, and FetchedManifestData class as you
have requested
- I have created a new ManifestData class which is only used in tests in order
to construct WebApkInfo more easily
- I have added a new WebappInfo#Icon class which avoids having to call
WebappInfo#encodeBitmapAsString() when creating a WebappInfo object

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

https://codereview.chromium.org/2459023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:94:
resources = packageManager.getResourcesForApplication(webApkPackageName);
On 2016/11/01 02:48:50, dominickn wrote:
> On 2016/11/01 01:32:42, pkotwicz wrote:
> > I like having short try{} statements. The alternative is to have a try{}
> > statement spanning > 20 lines
> 
> Why would it be >20 lines? I meant just move the one line
> 
> resources = packageManager.getResourcesForApplication(webApkPackageName);
> 
> into the previous try statement, move the variable declaration up, and delete
> this try statement. You already have the webApkPackageName in the previous try
> which is all you need. It would be a 2 line try statement.

Done.

[dominickn, 2016-11-22 20:49:45]

First pass. Thanks for doing this, I think having just the one WebAPK data class
makes everything much easier to reason about.

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130:
return !TextUtils.equals(bestIconMurmur2Hash, fetchedBestIconMurmur2Hash)
Can we move most of this comparison inside WebApkInfo?

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:64:
PackageManager packageManager =
ContextUtils.getApplicationContext().getPackageManager();
Nit: to split this method up a bit, can you pull out lines 64 - 74 into a method
that returns a Bundle called getWebApkMetadata(String webApkPackageName)? Then
return null if that method returns null.

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:149:
private static long getLongFromMetaData(Bundle metaData, String name, long
defaultValue) {
Nit: put these private methods at the bottom of the file to that the create
methods/constructors are together.

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:29:
public static class Icon {
Can this class get a String url parameter? Then WebAPKs can use that, with the
WebappInfo storing null. Then the WebAPK update classes don't need the extra
bestUrl parameter - they can get it straight off WebApkInfo. WDYT?

[pkotwicz, 2016-11-23 02:15:23]

Dominick, can you please take a look?

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130:
return !TextUtils.equals(bestIconMurmur2Hash, fetchedBestIconMurmur2Hash)
I don't think that having a WebApkInfo#equals() method makes sense.
- We are only checking a subset of the fields
- We are comparing some of the data in WebApkInfo#iconUrlToMurmur2HashMap()

What are you suggesting calling the method which does the comparison in
WebApkInfo?

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:64:
PackageManager packageManager =
ContextUtils.getApplicationContext().getPackageManager();
I have added two new methods
extractWebApkMetaData() and decodeImageResource()

I did not name the method getWebApkMetaData() because I only want to name
trivial getters and setters as get() and set(). (agrieve@ mentioned this rule of
thumb to me and I think it is a good one)

I also spelled the method MetaData instead of Metadata to match Android
PackageItemInfo#metaData. We seem to use both MetaData and Metadata
capitalizations in Chrome Java

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:149:
private static long getLongFromMetaData(Bundle metaData, String name, long
defaultValue) {
On 2016/11/22 20:49:45, dominickn wrote:
> Nit: put these private methods at the bottom of the file to that the create
> methods/constructors are together.

Done.

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:29:
public static class Icon {
I think that adding a url member to this class makes this class too complicated.

The url member would either:
- have to be set via the constructor. Both constructors will take two parameters
- ugly
- the constructor would only set the bitmap data and there will be an additional
setter for the url. Having a setter is weird in my opinion. WebappInfo populates
all of its members in the constructor

[dominickn, 2016-11-25 00:39:38]

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130:
return !TextUtils.equals(bestIconMurmur2Hash, fetchedBestIconMurmur2Hash)
On 2016/11/23 02:15:22, pkotwicz wrote:
> I don't think that having a WebApkInfo#equals() method makes sense.
> - We are only checking a subset of the fields
> - We are comparing some of the data in WebApkInfo#iconUrlToMurmur2HashMap()
> 
> What are you suggesting calling the method which does the comparison in
> WebApkInfo?

Perhaps needsUpgrade? equals definitely isn't the right name.

/**
 * Returns true if the fetched manifest info |fetchedInfo| has updated data
compared to this object.
 */
boolean needsUpgrade(WebApkInfo fetchedInfo)

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:64:
PackageManager packageManager =
ContextUtils.getApplicationContext().getPackageManager();
On 2016/11/23 02:15:22, pkotwicz wrote:
> I have added two new methods
> extractWebApkMetaData() and decodeImageResource()
> 
> I did not name the method getWebApkMetaData() because I only want to name
> trivial getters and setters as get() and set(). (agrieve@ mentioned this rule
of
> thumb to me and I think it is a good one)
> 
> I also spelled the method MetaData instead of Metadata to match Android
> PackageItemInfo#metaData. We seem to use both MetaData and Metadata
> capitalizations in Chrome Java

You've changed the behaviour now. Previously, you would return a null WebApkInfo
if you couldn't get the Resources from the package manager. Now you just set the
icon to be null. Is that correct/acceptable behaviour here?

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:29:
public static class Icon {
On 2016/11/23 02:15:23, pkotwicz wrote:
> I think that adding a url member to this class makes this class too
complicated.
> 
> The url member would either:
> - have to be set via the constructor. Both constructors will take two
parameters
> - ugly
> - the constructor would only set the bitmap data and there will be an
additional
> setter for the url. Having a setter is weird in my opinion. WebappInfo
populates
> all of its members in the constructor

Why is having a second parameter in the constructors ugly? Ideally, this class
wouldn't exist and WebappInfo would only store decodedIcon, but we want to avoid
the expensive image decode operation in WebappActivity#preInflationStartup,
which is where we initialize WebappInfo with a String encoded icon. So we need
this class as an internal detail to avoid that call.

WebappInfo would call new Icon(null, encodedIcon). Meanwhile, the WebApkInfo
constructor would take an explicit String bestIconUrl in create() and the
constructor, and call new Icon(bestIconUrl, icon). We get the advantage of not
having to pass around the separate bestIconUrl in the update code. Thoughts?

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:138:
String id = mInfo.id();
Nit: just inline id and bestIconMurmur2Hash in the nativeUpdateAsync call. No
need to have locals for them, it's not that much clearer.

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:26:
* Parameter for {@link WebappInfo#create()} method which allows either a Bitmap
or a PNG
Nit: "Bitmap or PNG encoded String"

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:268:
// This is needed for clients that want to send the icon trough an intent.
Nit: "through" (may as well fix it while you're here)

[pkotwicz, 2016-11-25 02:16:51]

Patchset #5 (id:160001) has been deleted

[pkotwicz, 2016-11-25 02:17:09]

Dominick, can you please take another look?

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130:
return !TextUtils.equals(bestIconMurmur2Hash, fetchedBestIconMurmur2Hash)
After thinking about this some more, I think we should not move this function
into WebApkInfo. This function uses knowledge about the implementation about how
the WebApkInfo is fetched in order to compare the hashes

The implementation of this function would change if we decided to fetch all of
the icons in the Web Manifest in ManifestUpgradeDetectorFetcher

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:64:
PackageManager packageManager =
ContextUtils.getApplicationContext().getPackageManager();
I don't think that what happens when we can't get resources matters. Not being
able to get the WebAPK's resources should not happen.

In particular, being able to get meta data for a WebAPK but not its resources
should not happen

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:29:
public static class Icon {
Currently, we don't cache the "best icon URL" in the WebAPK's meta data so it
does not make sense to store the best icon URL in WebApkInfo

We likely should cache the "best icon URL" in the WebAPK's meta data. See my
comments in https://codereview.chromium.org/2528073002/
I would rather address your comment if we choose to cache the "best icon URL" in
the WebAPK's meta data

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2459023002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:138:
String id = mInfo.id();
Changed

I have also changed ManifestUpgradeDetectorFetcher to fill in any fields in the
new WebApkInfo that it did not fetch such as the "deep link URL", "reason the
WebAPK was launched" and the "Shell APK version" from the old WebApkInfo.

[dominickn, 2016-11-25 04:54:40]

lgtm

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:130:
return !TextUtils.equals(bestIconMurmur2Hash, fetchedBestIconMurmur2Hash)
On 2016/11/25 02:17:08, pkotwicz wrote:
> After thinking about this some more, I think we should not move this function
> into WebApkInfo. This function uses knowledge about the implementation about
how
> the WebApkInfo is fetched in order to compare the hashes
> 
> The implementation of this function would change if we decided to fetch all of
> the icons in the Web Manifest in ManifestUpgradeDetectorFetcher

Acknowledged. Thanks for the reasoning here.

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:64:
PackageManager packageManager =
ContextUtils.getApplicationContext().getPackageManager();
On 2016/11/25 02:17:08, pkotwicz wrote:
> I don't think that what happens when we can't get resources matters. Not being
> able to get the WebAPK's resources should not happen.
> 
> In particular, being able to get meta data for a WebAPK but not its resources
> should not happen

Right, but the method is still able to return null, and if it can return null,
we need to handle that sanely. If you're comfortable with the icon being blank
if the resources can't be fetched, then I won't block.

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:29:
public static class Icon {
On 2016/11/25 02:17:09, pkotwicz wrote:
> Currently, we don't cache the "best icon URL" in the WebAPK's meta data so it
> does not make sense to store the best icon URL in WebApkInfo
> 
> We likely should cache the "best icon URL" in the WebAPK's meta data. See my
> comments in https://codereview.chromium.org/2528073002/
> I would rather address your comment if we choose to cache the "best icon URL"
in
> the WebAPK's meta data

Alright, we can leave this for future work.

[pkotwicz, 2016-11-25 15:25:56]

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2459023002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:64:
PackageManager packageManager =
ContextUtils.getApplicationContext().getPackageManager();
I am fine with a blank icon

[pkotwicz, 2016-11-27 20:04:35]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-11-27 20:04:44]

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-11-27 21:33:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-27 21:33:10]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2016-11-28 00:02:42]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-11-28 00:02:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2459023002/#ps200001
(title: "Merge branch 'update_fail_refactor003' into update_fail_refactor01")

[commit-bot: I haz the power, 2016-11-28 00:02:51]

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-11-28 01:30:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 01:30:54]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominickn, 2016-11-28 02:15:39]

On 2016/11/28 01:30:54, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

It's a good idea to ask for a double check if you have to make nontrivial
changes to get tests to pass. Please run the tryjobs prior to requesting a
review, that will ensure that test failures are caught earlier.

[pkotwicz, 2016-11-28 02:34:58]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-11-28 02:35:14]

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-11-28 03:05:29]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480300498876830,
"parent_rev": "2ec9fadb3de4c6b57a20000c6fcf210cbff521ae", "commit_rev":
"850743e49727d6bd66e38fe05aa7cc76c0b9066d"}

[commit-bot: I haz the power, 2016-11-28 03:06:08]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-11-28 03:09:00]

Description was changed from

==========
Merge WebappInfo and WebApkMetaData part 2/2

This CL:
- Deletes WebApkMetaData
- Adds parameters in WebApkMetaData but not in WebApkInfo to WebApkInfo.
- Moves code from WebApkMetaDataUtils to WebApkInfo

BUG=659845
==========

to

==========
Merge WebappInfo and WebApkMetaData part 2/2

This CL:
- Deletes WebApkMetaData
- Adds parameters in WebApkMetaData but not in WebApkInfo to WebApkInfo.
- Moves code from WebApkMetaDataUtils to WebApkInfo

BUG=659845

Committed: https://crrev.com/cb48ebabe16d969057fc583c496729547a1b10d5
Cr-Commit-Position: refs/heads/master@{#434594}
==========

[commit-bot: I haz the power, 2016-11-28 03:09:01]

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

[pkotwicz, 2016-11-28 03:28:52]

I did make a trivial change to ManifestUpgradeDetectorFetcher.java (I reverted
one of the changes that I made in patch set #5) in order to get the tests to
pass. I will post a follow up CL which gets rid of the slight confusion caused
by this revert.

In writing this CL, I ran all of the WebAPK tests locally so I was pretty
confident that I would not break anything unexpected. That's the fun of big
refactors. Yes, I should make it a habit to run try jobs