Issue 2453423002: Send all of the icon URLs listed in Web Manifest to WebAPK Server.

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-10-27 21:26:26 UTC) #1

Xi Han

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

4 years, 1 month ago (2016-10-27 21:45:33 UTC) #2

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2453423002/1

4 years, 1 month ago (2016-10-27 21:46:40 UTC) #3

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-27 21:52:11 UTC) #4

commit-bot: I haz the power

Dry run: 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_clang_dbg_recipe/builds/154217) android_n5x_swarming_rel on ...

4 years, 1 month ago (2016-10-27 21:52:12 UTC) #5

Xi Han

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

4 years, 1 month ago (2016-10-28 19:24:01 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2453423002/40001

4 years, 1 month ago (2016-10-28 19:24:26 UTC) #9

Xi Han

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

4 years, 1 month ago (2016-10-28 19:45:54 UTC) #10

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-28 20:40:42 UTC) #12

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 1 month ago (2016-10-28 20:40:43 UTC) #13

pkotwicz

Nits and a few comments for follow up CLs https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java#newcode156 chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:156: ...

4 years, 1 month ago (2016-11-01 00:29:50 UTC) #14

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 15:52:18 UTC) #18

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

Chrome only sends the best icon url to WebAPK Minter Server to create/install
WebAPKs. However, the server wants to have all the icon URLs listed in the Web
Manifest sent. In this CL, the icon urls are added to ShortcutInfo, so they
can be sent to the WebAPK server. These icon URLs are stored as a <meta-data>
tag
in the WebAPK's AndroidManifest.xml. Besides, the change of the icon URLs will
request an update for the WebAPK.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

Chrome only sends the best icon url to WebAPK Minter Server to create/install
WebAPKs. However, the server wants to have all the icon URLs listed in the Web
Manifest sent. In this CL, all of the icon urls are added to ShortcutInfo, so
they can be sent to the WebAPK server.

However, in the multipe icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space seperated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server genereates all the icon hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 15:52:41 UTC) #19

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

Chrome only sends the best icon url to WebAPK Minter Server to create/install
WebAPKs. However, the server wants to have all the icon URLs listed in the Web
Manifest sent. In this CL, all of the icon urls are added to ShortcutInfo, so
they can be sent to the WebAPK server.

However, in the multipe icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space seperated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server genereates all the icon hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon url to WebAPK Minter Server to
create/install
WebAPKs. However, the server wants to have all the icon URLs listed in the Web
Manifest sent. In this CL, all of the icon urls are added to ShortcutInfo, so
they can be sent to the WebAPK server.

However, in the multipe icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space seperated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server genereates all the icon hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 15:53:59 UTC) #20

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon url to WebAPK Minter Server to
create/install
WebAPKs. However, the server wants to have all the icon URLs listed in the Web
Manifest sent. In this CL, all of the icon urls are added to ShortcutInfo, so
they can be sent to the WebAPK server.

However, in the multipe icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space seperated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server genereates all the icon hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added to
ShortcutInfo, so
they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 15:54:14 UTC) #21

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added to
ShortcutInfo, so
they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

Xi Han

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

4 years, 1 month ago (2016-11-07 16:41:35 UTC) #22

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2453423002/120001

4 years, 1 month ago (2016-11-07 16:41:51 UTC) #23

Xi Han

Hi Peter, I update this CL according to the decision to put all the icon ...

4 years, 1 month ago (2016-11-07 16:51:43 UTC) #24

Hi Peter, I update this CL according to the decision to put all the icon urls
and icon hashs in the AndroidManifest.xml. Please take another look, thanks!

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

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:156:
fetchedData.iconUrls = new HashSet<String>();
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: Use org.chromium.base.CollectionUtil.newHashSet()

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:175:
if (!urlsMatchIgnoringFragments(mMetaData.bestIconUrl, fetchedData.bestIconUrl)
On 2016/11/01 00:29:49, pkotwicz wrote:
> For a different CL
> 
> This check is not correct in a multiple icon world. The bestIconUrl is
different
> for different devices. When minting a WebAPK for
>
https://webapk-test.appspot.com/generated.html?name=Long%20Train%20%F0%9F%9A%...
> the best icon is different on an Android One device and on a Nexus 5 device.
> 
> ManifestUpgradeDetectorFetcher should get the hash for the icon which is the
> best icon according to the "Android Manifest" as opposed to recomputing the
best
> icon.

Good catch. As discussed offline, the best icon URL and hash are removed from
the metadata, and have all the icon URLs and hashs in.

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

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaData.java:26:
public String iconMurmur2Hash;
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nits:
> |iconId| -> |bestIconId|
> |iconMurmur2Hash| -> |bestIconMurmur2Hash|
> 
> Move |iconUrls| either above or below all of the "best" properties

Done.

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

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:152:
public static String convertIconUrlsToString(Set<String> iconUrlSet) {
On 2016/11/01 00:29:49, pkotwicz wrote:
> I can't find any calls to this function

Good catch, the caller was removed after I rebased this CL after the changes of
WebApkMetaData. Now it is called from WebApkMetaDataUtilsTest, so I moved it
there.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:167:
*/
On 2016/11/01 00:29:49, pkotwicz wrote:
> How about: "Extract the icon URLs from the WebAPK's meta data. The icon URLs
are
> stored as a space separated list in a single meta data tag in the WebAPK's
> AndroidManifest.xml."
> 
> My version:
> - Is more explicit about what this function does "Extract the icon URLs from
the
> WebAPK's meta data"
> - Provides more details about how the icon URLs are stored

Sounds better, thanks. Only one change: space -> {@link
WebApkConstants#ICON_URL_SEPARATOR}.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:168:
public static Set<String> getIconUrlsFromString(String iconUrls) {
On 2016/11/01 00:29:49, pkotwicz wrote:
> Can you please modify WebApkMetaDataUtils#testSanity() to check parsing the
meta
> data tag

Good idea. I add a separate test since the icon urls aren't in WebappInfo.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:173:
iconUrlSet.addAll(Arrays.asList(urls));
On 2016/11/01 00:29:49, pkotwicz wrote:
> I think you can use CollectionUtil.newHashSet()

Done.

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

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:166:
iconUrls.toArray(new String[iconUrls.size()]), displayMode, orientation,
themeColor,
On 2016/11/01 00:29:49, pkotwicz wrote:
> Is this better:
> iconUrls.toArray(new String[0])

I am not sure which one is preferred, since both of them are used in chromium's
code base.

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

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:103:
mFetchedData.iconUrls.toArray(new String[mFetchedData.iconUrls.size()]),
On 2016/11/01 00:29:49, pkotwicz wrote:
> Is this more correct:
> mFetchedData.iconUrls.toArray(new String[0])

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/webapk/l...
File
chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java
(right):

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/webapk/l...
chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:26:
public static final String ICON_URL_SEPARATOR = "; ";
On 2016/11/01 00:29:49, pkotwicz wrote:
> Can the separator be just ' '?
> 
> I don't think that much is gained by making the separator "; " instead of " ".
> Both
> https://www.google.com/a b
> https://www.google.com/a; b
> are valid URLs
> 
> This CL puts a requirement on the WebAPK server to escape ' ' as %20
> The WebAPK server should not do any other escaping. In particular, Chrome has
> some very special handling for "https://www.google.com/%2%32"
> Chrome escapes "https://www.google.com/%2%32" as
"https://www.google.com/%2522"

We definitely could only use a space, since space is the only one that I find
shouldn't be in a valid URL. Yes, server will prepare such a metadata tag and
store it in the AndroidManifest.xml. Since Chrome canonicalizes all the URLs and
Serve just uses them and add a space between two icon URLs, we could handle it
correct when parsing the metadata.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/webapk/s...
File chrome/android/webapk/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/AndroidManifest.xml:49: <meta-data
android:name="org.chromium.webapk.shell_apk.iconId"
android:resource="@mipmap/app_icon" />
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: rename iconId -> bestIconId

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/AndroidManifest.xml:51: <meta-data
android:name="org.chromium.webapk.shell_apk.iconUrls" android:value="{{
icon_urls }}" />
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: Move this line before all of the "best" <meta-data> tags or after all of
> the "best" <meta-data> tags

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/AndroidManifest.xml:52: <meta-data
android:name="org.chromium.webapk.shell_apk.iconMurmur2Hash" android:value="{{
icon_murmur2_hash }}" />
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: Rename iconMurmur2Hash -> bestIconMurmur2Hash

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_info.cc (right):

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:67: // Set the icon urls based on the
icons on the manifest, if any.
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: "on the manifest" -> "in the manifest"

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_info.cc:69: for (const auto& icon :
manifest.icons)
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: "const auto& icon" -> "const Icon& icon"

content::Manifest::Icon, personally I would prefer to use "auto".

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right):

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:162: const
std::string& icon_murmur2_hash) {
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: Rename |icon_murmur2_hash| to |best_icon_murmur2_hash|

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:171:
OnDataAvailable(info_, icon_murmur2_hash, icon_);
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: |icon_| -> |best_icon_|

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:193:
gfx::ConvertToJavaBitmap(&icon_bitmap);
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: |java_bitmap| -> |java_best_bitmap|

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:196: for
(const auto& icon_url : info.icon_urls)
On 2016/11/01 00:29:49, pkotwicz wrote:
> Nit: "const auto& icon_url" -> "const GURL& icon_url"

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:119: webapk::Image* image =
web_app_manifest->add_icons();
On 2016/11/01 00:29:50, pkotwicz wrote:
> Nit: Rename |image| -> |best_image|

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:126: for (const auto& icon_url
: shortcut_info.icon_urls) {
On 2016/11/01 00:29:50, pkotwicz wrote:
> Nit: "const auto& icon_url" -> "const GURL& icon_url"

Done.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:131: }
On 2016/11/01 00:29:49, pkotwicz wrote:
> Can you check with Glenn whether reordering is ok? When I talked to Glenn
> offline about something unrelated he mentioned that the reordering might be
> suboptimial. (I guess we could asssume that reordering is OK and fix it once
we
> know whether it actually is ok)

I talked to Glenn and he prefers to have the best one as the first, and others
as ordered in the Web Manifest. Since we didn't change the orders of the urls
except the best, I assume it should be just fine.

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_update_manager.cc (right):

https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_manager.cc:56: const
JavaParamRef<jobject>& java_icon_bitmap,
On 2016/11/01 00:29:50, pkotwicz wrote:
> Nit:
> |java_icon_murmur2_hash| -> |java_best_icon_murmur2_hash|
> |java_icon_bitmap| -> |java_best_icon_bitmap|

Done.

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 16:52:17 UTC) #25

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

The metadata changes requires some behavior changes:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

Some behavior changes involved:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 16:53:55 UTC) #26

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

Some behavior changes involved:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

Internally, the icon URL and icon Hash pairs are stored in a map.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-07 16:54:26 UTC) #27

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

Internally, the icon URL and icon Hash pairs are stored in a map.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map underneath the code.

BUG=627593
==========

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 17:50:27 UTC) #28

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 1 month ago (2016-11-07 17:50:28 UTC) #29

pkotwicz

A few more comments. Mostly nits. Sorry for taking so long to review the CL ...

4 years, 1 month ago (2016-11-11 21:04:20 UTC) #30

Xi Han

PTAL, thanks! https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/shortcut_info.cc File chrome/browser/android/shortcut_info.cc (right): https://codereview.chromium.org/2453423002/diff/40001/chrome/browser/android/shortcut_info.cc#newcode69 chrome/browser/android/shortcut_info.cc:69: for (const auto& icon : manifest.icons) On ...

4 years, 1 month ago (2016-11-14 19:36:10 UTC) #34

pkotwicz

LGTM with nits https://codereview.chromium.org/2453423002/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java (right): https://codereview.chromium.org/2453423002/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java#newcode127 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:127: * single meta data tag in ...

4 years, 1 month ago (2016-11-14 22:43:11 UTC) #35

Xi Han

Hi Peter, I merge the icon urls and murmur2 hash into one single metadata tag, ...

4 years, 1 month ago (2016-11-15 20:07:08 UTC) #42

Xi Han

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

4 years, 1 month ago (2016-11-15 20:27:22 UTC) #43

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2453423002/340001

4 years, 1 month ago (2016-11-15 20:27:55 UTC) #44

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-15 20:41:24 UTC) #45

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/260950)

4 years, 1 month ago (2016-11-15 20:41:25 UTC) #46

pkotwicz

Still LGTM https://codereview.chromium.org/2453423002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java (right): https://codereview.chromium.org/2453423002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java#newcode130 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:130: * WebAPK's AndroidManifest.xml as followings: Nit: followings ...

4 years, 1 month ago (2016-11-15 22:04:47 UTC) #47

Xi Han

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

4 years, 1 month ago (2016-11-15 22:33:29 UTC) #48

Xi Han

Hi Dominick, could you please take a look? Thanks! https://codereview.chromium.org/2453423002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java (right): https://codereview.chromium.org/2453423002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java#newcode130 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:130: ...

4 years, 1 month ago (2016-11-15 22:33:30 UTC) #49

dominickn

Why is it necessary for all of the icon URLs to match for an upgrade ...

4 years, 1 month ago (2016-11-17 02:34:39 UTC) #50

Why is it necessary for all of the icon URLs to match for an upgrade to not be
triggered? That's a functional change from before where you only update if they
best icon + hash doesn't match.

Put another way, if you check the manifest, and the best icon is still exactly
the same as the one you currently have, and all the other details match, why do
you need to upgrade? If the WebAPK server is now minting a WebAPK with different
icons in it, but the specific icon you use is the same, do you need to upgrade?

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

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:63:
public Set<String> iconUrls;
Why does this need to be a set? It looks like it's an array elsewhere.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java
(right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:24:
void onGotManifestData(String startUrl, String scopeUrl, String name, String
shortName,
It's really confusing that this onGotManifestData takes lots of arguments, but
the onGotManifestData in WebApkUpdateManager just takes the FetchedManifestData.

I really think that all of them should take a WebApkInfo object and that should
have everything in it. The only time you should have an argument list this long
is for a CalledByNative method. Otherwise, we should use objects to encapsulate
the arguments.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
(right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:31:
private static final String TAG = "cr_WebApkMetaData";
Nit: newline after this.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:56:
metaData.backgroundColor, false, webApkPackageName);
Should this be metaData.iconUrlAndIconMurmur2HashMap.isEmpty() or something like
that?

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:142:
if (TextUtils.isEmpty(iconUrl)) {
Reverse this conditional, move line 145 inside, and remove line 143.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:151:
String[] urlsAndHashes = iconUrlsAndIconMurmur2Hashes.split("[ ]+");
Is it ever possible for a URL to be printed that contains a space? Or (even
worse) two spaces? Can this be tested in some way?

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

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:166:
String bestIconUrl, String bestIconMurmur2Hash, Bitmap bestIcon, Set<String>
iconUrls,
This shouldn't be a set.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:170:
bestIconMurmur2Hash, bestIcon, iconUrls.toArray(new String[0]), displayMode,
Declare String[] iconUrlsArray = new String[iconUrls.size()]. Then call
iconUrls.toArray(iconUrlsArray). Relying on the JVM to implicitly resize the
array is magical (and seeing String[0] is just weird).

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:57: std::vector<GURL> icon_urls;
Why is this a vector but it's a set in Java?

Also, I'm not a big fan of the "best" terminology. I'll think about it
overnight...

Also, is there any need for this to be a container of GURLs? It looks like
everything that accesses this member calls spec() on each object (or starts with
a string and has to call GURL()). Why not have a container of strings?

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-17 16:47:50 UTC) #51

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, the server wants to have all the icon URLs
listed in the Web Manifest sent. In this CL, all of the icon URLs are added
to ShortcutInfo, so they can be sent to the WebAPK server.

However, in the multiple icon world, it isn't correct to still keep the "best
icon URL" and the "best icon hash" in AndroidManifest.xml. This is because the
best icon URL will be different for devices with different resolution. For
example, a nexus 5 and an Android one device will have different best icon URL.
To prevent creating a WebAPK for a device with a wrong baked in metadata, the
"best icon URL" and "best Icon Hash" are removed from the metadata. Instead,
all of the icon URLs, as well as all of the icon Hashs are baked in the
AndroidManifest.xml. These icon URLs are stored as a space separated single
<meta-data> tag, and so do the icon Hashs.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + the best icon URL + best
icon hash to the Server, and Server generates all the icon Hashs and bakes
them in the WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map underneath the code.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked in metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the icon images.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which uses the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-17 16:48:42 UTC) #52

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked in metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the icon images.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which uses the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the icon images.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which uses the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-17 16:49:30 UTC) #53

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the icon images.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which uses the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which uses the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

Xi Han

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-17 16:50:04 UTC) #54

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which uses the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which use the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

Xi Han

I change the update rule for icons: Chrome only sends update request if the best ...

4 years, 1 month ago (2016-11-17 18:09:58 UTC) #55

I change the update rule for icons: Chrome only sends update request if the best
icon url + hash changed. We will not consider other icons. The only concern is
if Android picks a different icon, we may fail to update. But I don't think this
is a big concern. I also update the CL description, and hope it helps. PTAL,
thanks!

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

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:63:
public Set<String> iconUrls;
On 2016/11/17 02:34:38, dominickn wrote:
> Why does this need to be a set? It looks like it's an array elsewhere.

It is because the order of the URLs doesn't matter. So if only the order
changes, we should treat as the URLs didn't change. When using a set, it would
be easy to do such a comparison without worrying about the order of the
elements.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java
(right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:24:
void onGotManifestData(String startUrl, String scopeUrl, String name, String
shortName,
On 2016/11/17 02:34:38, dominickn wrote:
> It's really confusing that this onGotManifestData takes lots of arguments, but
> the onGotManifestData in WebApkUpdateManager just takes the
FetchedManifestData.
I haven't thought about it before, but it does make sense to me. Move the
generation of the FetchedManifestData from ManifestUpgradeDetector to
ManifestUpgradeDetectorFetcher.
 

> I really think that all of them should take a WebApkInfo object and that
should
> have everything in it. The only time you should have an argument list this
long
> is for a CalledByNative method. Otherwise, we should use objects to
encapsulate
> the arguments.
The WebApkInfo and FetchedManifestData are slightly different. For example,
WebApkInfo have encodedIcon, while we need to store the bestIconUrl and
bestIconHash in the FetchedManifestData. Personally I would prefer to use
FetchedManifestData here.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
(right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:31:
private static final String TAG = "cr_WebApkMetaData";
On 2016/11/17 02:34:38, dominickn wrote:
> Nit: newline after this.

Done.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:56:
metaData.backgroundColor, false, webApkPackageName);
On 2016/11/17 02:34:38, dominickn wrote:
> Should this be metaData.iconUrlAndIconMurmur2HashMap.isEmpty() or something
like
> that?
Good catch!

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:142:
if (TextUtils.isEmpty(iconUrl)) {
On 2016/11/17 02:34:38, dominickn wrote:
> Reverse this conditional, move line 145 inside, and remove line 143.

Done.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:151:
String[] urlsAndHashes = iconUrlsAndIconMurmur2Hashes.split("[ ]+");
On 2016/11/17 02:34:38, dominickn wrote:
> Is it ever possible for a URL to be printed that contains a space? Or (even
> worse) two spaces? Can this be tested in some way?

The server doesn't store the original icon URLs from the web manifest in the
WebAPK's AndroidManifest, but the canonicalized icon URLs sent from Chrome.
Because Chrome canonicalizes all the URLs before using them, so if we want to
know whether the URLs change or not, we need to compare them with previously
canonicalized ones.

Filed a bug to add a test in a separate CL.

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

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:166:
String bestIconUrl, String bestIconMurmur2Hash, Bitmap bestIcon, Set<String>
iconUrls,
On 2016/11/17 02:34:38, dominickn wrote:
> This shouldn't be a set.

As mentioned before, using Set can ignore the order changes of the URLs. Keep a
Set here is just for consistency.

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:170:
bestIconMurmur2Hash, bestIcon, iconUrls.toArray(new String[0]), displayMode,
On 2016/11/17 02:34:38, dominickn wrote:
> Declare String[] iconUrlsArray = new String[iconUrls.size()]. Then call
> iconUrls.toArray(iconUrlsArray). Relying on the JVM to implicitly resize the
> array is magical (and seeing String[0] is just weird).

Done.

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:57: std::vector<GURL> icon_urls;
On 2016/11/17 02:34:38, dominickn wrote:
> Why is this a vector but it's a set in Java?
In c++, we only need to get these URLs; while in Java, we need to compare them
with the ones backed in WebAPK's metadata to see whether they changed or not.

 
> Also, I'm not a big fan of the "best" terminology. I'll think about it
> overnight... 
All right.
 
> Also, is there any need for this to be a container of GURLs? It looks like
> everything that accesses this member calls spec() on each object (or starts
with
> a string and has to call GURL()). Why not have a container of strings?
It looks like every variable which is called ".._url" is declared as GURL in
c++, I think it's better to keep this conventions.

dominickn

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java#newcode63 chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:63: public Set<String> iconUrls; On 2016/11/17 18:09:58, Xi Han wrote: ...

4 years, 1 month ago (2016-11-17 18:50:54 UTC) #56

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

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:63:
public Set<String> iconUrls;
On 2016/11/17 18:09:58, Xi Han wrote:
> On 2016/11/17 02:34:38, dominickn wrote:
> > Why does this need to be a set? It looks like it's an array elsewhere.
> 
> It is because the order of the URLs doesn't matter. So if only the order
> changes, we should treat as the URLs didn't change. When using a set, it would
> be easy to do such a comparison without worrying about the order of the
> elements.

But we're not doing a comparison with this anymore correct? Probably best to
just make it a primitive array or something like that, since it's passed in as a
primitive array.

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:57: std::vector<GURL> icon_urls;
On 2016/11/17 18:09:58, Xi Han wrote:
> > Also, is there any need for this to be a container of GURLs? It looks like
> > everything that accesses this member calls spec() on each object (or starts
> with
> > a string and has to call GURL()). Why not have a container of strings?
> It looks like every variable which is called ".._url" is declared as GURL in
> c++, I think it's better to keep this conventions.

You've actually broken this convention in manifest_upgrade_detector_fetcher.cc,
with std::vector<std::string> icon_urls. ;)

There are plenty of cases where we have some url member which isn't a GURL type
and I think this makes sense as one of them. It will save extra string <-> GURL
type conversions in webapk_update_manager, webapk_installer, and
manifest_upgrade_detector_fetcher so it's definitely worth having.

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:47:
public Set<String> iconUrls;
As pointed out above, this isn't being used in an equality comparison any more,
so it can just be a primitive array so you don't have the memory + construction
overhead of a set. You might need it to be final to make FindBugs happy.

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:155:
if (urlsAndHashes.length % 2 != 0) {
This is slighter nicer as:

if (urlsAndHashes.length % 2 == 0) {
    for (int i = 0; i < urlsAndHashes.length; i += 2) {
         iconUrlAndIconMurmur2HashMap.put(urlsAndHashes[i], urlsAndHashes[i +
1]);
    }
} else {
    Log.e(TAG, "Icon URLs and hashes were not in pairs.");
}
return iconUrlAndIconMurmur2HashMap;

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

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:166:
String bestIconUrl, String bestIconMurmur2Hash, Bitmap bestIcon, Set<String>
iconUrls,
This can just take a String[] iconUrls once you change the type in
FetchedManifestData.

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:297:
public void testIconUrlsChangeShouleNotUpgradeIfTheBestIconUrlDoesNotChange() {
Nit: "ShouldNotUpgrade"

Xi Han

PTAL, thanks! https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java#newcode63 chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:63: public Set<String> iconUrls; On 2016/11/17 18:50:53, dominickn ...

4 years, 1 month ago (2016-11-17 20:42:38 UTC) #58

PTAL, thanks!

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

https://codereview.chromium.org/2453423002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:63:
public Set<String> iconUrls;
On 2016/11/17 18:50:53, dominickn wrote:
> On 2016/11/17 18:09:58, Xi Han wrote:
> > On 2016/11/17 02:34:38, dominickn wrote:
> > > Why does this need to be a set? It looks like it's an array elsewhere.
> > 
> > It is because the order of the URLs doesn't matter. So if only the order
> > changes, we should treat as the URLs didn't change. When using a set, it
would
> > be easy to do such a comparison without worrying about the order of the
> > elements.
> 
> But we're not doing a comparison with this anymore correct? Probably best to
> just make it a primitive array or something like that, since it's passed in as
a
> primitive array.

Done.

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
File chrome/browser/android/shortcut_info.h (right):

https://codereview.chromium.org/2453423002/diff/360001/chrome/browser/android...
chrome/browser/android/shortcut_info.h:57: std::vector<GURL> icon_urls;
On 2016/11/17 18:50:53, dominickn wrote:
> On 2016/11/17 18:09:58, Xi Han wrote:
> > > Also, is there any need for this to be a container of GURLs? It looks like
> > > everything that accesses this member calls spec() on each object (or
starts
> > with
> > > a string and has to call GURL()). Why not have a container of strings?
> > It looks like every variable which is called ".._url" is declared as GURL in
> > c++, I think it's better to keep this conventions.
> 
> You've actually broken this convention in
manifest_upgrade_detector_fetcher.cc,
> with std::vector<std::string> icon_urls. ;)
> 
> There are plenty of cases where we have some url member which isn't a GURL
type
> and I think this makes sense as one of them. It will save extra string <->
GURL
> type conversions in webapk_update_manager, webapk_installer, and
> manifest_upgrade_detector_fetcher so it's definitely worth having.

Done.

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:47:
public Set<String> iconUrls;
On 2016/11/17 18:50:53, dominickn wrote:
> As pointed out above, this isn't being used in an equality comparison any
more,
> so it can just be a primitive array so you don't have the memory +
construction
> overhead of a set. You might need it to be final to make FindBugs happy.

Change it to String[]. We have some tests that modify the iconUrls to test
whether update is needed, how about not making it as final for now?

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkMetaDataUtils.java:155:
if (urlsAndHashes.length % 2 != 0) {
On 2016/11/17 18:50:53, dominickn wrote:
> This is slighter nicer as:
> 
> if (urlsAndHashes.length % 2 == 0) {
>     for (int i = 0; i < urlsAndHashes.length; i += 2) {
>          iconUrlAndIconMurmur2HashMap.put(urlsAndHashes[i], urlsAndHashes[i +
> 1]);
>     }
> } else {
>     Log.e(TAG, "Icon URLs and hashes were not in pairs.");
> }
> return iconUrlAndIconMurmur2HashMap;

Personally I would prefer early exist, but if you have strong preference, I can
change it too.

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

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:166:
String bestIconUrl, String bestIconMurmur2Hash, Bitmap bestIcon, Set<String>
iconUrls,
On 2016/11/17 18:50:53, dominickn wrote:
> This can just take a String[] iconUrls once you change the type in
> FetchedManifestData.

Done.

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:297:
public void testIconUrlsChangeShouleNotUpgradeIfTheBestIconUrlDoesNotChange() {
On 2016/11/17 18:50:53, dominickn wrote:
> Nit: "ShouldNotUpgrade"

Done.

dominickn

Just a couple of things. https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java (right): https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java#newcode47 chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:47: public Set<String> iconUrls; On ...

4 years, 1 month ago (2016-11-17 21:16:44 UTC) #59

Xi Han

PTAL, thanks! https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java (right): https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java#newcode47 chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:47: public Set<String> iconUrls; On 2016/11/17 21:16:43, dominickn ...

4 years, 1 month ago (2016-11-17 21:34:10 UTC) #60

PTAL, thanks!

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java
(right):

https://codereview.chromium.org/2453423002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:47:
public Set<String> iconUrls;
On 2016/11/17 21:16:43, dominickn wrote:
> On 2016/11/17 20:42:38, Xi Han wrote:
> > On 2016/11/17 18:50:53, dominickn wrote:
> > > As pointed out above, this isn't being used in an equality comparison any
> > more,
> > > so it can just be a primitive array so you don't have the memory +
> > construction
> > > overhead of a set. You might need it to be final to make FindBugs happy.
> > 
> > Change it to String[]. We have some tests that modify the iconUrls to test
> > whether update is needed, how about not making it as final for now?
> 
> If FindBugs doesn't complain, that's fine by me. I've recently sent a CL
through
> with a array where it was unhappy at the modifiability is all. :)

Acknowledged.

https://codereview.chromium.org/2453423002/diff/440001/chrome/browser/android...
File chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc (right):

https://codereview.chromium.org/2453423002/diff/440001/chrome/browser/android...
chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc:195:
std::vector<std::string> icon_urls;
On 2016/11/17 21:16:43, dominickn wrote:
> Can this just be:
> 
> ScopedJavaLocalRef<jobjectArray> java_icon_urls =
> base::android::ToJavaArrayOfStrings(env, info.icon_urls);

Good catch, thanks!

https://codereview.chromium.org/2453423002/diff/440001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2453423002/diff/440001/chrome/browser/android...
chrome/browser/android/webapk/webapk_installer.cc:155: if
(icon_url.compare(shortcut_info.best_icon_url.spec()) == 0)
On 2016/11/17 21:16:43, dominickn wrote:
> Nit: use ==

Done.

https://codereview.chromium.org/2453423002/diff/440001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_update_manager.cc (right):

https://codereview.chromium.org/2453423002/diff/440001/chrome/browser/android...
chrome/browser/android/webapk/webapk_update_manager.cc:92:
base::android::JavaArrayOfByteArrayToStringVector(
On 2016/11/17 21:16:43, dominickn wrote:
> Can this just be
> 
> base::android::JavaArrayOfByteArrayToStringVector(env, java_icon_urls.obj(),
> &info.icon_urls);

Done.

dominickn

On 2016/11/17 21:35:36, dominickn wrote: > lgtm Thanks for bearing with me!

4 years, 1 month ago (2016-11-17 21:36:02 UTC) #62

Xi Han

The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the ...

4 years, 1 month ago (2016-11-18 13:49:53 UTC) #64

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 13:50:04 UTC) #65

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 15:20:37 UTC) #66

commit-bot: I haz the power

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_android_rel_ng/builds/183663)

4 years, 1 month ago (2016-11-18 15:20:38 UTC) #67

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 15:41:30 UTC) #69

commit-bot: I haz the power

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-18 16:23:29 UTC) #70

Message was sent while issue was closed.

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which use the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which use the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

commit-bot: I haz the power

Description was changed from ========== Send all of the icon URLs listed in Web Manifest ...

4 years, 1 month ago (2016-11-18 16:25:28 UTC) #72

Message was sent while issue was closed.

Description was changed from

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to 
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which use the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593
==========

to

==========
Send all of the icon URLs listed in Web Manifest to WebAPK Server.

So far, Chrome only sends the best icon URL to WebAPK Minter Server to
create/install WebAPKs. However, in the multiple icon world, it isn't correct
to still keep the "best icon URL" and the "best icon hash" in
AndroidManifest.xml. This is because the best icon URL will be different for
devices with different resolution. For example, a nexus 5 and an Android one
device will have different best icon URL. To prevent creating a WebAPK for
a device with a wrong baked metadata, the "best icon URL" and "best Icon
Hash" are removed from the metadata. Instead, all of the icon URLs, as well
as all of the icon Hashs are baked in the AndroidManifest.xml. These icon URLs
and Hashes are stored as a space separated single <meta-data> tag, as:
"URL1 hash1 URL2 hash2 URL3 hash3..."

Another change in this CL is to send all the icon URLs to the server, since
the server wants to check if any of these icon URLs changed before starting
a fetch of the raw icon data.

Some behavior changes are involved:
1) For creating a WebAPK, Chrome sends all icon URLs + (the best icon URL + best
icon hash + best icon raw image) to the Server. The reason to still send the
best
icon is for one-off WebAPKs which use the image from Chrome, since server will
fail
to fetch any of them. Server generates all the icon Hashs and bakes them in the
WebAPK's metadata in AndroidManifest.xml.
2) When checking updates, Chrome compute the best icon URL and hash when
fetching
the Web Manifest, and compare the new computed hash with the one backed in
the metadata to see whether the image is updated.

The icon URL and icon Hash pairs are stored in a map in WebApkMetaData.

BUG=627593

Committed: https://crrev.com/75ca9beb7e7fcd03feb23bb1e246be3ff5de6b6e
Cr-Commit-Position: refs/heads/master@{#433215}
==========

commit-bot: I haz the power

4 years, 1 month ago (2016-11-18 16:25:30 UTC) #73

Message was sent while issue was closed.

Patchset 9 (id:??) landed as
https://crrev.com/75ca9beb7e7fcd03feb23bb1e246be3ff5de6b6e
Cr-Commit-Position: refs/heads/master@{#433215}

Issue 2453423002: Send all of the icon URLs listed in Web Manifest to WebAPK Server. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Remove best_icon_url and best_icon_hash from metadata, but add all icon urls and icon hashs in. #

Patch Set 3 : pkotwicz@'s comments. #

Patch Set 4 : Store icon Urls and murmur2 hashes in one single metadata tag. #

Patch Set 5 : use space to separate icon_url and icon_hash. #

Patch Set 6 : dominickn@'s comments. #

Patch Set 7 : Remove icon urls comparison when checking updates. #

Patch Set 8 : Use String[] for icon urls. #

Patch Set 9 : Nits. #

Messages