Add web_manifest_url in WebappInfo.

Add web_manifest_url in WebappInfo.

When detecting web manifest resources changes, we compare the loaded
manifest url with the one baked in WebAPK's AndroidManifest.xml to see
whether they match.
See CL: https://codereview.chromium.org/2124513002/

BUG=624834
Committed: https://crrev.com/69d95c9d27e62badd7c88046ebf0507a9900c1c8
Cr-Commit-Position: refs/heads/master@{#406265}

[Xi Han, 2016-07-12 18:36:16]

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

[Xi Han, 2016-07-12 18:36:17]

Hi Peter, I make the plumbing of web_manifest_url as a separate CL, please take
a look. Thanks!

[pkotwicz, 2016-07-12 20:12:45]

LGTM!

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/l...
File
chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java
(right):

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/l...
chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:37:
private static final String META_DATA_WEB_MANFIEST_URL = "webManifestUrl";
Nit: META_DATA_WEB_MANFIEST_URL -> META_DATA_WEB_MANIFEST_URL

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/s...
File chrome/android/webapk/shell_apk/BUILD.gn (right):

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/BUILD.gn:15: # The Url of the web manifest file.
Nit: "web manifest" -> "Web Manifest"

[Xi Han, 2016-07-12 20:58:32]

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

[Xi Han, 2016-07-12 20:58:33]

Hi Dominick, please take a look, thanks!

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/l...
File
chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java
(right):

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/l...
chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/HostBrowserLauncher.java:37:
private static final String META_DATA_WEB_MANFIEST_URL = "webManifestUrl";
On 2016/07/12 20:12:45, pkotwicz wrote:
> Nit: META_DATA_WEB_MANFIEST_URL -> META_DATA_WEB_MANIFEST_URL

Done.

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/s...
File chrome/android/webapk/shell_apk/BUILD.gn (right):

https://codereview.chromium.org/2140273002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/BUILD.gn:15: # The Url of the web manifest file.
On 2016/07/12 20:12:45, pkotwicz wrote:
> Nit: "web manifest" -> "Web Manifest"

Done.

[dominickn, 2016-07-15 00:33:12]

There is now some inconsistency creeping in between WebappInfo and
WebappDataStorage, when previously it was mostly equivalent what was being
stored in each. Do you have a general framework for what information is and
isn't added in each location?

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

https://codereview.chromium.org/2140273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:330:
if (mWebManifestUrl != null) {
webManifestUrl() for consistency.

[Xi Han, 2016-07-15 02:23:40]

Generally we try to store anything that is non-critical in the SharedPreference,
so their changes won't request to re-mint a WebAPK. However, some properties are
so important that we can't restore them when user cleans Chrome's data, we have
to keep their latest values in the AnderoidManifest, and request re-mint if
their values changed.  For example, "web_manifest_url". Since we can always get
its latest value from the APK itself, we don't need to store it in the
SharedPreference.

PTAL, thanks!

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

https://codereview.chromium.org/2140273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:330:
if (mWebManifestUrl != null) {
On 2016/07/15 00:33:12, dominickn wrote:
> webManifestUrl() for consistency.

Done.

[dominickn, 2016-07-15 02:48:50]

Thanks for the explanation, it would be good to see comments in places like
WebappInfo and WebappDataStorage to note what lives:

 - in a WebAPK manifest + WebappInfo
 - in WebappDataStorage - WebappInfo
 - in all three (probably nothing?)

lgtm

[Xi Han, 2016-07-15 14:32:05]

On 2016/07/15 02:48:50, dominickn wrote:
> Thanks for the explanation, it would be good to see comments in places like
> WebappInfo and WebappDataStorage to note what lives:
> 
>  - in a WebAPK manifest + WebappInfo
>  - in WebappDataStorage - WebappInfo
>  - in all three (probably nothing?)
> 
> lgtm

That is a good idea, thanks! I look the properties in WebappInfo, and
web_manifest_url is the only one that not in WebappDataStorage, so I just put a
comment for it. Other properties like backgroundColor, even we don't rely on its
value in the WebappDataStorage, I didn't add such comments for that since it is
already in the WebappDataStorage.

Will add similar comment when inconsistency is introduced in the future.

[Xi Han, 2016-07-15 14:33:32]

Hi Dan, I need OWNERS' review for:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java

Could you please take a look? Thanks!

[Xi Han, 2016-07-15 14:34:02]

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

[Xi Han, 2016-07-15 14:34:03]

Hi Dan, I need OWNERS' review for:
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java

Could you please take a look? Thanks!

[Xi Han, 2016-07-15 15:18:20]

Patchset #5 (id:80001) has been deleted

[gone, 2016-07-15 21:11:04]

lgtm

https://chromiumcodereview.appspot.com/2140273002/diff/100001/chrome/android/...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://chromiumcodereview.appspot.com/2140273002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:37:
/** The Web Manifest URL lives in a WebAPK AndroidManifest and WebappInfo, not
in
Use correct javadoc syntax.

Either a full one:

/**
 * The Web Manifest URL lives in a WebAPK AndroidManifest and WebappInfo, but
not in
 * WebappDataStorage.
 */

Or a one-liner:

/** Stored in a WebAPK AndroidManifest and in WebappInfo, not in
WebappDataStorage. */

Don't know if hybrids are valid.

[pkotwicz, 2016-07-15 21:14:13]

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

https://codereview.chromium.org/2140273002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:39:
private String mWebManifestUrl;
Something I just noticed. Here |mWebManifestUrl| is a String but in
https://codereview.chromium.org/2147323002/ scope is a Uri. It would be nice if
both |mWebManifestUrl| and |mScopeUri| were of the same type (Not sure which
type is correct)

[Xi Han, 2016-07-18 13:44:09]

Patchset #6 (id:120001) has been deleted

[Xi Han, 2016-07-18 13:45:22]

Hi Peter, I changed the type of webManifestUrl from String to Uri, PTAL, thanks!

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

https://codereview.chromium.org/2140273002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:37:
/** The Web Manifest URL lives in a WebAPK AndroidManifest and WebappInfo, not
in
On 2016/07/15 21:11:04, dfalcantara wrote:
> Use correct javadoc syntax.
> 
> Either a full one:
> 
> /**
>  * The Web Manifest URL lives in a WebAPK AndroidManifest and WebappInfo, but
> not in
>  * WebappDataStorage.
>  */
> 
> Or a one-liner:
> 
> /** Stored in a WebAPK AndroidManifest and in WebappInfo, not in
> WebappDataStorage. */
> 
> Don't know if hybrids are valid.

Done.

https://codereview.chromium.org/2140273002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:39:
private String mWebManifestUrl;
On 2016/07/15 21:14:13, pkotwicz wrote:
> Something I just noticed. Here |mWebManifestUrl| is a String but in
> https://codereview.chromium.org/2147323002/ scope is a Uri. It would be nice
if
> both |mWebManifestUrl| and |mScopeUri| were of the same type (Not sure which
> type is correct)

Good catch, changed |mWebManifestUrl| to Uri.

[Xi Han, 2016-07-18 13:45:32]

The CQ bit was checked by hanxi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 13:45:48]

Dry run: 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-07-18 15:04:03]

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

[commit-bot: I haz the power, 2016-07-18 15:04:04]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-07-19 03:11:04]

still lgtm % nit.

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

https://codereview.chromium.org/2140273002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:175:
Uri webManifestUri = webManifestUrl != null ? Uri.parse(webManifestUrl) : null;
Nit: make this check the same as the one for scope in crrev.com/2147323002.

[Xi Han, 2016-07-19 12:59:11]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2016-07-19 12:59:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, dfalcantara@chromium.org, dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2140273002/#ps160001
(title: "Nits.")

[commit-bot: I haz the power, 2016-07-19 12:59:41]

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-07-19 13:51:14]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-07-19 13:51:27]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 13:52:57]

Description was changed from

==========
Add web_manifest_url in WebappInfo.

When detecting web manifest resources changes, we compare the loaded
manifest url with the one baked in WebAPK's AndroidManifest.xml to see
whether they match.
See CL: https://codereview.chromium.org/2124513002/

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

to

==========
Add web_manifest_url in WebappInfo.

When detecting web manifest resources changes, we compare the loaded
manifest url with the one baked in WebAPK's AndroidManifest.xml to see
whether they match.
See CL: https://codereview.chromium.org/2124513002/

BUG=624834

Committed: https://crrev.com/69d95c9d27e62badd7c88046ebf0507a9900c1c8
Cr-Commit-Position: refs/heads/master@{#406265}
==========

[commit-bot: I haz the power, 2016-07-19 13:52:59]

Patchset 7 (id:??) landed as
https://crrev.com/69d95c9d27e62badd7c88046ebf0507a9900c1c8
Cr-Commit-Position: refs/heads/master@{#406265}