Merge WebappInfo and WebApkMetaData part 1/2

Merge WebappInfo and WebApkMetaData part 1/2

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
    in WebappInfo.
  - WebApkInfo#create() creates a WebApkInfo from an intent. Less intent
    properties need to be read to create a WebApkInfo object than a WebappInfo
    object.
- Passes WebApkInfo to WebApkUpdateManager

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

[pkotwicz, 2016-10-27 03:13:42]

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

[pkotwicz, 2016-10-27 03:13:43]

Dominick, can you please take a look? I will fix up the tests once you are happy
with the structure of the CL.

This CL depends on https://codereview.chromium.org/2452233002/

[pkotwicz, 2016-10-27 17:41:21]

Description was changed from

==========
Merge WebappInfo and WebApkMetaData part 1

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
in WebappInfo.
- Introduces WebappInfoFactory and WebApkInfoFactory which create WebappInfo and
WebApkInfo respectively from an Intent. Less intent properties need to be read
to create a WebApkInfo object than a WebappInfo object.
- Passes WebApkInfo to WebApkUpdateManager

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

to

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

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
    in WebappInfo.
  - WebApkInfo#create() creates a WebApkInfo from an intent. Less intent
    properties need to be read to create a WebApkInfo object than a WebappInfo
    object.
- Passes WebApkInfo to WebApkUpdateManager

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

[pkotwicz, 2016-10-27 17:48:49]

Dominick, can you please take a look?

I have removed WebappFactory and WebApkFactory as you have suggested.
I have also duplicated the null check in WebappInfo#create() and
WebApkInfo#create()

[pkotwicz, 2016-10-28 02:21:20]

Patchset #3 (id:40001) has been deleted

[dominickn, 2016-10-28 02:54:06]

Looks pretty good. Can you delete WebApkMetaData.java? Otherwise some fairly
small comments.

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

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:20:
/** ManifestUpgradeDetector callback. */
Nit: the comment isn't really that descriptive given the interface is called
Callback and it's inside ManifestUpgradeDetector. Remove?

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
(right):

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:146:
mUpdateManager.updateIfNeeded(getActivityTab(), (WebApkInfo) mWebappInfo);
As noted in the other comment, I don't think you need to cast this because
updateIfNeeded only needs to call WebappInfo's public interface (and
polymorphism should take care of the rest).

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

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:71:
"Data passed in was incomplete: " + id + ", " + url + ", " + webApkPackageName);
Minor nit: change string to "Incomplete data provided:" so that the log fits on
one line.

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

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:62:
public void updateIfNeeded(Tab tab, WebApkInfo info) {
Strictly speaking, this is only using properties in WebappInfo right? I don't
think you need to change this type.

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:120:
Log.e("WebappInfo", "Data passed in was incomplete: " + id + ", " + url);
Nit: Use TAG.

Minor nit: change the string to "Incomplete data provided:" to match WebApkInfo
suggestion.

[pkotwicz, 2016-10-28 14:54:39]

I delete WebApkMetaData in part 2 https://codereview.chromium.org/2459023002/

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

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:20:
/** ManifestUpgradeDetector callback. */
I think this is an incorrect diff. I added this line in a previous CL because
presubmit now fails on lint errors. I am going to refactor the callback class
soon (and rename it to observer).

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

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:71:
"Data passed in was incomplete: " + id + ", " + url + ", " + webApkPackageName);
On 2016/10/28 02:54:06, dominickn wrote:
> Minor nit: change string to "Incomplete data provided:" so that the log fits
on
> one line.

Done.

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

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:62:
public void updateIfNeeded(Tab tab, WebApkInfo info) {
In part 2, I will be using the properties of WebApkInfo. I purposefully changed
the type in this CL because it has implications for WebappInfo and WebApkInfo.

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2457663002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:120:
Log.e("WebappInfo", "Data passed in was incomplete: " + id + ", " + url);
On 2016/10/28 02:54:06, dominickn wrote:
> Nit: Use TAG.
> 
> Minor nit: change the string to "Incomplete data provided:" to match
WebApkInfo
> suggestion.

Done.

[dominickn, 2016-10-31 03:24:09]

Nearly there.

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:55:
boolean isValidWebApk = isValidWebApk(intent);
Nit: don't shadow a method with a variable name, change the variable name.

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(left):

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:287:
public void testIntentWebApkPackageName() {
You should probably put this packageName test in WebApkInfoTest.

[pkotwicz, 2016-10-31 04:09:12]

Dominick, can you please take another look?

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(left):

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:287:
public void testIntentWebApkPackageName() {
This is already tested by WebApkInfoTest#testSanity()

[dominickn, 2016-10-31 04:15:57]

lgtm

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java
(left):

https://codereview.chromium.org/2457663002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:287:
public void testIntentWebApkPackageName() {
On 2016/10/31 04:09:12, pkotwicz wrote:
> This is already tested by WebApkInfoTest#testSanity()

Hmm, so it is. It's a bit hidden but it is being tested.

[pkotwicz, 2016-11-01 00:30:55]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-11-01 00:31:06]

The CQ bit was unchecked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-11-01 00:31:14]

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

[pkotwicz, 2016-11-01 00:32:49]

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

[pkotwicz, 2016-11-01 00:32:49]

dfalcantara@ for files that Dominick is not an OWNER of

[gone, 2016-11-01 19:51:32]

lgtm

[pkotwicz, 2016-11-01 21:16:06]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-11-01 21:16:48]

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-01 22:55:54]

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

[commit-bot: I haz the power, 2016-11-01 22:55:56]

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

[pkotwicz, 2016-11-02 19:05:26]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-11-02 19:05:26]

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

[commit-bot: I haz the power, 2016-11-02 19:06:08]

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-02 20:04:07]

Description was changed from

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

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
    in WebappInfo.
  - WebApkInfo#create() creates a WebApkInfo from an intent. Less intent
    properties need to be read to create a WebApkInfo object than a WebappInfo
    object.
- Passes WebApkInfo to WebApkUpdateManager

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

to

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

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
    in WebappInfo.
  - WebApkInfo#create() creates a WebApkInfo from an intent. Less intent
    properties need to be read to create a WebApkInfo object than a WebappInfo
    object.
- Passes WebApkInfo to WebApkUpdateManager

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

[commit-bot: I haz the power, 2016-11-02 20:04:08]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-02 20:07:23]

Description was changed from

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

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
    in WebappInfo.
  - WebApkInfo#create() creates a WebApkInfo from an intent. Less intent
    properties need to be read to create a WebApkInfo object than a WebappInfo
    object.
- Passes WebApkInfo to WebApkUpdateManager

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

to

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

This CL:
- Introduces a new class WebApkInfo which extends WebappInfo
  - WebApkInfo will contain the properties which are in WebApkMetaData but not
    in WebappInfo.
  - WebApkInfo#create() creates a WebApkInfo from an intent. Less intent
    properties need to be read to create a WebApkInfo object than a WebappInfo
    object.
- Passes WebApkInfo to WebApkUpdateManager

BUG=659845

Committed: https://crrev.com/560d9b079835e22e14ed118b72840f8b096cda85
Cr-Commit-Position: refs/heads/master@{#429378}
==========

[commit-bot: I haz the power, 2016-11-02 20:07:24]

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