Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528073002:
Add a flag in WebAPK's proto when the Web App Manifest is no longer available. (Closed)
DescriptionAdd a flag in WebAPK's proto when the Web App Manifest is no longer
available.
A special case of WebAPK's update is: the Web Manifest of a WebAPK has
been removed from its site and the shell version of this WebAPK is
out-of-date. We want to force such an update, so needs to add a new
flag in the proto to indicate this special update request.
On the server side, WebAPK server will look for the cached resources of
the given package name, and create a updated WebAPK with the latest
shell. However, one-off WebAPKs will be cleaned up from the server if
there isn't any request within a period of time. For this very rare case,
we will send all the icon URLs an icon hashs to allow server create an
updated one-off WebAPKs with the same AndroidManfiest.xml.
BUG=665078
Committed: https://crrev.com/a98b72dc3235a7bf230877beafc9b3d131b311f7
Cr-Commit-Position: refs/heads/master@{#440420}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Pass the stale_manifest flag from Java. #Patch Set 3 : Send icon hashs. #Patch Set 4 : Fix the compile error. #
Total comments: 6
Patch Set 5 : Remove best_icon_hash. #
Total comments: 21
Patch Set 6 : Add tests. #Patch Set 7 : Nits. #
Total comments: 7
Patch Set 8 : Update unit tests and nits. #
Total comments: 6
Patch Set 9 : Nits. #Patch Set 10 : Try to fix the compile error. #
Total comments: 4
Patch Set 11 : Add a member function for testing. #Patch Set 12 : Fix compile error. #
Total comments: 10
Patch Set 13 : pkotwicz@'s comments. #
Total comments: 1
Patch Set 14 : Nits. #
Total comments: 62
Patch Set 15 : dominickn@'s comments. #Patch Set 16 : Renaming. #Messages
Total messages: 83 (44 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. BUG=665078 ========== to ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. BUG=665078 ==========
Description was changed from ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. BUG=665078 ========== to ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. BUG=665078 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Thank you for following up on this bug. https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:127: return webapk; For the sake of simplicity, can we not do the return here? We will populate all the fields as usual and the server will choose to ignore them [For a different CL perhaps] I don't know whether the server request will succeed if - we are updating a OneOffWebApk over a year after the server created the WebAPK. - the web manifest is stale Does the server retain data for OneOffWebAPKs indefinitely? https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:258: int webapk_version) { Can we pass whether the manifest is stale from Java?
PTAL, thanks! https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:127: return webapk; On 2016/11/24 22:39:26, pkotwicz wrote: > For the sake of simplicity, can we not do the return here? We will populate all > the fields as usual and the server will choose to ignore them We don't have the best_icon_url, so we can't really populate all the fields. So for one-off WebAPK, we only send incomplete data; for certified WebAPK, the server won't and shouldn't trust the resources sent from the client. Neither case can't benefit from sending more data, personally I would prefer to have a return here. > [For a different CL perhaps] I don't know whether the server request will > succeed if > - we are updating a OneOffWebApk over a year after the server created the > WebAPK. > - the web manifest is stale > Does the server retain data for OneOffWebAPKs indefinitely? Currently yes, but they have a plan to clean up caches to avoid the storage grow. Talked with Gelnn offline, maybe the best solution is to stop supporting such a WebAPK. Filed a bug crbug.com/669060 to track this case. https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:258: int webapk_version) { On 2016/11/24 22:39:26, pkotwicz wrote: > Can we pass whether the manifest is stale from Java? Done.
https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:127: return webapk; > We don't have the best_icon_url, so we can't really populate all the > fields. So > for one-off WebAPK, we only send incomplete data; for certified WebAPK, > the > server won't and shouldn't trust the resources sent from the client. > Neither > case can't benefit from sending more data, personally I would prefer to > have a > return here. I agree that sending more data is not useful. However, we are sending the extra data in the 99% case. In my opinion it is clearer and simpler to send data in this case too (Not sending it requires knowledge of how the server is implemented) > Currently yes, but they have a plan to clean up caches to avoid the > storage > grow. Talked with Gelnn offline, maybe the best solution is to stop > supporting > such a WebAPK. Filed a bug crbug.com/669060 to track this case. I think that we should store the bestIconUrl in AndroidManifest.xml to handle this case
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. BUG=665078 ========== to ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server creates an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 ==========
Description was changed from ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server creates an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 ========== to ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server create an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 ==========
PTAL, thanks! https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:127: return webapk; On 2016/11/28 21:29:07, pkotwicz wrote: > > We don't have the best_icon_url, so we can't really populate all the > > fields. So > > for one-off WebAPK, we only send incomplete data; for certified WebAPK, > > the > > server won't and shouldn't trust the resources sent from the client. > > Neither > > case can't benefit from sending more data, personally I would prefer to > > have a > > return here. > > I agree that sending more data is not useful. However, we are sending the extra > data in the 99% case. In my opinion it is clearer and simpler to send data in > this case too (Not sending it requires knowledge of how the server is > implemented) Done. > > Currently yes, but they have a plan to clean up caches to avoid the > > storage > > grow. Talked with Gelnn offline, maybe the best solution is to stop > > supporting > > such a WebAPK. Filed a bug crbug.com/669060 to track this case. > I think that we should store the bestIconUrl in AndroidManifest.xml to handle > this case As discussed offline, it is better to avoid storing bestIconUrl anyway.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2528073002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:277: String name, String shortName, String bestIconUrl, String bestIconMurmur2Hash, Can we remove the |bestIconMurmur2Hash| completely? https://codereview.chromium.org/2528073002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:148: best_image->set_image_data(&png_bytes.front(), png_bytes.size()); This function should fail (return NULL) if the icon_urls and hashes have a different length https://codereview.chromium.org/2528073002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:264: const std::vector<std::string>& icon_hashs) { Nit: icon_hashs -> icon_hashes
PTAL, thanks! https://codereview.chromium.org/2528073002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:277: String name, String shortName, String bestIconUrl, String bestIconMurmur2Hash, On 2016/12/06 17:53:37, pkotwicz wrote: > Can we remove the |bestIconMurmur2Hash| completely? Good point. Removed. https://codereview.chromium.org/2528073002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:148: best_image->set_image_data(&png_bytes.front(), png_bytes.size()); On 2016/12/06 17:53:37, pkotwicz wrote: > This function should fail (return NULL) if the icon_urls and hashes have a > different length Pass in a map instead of two arrays, and this function won't fall. https://codereview.chromium.org/2528073002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:264: const std::vector<std::string>& icon_hashs) { On 2016/12/06 17:53:37, pkotwicz wrote: > Nit: icon_hashs -> icon_hashes Done.
Just nits https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:138: String[] iconUrls = iconUrlSet.toArray(new String[iconUrlSet.size()]); Shouldn't this be new String[0] https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:139: String[] iconHashs = new String[iconUrls.length]; iconHashs -> iconHashes https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:141: String icon_hash = info.iconUrlToMurmur2HashMap().get(iconUrls[i]); icon_hash -> iconHash https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:110: std::unique_ptr<webapk::WebApk> BuildWebApkProtoInBackground( We should test the behavior of this method when ShortcutInfo::best_icon_url is empty I haven't tried but perhaps you can parse the proto from net::test_server::HttpRequest::content https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:150: best_image->set_hash(""); Is line 150 necessary? What would go wrong if we just deleted it? https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:166: image->set_hash(entry.second); Can't we always set the hash and remove line 165? https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:388: for (const std::string& icon_url: shortcut_info_.icon_urls) { Nit: Space before ':' https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:72: const std::map<std::string, std::string>& icon_url_hash_map); Nit: Name this |icon_url_to_murmur2_hash_map| to match WebApkInfo.java https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:99: std::map<std::string, std::string> icon_url_hash_map; Nit: Name this |icon_url_to_murmur2_hash_map| to match WebApkInfo.java https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:101: std::string icon_hash = i < icon_hashes.size() ? icon_hashes[i] : ""; I think that it is unnecessary to handle the case of |icon_hashes| and |icon_urls| having a different length. This function is closesly coupled to Java's WebApkUpdateManager#updateAsync()
Patchset #7 (id:200001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:220001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:138: String[] iconUrls = iconUrlSet.toArray(new String[iconUrlSet.size()]); On 2016/12/07 20:41:30, pkotwicz wrote: > Shouldn't this be new String[0] Done. https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:139: String[] iconHashs = new String[iconUrls.length]; On 2016/12/07 20:41:30, pkotwicz wrote: > iconHashs -> iconHashes Done. https://codereview.chromium.org/2528073002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:141: String icon_hash = info.iconUrlToMurmur2HashMap().get(iconUrls[i]); On 2016/12/07 20:41:30, pkotwicz wrote: > icon_hash -> iconHash Sorry for the wrong naming style again. I will be more careful about this. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:110: std::unique_ptr<webapk::WebApk> BuildWebApkProtoInBackground( On 2016/12/07 20:41:30, pkotwicz wrote: > We should test the behavior of this method when ShortcutInfo::best_icon_url is > empty > > I haven't tried but perhaps you can parse the proto from > net::test_server::HttpRequest::content It is easier to parse the created proto directly. Add 2 tests for this function. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:150: best_image->set_hash(""); On 2016/12/07 20:41:30, pkotwicz wrote: > Is line 150 necessary? What would go wrong if we just deleted it? Looks like should be fine. Removed. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:166: image->set_hash(entry.second); On 2016/12/07 20:41:30, pkotwicz wrote: > Can't we always set the hash and remove line 165? Personally I don't prefer to do that, since more data will be sent to the server. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:388: for (const std::string& icon_url: shortcut_info_.icon_urls) { On 2016/12/07 20:41:30, pkotwicz wrote: > Nit: Space before ':' Done. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:72: const std::map<std::string, std::string>& icon_url_hash_map); On 2016/12/07 20:41:30, pkotwicz wrote: > Nit: Name this |icon_url_to_murmur2_hash_map| to match WebApkInfo.java Done. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:99: std::map<std::string, std::string> icon_url_hash_map; On 2016/12/07 20:41:30, pkotwicz wrote: > Nit: Name this |icon_url_to_murmur2_hash_map| to match WebApkInfo.java Done. https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:101: std::string icon_hash = i < icon_hashes.size() ? icon_hashes[i] : ""; On 2016/12/07 20:41:30, pkotwicz wrote: > I think that it is unnecessary to handle the case of |icon_hashes| and > |icon_urls| having a different length. This function is closesly coupled to > Java's WebApkUpdateManager#updateAsync() Done.
https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:166: image->set_hash(entry.second); Always sending the hashes makes things clearer. The hashes are not a lot of data compared to the bitmap data that we send
PTAL, thanks!
L-G-T-M once you address my test related comment https://codereview.chromium.org/2528073002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:120: // an empty "best icon URL" to tell the server that there is no best icon URL. Please add a comment describing why we set the staleManifest flag. Perhaps: "Tell the server that the our version of the Web Manifest might be stale and to ignore our Web Manifest data if the server's Web Manifest data is newer. This scenario can occur if the Web Manifest is temporarily unreachable." Something I have not considered is what occurs when a Web Manifest transitions from being "generic" to being "customized to each user". I don't think that it is possible to upgrade a CertifiedWebAPK to a OneOffWebAPK. If a site transitions from the certified case to the OneOff case I think that all of the currently installed CertifiedWebAPKs will permanently fail to update. Perhaps: - the staleManifest flag should tell the server whether it should refetch the Web Manifest - the server should return the newest CertifiedWebApk it has if the fetched Web Manifest data does not match what is provided by the client. https://codereview.chromium.org/2528073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:138: for (int i = 0; i < iconUrls.length; ++i) { Nit: I suspect that Dominick will want you to use iconUrlToMurmur2HashMap.entrySet() here https://codereview.chromium.org/2528073002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:93: // Should be used only for testing. This is kind of weird. You could move BuildWebApkProtoInBackground() from the anonymous namespace and make it private static (and make WebApkInstallerTest a friend) https://codereview.chromium.org/2528073002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:389: EXPECT_EQ(best_icon_url.spec(), icons[0].src()); Replace best_icon_url.spec() with "" to make it super-duper-crystal-clear
Patchset #8 (id:280001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2528073002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:120: // an empty "best icon URL" to tell the server that there is no best icon URL. On 2016/12/09 22:18:55, pkotwicz wrote: > Please add a comment describing why we set the staleManifest flag. > > Perhaps: "Tell the server that the our version of the Web Manifest might be > stale and to ignore our Web Manifest data if the server's Web Manifest data is > newer. This scenario can occur if the Web Manifest is temporarily unreachable." SGTM, added. > Something I have not considered is what occurs when a Web Manifest transitions > from being "generic" to being "customized to each user". I don't think that it > is possible to upgrade a CertifiedWebAPK to a OneOffWebAPK. If a site > transitions from the certified case to the OneOff case I think that all of the > currently installed CertifiedWebAPKs will permanently fail to update. A certified WebAPK won't be upgraded to a OneOffWebAPK. That is why the server will already keep a cached version of a certified WebAPK. > Perhaps: > - the staleManifest flag should tell the server whether it should refetch the > Web Manifest > - the server should return the newest CertifiedWebApk it has if the fetched Web > Manifest data does not match what is provided by the client. https://codereview.chromium.org/2528073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:138: for (int i = 0; i < iconUrls.length; ++i) { On 2016/12/09 22:18:55, pkotwicz wrote: > Nit: I suspect that Dominick will want you to use > iconUrlToMurmur2HashMap.entrySet() here Done. https://codereview.chromium.org/2528073002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:389: EXPECT_EQ(best_icon_url.spec(), icons[0].src()); On 2016/12/09 22:18:55, pkotwicz wrote: > Replace best_icon_url.spec() with "" to make it super-duper-crystal-clear Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was unchecked by pkotwicz@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
Adding dominickn@ for his thoughts. The CL looks good to me with the exception of a few nits in webapk_installer_unittest.cc https://codereview.chromium.org/2528073002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:149: iconHashes[i++] = iconHash != null ? iconHash : ""; Nit: Increment |i| on its own line. Yes, your code is correct but it is confusing https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:178: Run(); Can you do this: base::RunLoop run_loop; base::PostTaskAndReplyWithResult( GetBackgroundTaskRunner().get(), FROM_HERE, base::Bind(&WebApkInstaller::BuildWebApkProtoInBackground, shortcut_info, SkBitmap(), stale_manifest, icon_url_to_murmur2_hash_map), run_loop.QuitClosure()); run_loop.Run(); Can you put this code in a function which is not in WebApkInstallerRunner? Given that this function doesn't try to do an install or an update I think that it doesn't belong.
PTAL, thanks! https://codereview.chromium.org/2528073002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:149: iconHashes[i++] = iconHash != null ? iconHash : ""; On 2016/12/14 04:05:41, pkotwicz wrote: > Nit: Increment |i| on its own line. Yes, your code is correct but it is > confusing Done. https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:178: Run(); On 2016/12/14 04:05:41, pkotwicz wrote: > Can you do this: > > base::RunLoop run_loop; > base::PostTaskAndReplyWithResult( > GetBackgroundTaskRunner().get(), FROM_HERE, > base::Bind(&WebApkInstaller::BuildWebApkProtoInBackground, > shortcut_info, SkBitmap(), stale_manifest, > icon_url_to_murmur2_hash_map), run_loop.QuitClosure()); > run_loop.Run(); > > Can you put this code in a function which is not in WebApkInstallerRunner? Given > that this function doesn't try to do an install or an update I think that it > doesn't belong. We can't use run_loop.QuitClosure() directly. This is because the base::PostTaskAndReplyWithResult() requires the return value of the first callback be the input parameter of the second callback. That is why we need another function to bind. Personally I would prefer to move the BuildWebApkProto() to WebApkInstallerRunner since it is neat to have a run_loop in only one class, not both WebApkInstallerRunner and WebApkInstallerTest.
https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:178: Run(); Can you use PostTaskAndReply() instead? https://cs.chromium.org/chromium/src/base/task_runner.h?rcl=0&l=125
https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:209: static std::unique_ptr<webapk::WebApk> BuildWebApkProtoInBackground( A private static method with a friended test is very ugly. Is there a way to restructure this? Perhaps Peter's suggestion will let you avoid having to call this method outside this class. https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:213: std::map<std::string, std::string> icon_url_to_murmur2_hash_map); Make this a const reference.
https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:209: static std::unique_ptr<webapk::WebApk> BuildWebApkProtoInBackground( Dominick, how do you suggest making this cleaner. One option would be to make this method public static I was the one to suggest a private static method with a friended test class. I know that friending a test class is a common pattern for testing code (or at least it used to be.) For instance HistoryBackendTest and HistoryBackend. The other pattern I know of is having an inner TestApi class. ScopedCaptureClient::TestApi for instance
https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/340001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:209: static std::unique_ptr<webapk::WebApk> BuildWebApkProtoInBackground( On 2016/12/15 04:21:52, pkotwicz wrote: > Dominick, how do you suggest making this cleaner. One option would be to make > this method public static > > I was the one to suggest a private static method with a friended test class. > > I know that friending a test class is a common pattern for testing code (or at > least it used to be.) For instance HistoryBackendTest and HistoryBackend. > The other pattern I know of is having an inner TestApi class. > ScopedCaptureClient::TestApi for instance Friending a test class is fine - what's ugly here is that the only reason this method is private static is because it's called explicitly by the test class. It would be better if this method didn't have to be called by the tester, and instead the tester called a member method which then called this (so it can be in the anonymous namespace again). Otherwise, if this functionality is actually static and could be called by multiple places legitimately (other than the test), then you should put it in the webapk namespace as a top level method.
Hi Dominick, please take a look Set7# and see whether it is something that you are looking for?
ps7 is better - having ForTesting methods is a much more common pattern than private static methods.
Patchset #11 (id:360001) has been deleted
Patchset #11 (id:380001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Dominick, please take another look, thanks! https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:178: Run(); On 2016/12/15 02:38:53, pkotwicz wrote: > Can you use PostTaskAndReply() instead? > https://cs.chromium.org/chromium/src/base/task_runner.h?rcl=0&l=125 Since the WebApkInstaller::BuildWebApkProtoInBackground() has a return value, it doesn't match the signature of base::Closure, which is void().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:149: void BuildWebApkProto( Can you move this to a different class. I know that there will be some copying and pasting. This method does not really belong in this class. For instance, it does not use |url_request_context_getter_| or |best_icon_url_| or |has_google_play_webapk_install_delegate_| https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:416: EXPECT_NE(nullptr, webapk_request); If this EXPECT_NE() fails the test will crash. If EXPECT_NE() fails, the test continues to the next EXPECT. For cases like this we use ASSERT_NE(). ASSERT_NE() terminates the test if it fails https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:419: EXPECT_EQ(3, manifest.icons_size()); ASSERT_EQ() https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:453: EXPECT_NE(nullptr, webapk_request); ASSERT_NE() https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:456: EXPECT_EQ(2, manifest.icons_size()); ASSERT_EQ()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Peter, could you please take another look? Thanks! https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:149: void BuildWebApkProto( On 2016/12/19 15:28:00, pkotwicz wrote: > Can you move this to a different class. I know that there will be some copying > and pasting. This method does not really belong in this class. For instance, it > does not use |url_request_context_getter_| or |best_icon_url_| or > |has_google_play_webapk_install_delegate_| Done. https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:416: EXPECT_NE(nullptr, webapk_request); On 2016/12/19 15:28:00, pkotwicz wrote: > If this EXPECT_NE() fails the test will crash. If EXPECT_NE() fails, the test > continues to the next EXPECT. For cases like this we use ASSERT_NE(). > ASSERT_NE() terminates the test if it fails Thanks for catching these, I updated all of them. https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:419: EXPECT_EQ(3, manifest.icons_size()); On 2016/12/19 15:28:00, pkotwicz wrote: > ASSERT_EQ() Done. https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:453: EXPECT_NE(nullptr, webapk_request); On 2016/12/19 15:28:00, pkotwicz wrote: > ASSERT_NE() Done. https://codereview.chromium.org/2528073002/diff/420001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:456: EXPECT_EQ(2, manifest.icons_size()); On 2016/12/19 15:28:00, pkotwicz wrote: > ASSERT_EQ() Done.
https://codereview.chromium.org/2528073002/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:254: void BuildWebApkProto( Sorry for being a pain. Can you move this function into its own class?
Hi Peter, could you please take another look? Thanks!
LGTM! Thank you very much for bearing with me https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:227: installer->SetTimeoutMs(100); Nit: You don't need to set a timeout https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:242: // Called when the |webapk| request is populated. |webapk| request -> |webapk_request_| We use '||' to indicate a variable name https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:251: // Called after the installation process has succeeded or failed. Please update this comment :)
Mostly comments on the naming. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:24: import java.util.Map.Entry; Minor nit: I would just use Map.Entry rather than having the extra import, but I don't care too much about it. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:120: updateAsync(fetchedInfo, bestIconUrl, false); A useful pattern I've recently learned is updateAsync(fetchedInfo, bestIconUrl, false /* isManifestStale */); This adds some more documentation to the calling location (because raw true/falses are a ripe area for errors). https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:127: updateAsync(mInfo, "", true); true /* isManifestStale */) https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:140: protected void updateAsync(WebApkInfo info, String bestIconUrl, boolean staleManifest) { Call this variable isManifestStale so make it more obvious that it's a boolean. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:326: boolean staleManifest); isManifestStale https://codereview.chromium.org/2528073002/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:80: boolean staleManifest) { isManifestStale https://codereview.chromium.org/2528073002/diff/460001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:141: protected void updateAsync(WebApkInfo info, String bestIconUrl, boolean staleManifest) { isManifestStale https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:52: // no longer avaialbe. "available" https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:114: bool stale_manifest, is_manifest_stale https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:115: std::map<std::string, std::string> icon_url_to_murmur2_hash_map) { const std::map<std::string, std::string>& Also call it icon_url_to_murmur2_map (std::map is not a hash map) https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:146: std::map<std::string, std::string>::iterator it = Does auto it work here? https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:154: for (const std::pair<std::string, std::string>& entry : const auto& entry? https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:269: const std::map<std::string, std::string>& icon_url_to_murmur2_hash_map) { icon_url_to_murmur2_map (std::map is not a hash map) https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:282: const std::map<std::string, std::string>& icon_url_to_murmur2_hash_map) { icon_url_to_murmur2_map (std::map is not a hash map) https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:314: bool stale_manifest, is_manifest_stale https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:315: const std::map<std::string, std::string>& icon_url_to_murmur2_hash_map) { icon_url_to_murmur2_map (std::map is not a hash map) https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:443: std::map<std::string, std::string> icon_url_to_murmur2_hash_map; Minor nit: call this icon_url_to_murmur2_map. std::map isn't a hash map, it's a red-black tree. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:72: bool stale_manifest, is_manifest_stale Also consider having the bool argument last (makes it more readable). https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:81: bool stale_manifest, is_manifest_stale https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:96: bool stale_manifest, is_manifest_stale, consider moving it to last. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:134: WebApkInstaller* installer = CreateWebApkInstaller(); Minor nit: remove this line, and just do CreateWebApkInstaller()->UpdateAsync on line 138 (unless you want to DCHECK it or something) https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:135: std::map<std::string, std::string> icon_url_to_murmur2_hash_map; icon_url_to_murmur2_map (std::map is not a hash map) https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:136: icon_url_to_murmur2_hash_map[best_icon_url_.spec()] = kIconMurmur2Hash; can this be std::map<std::string, std::string> icon_url_to_murmur2_map { {best_icon_url_.spec(), "0"} }; And then remove kIconMurmur2Hash https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:143: false /* stale_manifest */, is_manifest_stale https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:66: jboolean java_stale_manifest) { is_manifest_stale https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:99: std::map<std::string, std::string> icon_url_to_murmur2_hash_map; Not a hash map
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Hi Dominick, PTAL, thanks! https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:24: import java.util.Map.Entry; On 2016/12/20 05:05:48, dominickn wrote: > Minor nit: I would just use Map.Entry rather than having the extra import, but I > don't care too much about it. I would like to, but the compiler complains if I remove any of them. For example, line 295 findMurmur2HashForUrlIgnoringFragment(Map<String, String> iconUrlToMurmur2HashMap, String iconUrlToMatch) needs the import of java.util.Map. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:120: updateAsync(fetchedInfo, bestIconUrl, false); On 2016/12/20 05:05:48, dominickn wrote: > A useful pattern I've recently learned is > > updateAsync(fetchedInfo, bestIconUrl, false /* isManifestStale */); > > This adds some more documentation to the calling location (because raw > true/falses are a ripe area for errors). Acknowledged. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:127: updateAsync(mInfo, "", true); On 2016/12/20 05:05:48, dominickn wrote: > true /* isManifestStale */) Acknowledged. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:140: protected void updateAsync(WebApkInfo info, String bestIconUrl, boolean staleManifest) { On 2016/12/20 05:05:48, dominickn wrote: > Call this variable isManifestStale so make it more obvious that it's a boolean. Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:326: boolean staleManifest); On 2016/12/20 05:05:48, dominickn wrote: > isManifestStale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:80: boolean staleManifest) { On 2016/12/20 05:05:48, dominickn wrote: > isManifestStale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:141: protected void updateAsync(WebApkInfo info, String bestIconUrl, boolean staleManifest) { On 2016/12/20 05:05:48, dominickn wrote: > isManifestStale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:52: // no longer avaialbe. On 2016/12/20 05:05:48, dominickn wrote: > "available" Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:114: bool stale_manifest, On 2016/12/20 05:05:49, dominickn wrote: > is_manifest_stale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:115: std::map<std::string, std::string> icon_url_to_murmur2_hash_map) { On 2016/12/20 05:05:48, dominickn wrote: > const std::map<std::string, std::string>& Done. > Also call it icon_url_to_murmur2_map (std::map is not a hash map) Yes, it is a little bit confusing. The "murmur2_hash" is together, so it is a "icon_url" to "murmur2_hash" map. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:146: std::map<std::string, std::string>::iterator it = On 2016/12/20 05:05:48, dominickn wrote: > Does auto it work here? Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:154: for (const std::pair<std::string, std::string>& entry : On 2016/12/20 05:05:48, dominickn wrote: > const auto& entry? Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:269: const std::map<std::string, std::string>& icon_url_to_murmur2_hash_map) { On 2016/12/20 05:05:48, dominickn wrote: > icon_url_to_murmur2_map (std::map is not a hash map) same. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:282: const std::map<std::string, std::string>& icon_url_to_murmur2_hash_map) { On 2016/12/20 05:05:49, dominickn wrote: > icon_url_to_murmur2_map (std::map is not a hash map) same. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:314: bool stale_manifest, On 2016/12/20 05:05:48, dominickn wrote: > is_manifest_stale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:315: const std::map<std::string, std::string>& icon_url_to_murmur2_hash_map) { On 2016/12/20 05:05:48, dominickn wrote: > icon_url_to_murmur2_map (std::map is not a hash map) same. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:443: std::map<std::string, std::string> icon_url_to_murmur2_hash_map; On 2016/12/20 05:05:49, dominickn wrote: > Minor nit: call this icon_url_to_murmur2_map. std::map isn't a hash map, it's a > red-black tree. same. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:72: bool stale_manifest, On 2016/12/20 05:05:49, dominickn wrote: > is_manifest_stale > > Also consider having the bool argument last (makes it more readable). Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:81: bool stale_manifest, On 2016/12/20 05:05:49, dominickn wrote: > is_manifest_stale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:96: bool stale_manifest, On 2016/12/20 05:05:49, dominickn wrote: > is_manifest_stale, consider moving it to last. Moved it to last and updated other places as well. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:134: WebApkInstaller* installer = CreateWebApkInstaller(); On 2016/12/20 05:05:49, dominickn wrote: > Minor nit: remove this line, and just do CreateWebApkInstaller()->UpdateAsync on > line 138 (unless you want to DCHECK it or something) Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:135: std::map<std::string, std::string> icon_url_to_murmur2_hash_map; On 2016/12/20 05:05:49, dominickn wrote: > icon_url_to_murmur2_map (std::map is not a hash map) same. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:136: icon_url_to_murmur2_hash_map[best_icon_url_.spec()] = kIconMurmur2Hash; On 2016/12/20 05:05:49, dominickn wrote: > can this be > > std::map<std::string, std::string> icon_url_to_murmur2_map { > {best_icon_url_.spec(), "0"} }; > > And then remove kIconMurmur2Hash Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:143: false /* stale_manifest */, On 2016/12/20 05:05:49, dominickn wrote: > is_manifest_stale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:227: installer->SetTimeoutMs(100); On 2016/12/19 23:04:31, pkotwicz wrote: > Nit: You don't need to set a timeout Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:242: // Called when the |webapk| request is populated. On 2016/12/19 23:04:31, pkotwicz wrote: > |webapk| request -> |webapk_request_| > > We use '||' to indicate a variable name Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:251: // Called after the installation process has succeeded or failed. On 2016/12/19 23:04:31, pkotwicz wrote: > Please update this comment :) Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:66: jboolean java_stale_manifest) { On 2016/12/20 05:05:49, dominickn wrote: > is_manifest_stale Done. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:99: std::map<std::string, std::string> icon_url_to_murmur2_hash_map; On 2016/12/20 05:05:49, dominickn wrote: > Not a hash map same.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:24: import java.util.Map.Entry; On 2016/12/20 20:25:08, Xi Han wrote: > On 2016/12/20 05:05:48, dominickn wrote: > > Minor nit: I would just use Map.Entry rather than having the extra import, but > I > > don't care too much about it. > > I would like to, but the compiler complains if I remove any of them. For > example, line 295 findMurmur2HashForUrlIgnoringFragment(Map<String, String> > iconUrlToMurmur2HashMap, String iconUrlToMatch) needs the import of > java.util.Map. I meant, only import java.util.Map, and don't import java.util.Map.Entry (and then just use Map.Entry). Like I said, I'm not overly fussed. https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:115: std::map<std::string, std::string> icon_url_to_murmur2_hash_map) { On 2016/12/20 20:25:09, Xi Han wrote: > On 2016/12/20 05:05:48, dominickn wrote: > > const std::map<std::string, std::string>& > Done. > > > Also call it icon_url_to_murmur2_map (std::map is not a hash map) > Yes, it is a little bit confusing. The "murmur2_hash" is together, so it is a > "icon_url" to "murmur2_hash" map. We should try and eliminate this confusion. An alternative suggestion is to just call the variable icon_url_to_murmur2_hash, since the type already has map in it.
Hi Dominick, PTAL, thanks! https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:24: import java.util.Map.Entry; On 2016/12/21 02:51:49, dominickn wrote: > On 2016/12/20 20:25:08, Xi Han wrote: > > On 2016/12/20 05:05:48, dominickn wrote: > > > Minor nit: I would just use Map.Entry rather than having the extra import, > but > > I > > > don't care too much about it. > > > > I would like to, but the compiler complains if I remove any of them. For > > example, line 295 findMurmur2HashForUrlIgnoringFragment(Map<String, String> > > iconUrlToMurmur2HashMap, String iconUrlToMatch) needs the import of > > java.util.Map. > > I meant, only import java.util.Map, and don't import java.util.Map.Entry (and > then just use Map.Entry). Like I said, I'm not overly fussed. I see. Updated! https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2528073002/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:115: std::map<std::string, std::string> icon_url_to_murmur2_hash_map) { On 2016/12/21 02:51:49, dominickn wrote: > On 2016/12/20 20:25:09, Xi Han wrote: > > On 2016/12/20 05:05:48, dominickn wrote: > > > const std::map<std::string, std::string>& > > Done. > > > > > Also call it icon_url_to_murmur2_map (std::map is not a hash map) > > Yes, it is a little bit confusing. The "murmur2_hash" is together, so it is a > > "icon_url" to "murmur2_hash" map. > > We should try and eliminate this confusion. An alternative suggestion is to just > call the variable icon_url_to_murmur2_hash, since the type already has map in > it. Done.
lgtm, thanks!
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2528073002/#ps500001 (title: "Renaming.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1482417235409500,
"parent_rev": "9a3510947539f76a752a2d786067d5440fd7c389", "commit_rev":
"1a8a4685ccc6e474ade934aae2255ec2564e5af0"}
Message was sent while issue was closed.
Description was changed from ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server create an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 ========== to ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server create an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 Review-Url: https://codereview.chromium.org/2528073002 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server create an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 Review-Url: https://codereview.chromium.org/2528073002 ========== to ========== Add a flag in WebAPK's proto when the Web App Manifest is no longer available. A special case of WebAPK's update is: the Web Manifest of a WebAPK has been removed from its site and the shell version of this WebAPK is out-of-date. We want to force such an update, so needs to add a new flag in the proto to indicate this special update request. On the server side, WebAPK server will look for the cached resources of the given package name, and create a updated WebAPK with the latest shell. However, one-off WebAPKs will be cleaned up from the server if there isn't any request within a period of time. For this very rare case, we will send all the icon URLs an icon hashs to allow server create an updated one-off WebAPKs with the same AndroidManfiest.xml. BUG=665078 Committed: https://crrev.com/a98b72dc3235a7bf230877beafc9b3d131b311f7 Cr-Commit-Position: refs/heads/master@{#440420} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a98b72dc3235a7bf230877beafc9b3d131b311f7 Cr-Commit-Position: refs/heads/master@{#440420} |
