Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184913005:
Add calls to the server to request WebAPK updates. (Closed)
DescriptionAdd calls to the server to request WebAPK updates.
Introduce WebApkUpdateManager to manage when to check updates and calls
WebApkInstaller in C++ to send update request to WebAPK Minting Server.
BUG=631025
Committed: https://crrev.com/4ebb44de7b424be3116c02bf685c7cfa9a38aded
Cr-Commit-Position: refs/heads/master@{#412977}
Patch Set 1 #
Total comments: 25
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 8
Patch Set 3 : Rebase and nits. #Patch Set 4 : Rebase and add AsyncTask for checking updates. #
Total comments: 1
Patch Set 5 : Add Jni call for updateAsync. #
Total comments: 40
Patch Set 6 : Rebase and pkotwicz@'s comments. #
Total comments: 68
Patch Set 7 : WebAPK no longer owns a ManifestUpgradeDetector and rebase on "worker thread" CL. #
Total comments: 77
Patch Set 8 : Nits #
Total comments: 6
Patch Set 9 : yfriedman@'s comments. #
Total comments: 4
Patch Set 10 : Rebase and nits. #
Total comments: 4
Patch Set 11 : Rebase and nits. #
Total comments: 8
Patch Set 12 : Nits. #Messages
Total messages: 95 (57 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Add calls to the server to request WebAPK updates. BUG=631025 ========== to ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 ==========
Description was changed from ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 ========== to ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. This CL depends on CL (https://codereview.chromium.org/2138973002/). BUG=631025 ==========
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #1 (id:100001) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Xi I will look at your CL. I am going to address dominickn@'s comments to https://codereview.chromium.org/2138973002/ first (That involves fixing a few crashes in the current "Add to Homescreen Code")
Xi, thank you for doing this work. Some initial comments about the proto https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:24: message WebApkResponse { I think that we should either: - Have a different "create WebAPK proto" and a different "update WebAPK" proto and have a different "create WebAPK response" proto and a different "update WebAPK response" proto OR - Have the same proto for "Creating WebAPKs" and for "Updating WebAPKs" and have the same proto for the "Create WebAPK response" and for the "Update WebAPK response" https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:43: optional string version = 2; You do not seem to send this field in webapk_installer.cc
https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:180: WebApkUpdateManager.updateAsync(mWebappInfo); Shouldn't you call WebApkUpdateManager with the newly fetched Web Manifest info? Otherwise, in the case of a OneOffWebAPK the WebAPK server will generate the exact same OneOffWebApk https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:65: updateAsync(detector.getWebappInfo()); Don't you want to call updateAsync() with the new Web Manifest data? Using the new Web Manifest data makes things complicated. Perhaps calling checkUpdate() in the retry case is OK? The update request should not fail often https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:32: optional string signed_market_url = 5; In the new world, Chrome is responsible for downloading the WebAPK not the Google Play server. In this new world, I think that |signed_download_url| makes more sense than |signed_market_url| Yes, the name of the field on the server needs to be updated https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:35: repeated string warnings = 4; We do not seem to be using this field. Let's skip it for now and use the |reserved| keyword https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:112: // InitializeCreateRequestContextGetterOnUIThread() is called. I think you can use BrowserThread::PostTaskAndReply() so that you do not need to have both WebApkInstaller::InitializeCreateRequestContextGetterOnUIThread() and WebApkInstaller::InitializeUpdateRequestContextGetterOnUIThread() https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:204: void WebApkInstaller::SendRequest(std::unique_ptr<RequestProto> request_proto) { Nit: You can change the type to std::unique_ptr<::google::protobuf::MessageLite> https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:265: I think that templates are generally discouraged in Chromium (though I can't find a place in the style guide which says this) Here are my suggestions: Change function to std::unique_ptr<webapk::WebApk> WebApkInstaller::BuildWebApkProto() {} and use CreateWebApkRequest::set_allocated_webapk() OR Change function to void WebApkInstaller::PopulateWebApkProto(webapk::WebApk* webapk) {} The generated C++ code for the proto is located in out/gn_Debug/gen/chrome/browser/android/webapk/webapk.pb.h https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:6: #define CHROME_BROWSER_ANDROID_WEBAPK_WEBAPK_INSTALLER_H_ 😲 Thanks for catching this! https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:57: const std::string& WebApkPackageName); Nit: |webapk_package_name| and |webapk_version_code|should be passed into UpdateAsync(). (I think that it is slightly more intuitive) https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:86: installer->UpdateAsync(base::Bind(&WebApkUpdateManager::OnBuiltWebApk)); Does WebApkInstaller::FinishCallback need to pass the package? Can we instead do: installer->UpdateAsync(base::Bind(&WebApkUpdateManager::OnBuiltWebApk, webapk_package));
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:180: WebApkUpdateManager.updateAsync(mWebappInfo); On 2016/08/03 01:07:06, pkotwicz wrote: > Shouldn't you call WebApkUpdateManager with the newly fetched Web Manifest info? > Otherwise, in the case of a OneOffWebAPK the WebAPK server will generate the > exact same OneOffWebApk Discussed with Glen, yes, we need to send the new data. Thanks for catching this. https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:65: updateAsync(detector.getWebappInfo()); On 2016/08/03 01:07:06, pkotwicz wrote: > Don't you want to call updateAsync() with the new Web Manifest data? > > Using the new Web Manifest data makes things complicated. Perhaps calling > checkUpdate() in the retry case is OK? The update request should not fail often > Good point, I think calling checkUpdate() is ok. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:24: message WebApkResponse { On 2016/08/02 21:05:04, pkotwicz wrote: > I think that we should either: > - Have a different "create WebAPK proto" and a different "update WebAPK" proto > and have a different "create WebAPK response" proto and a different "update > WebAPK response" proto > > OR > > - Have the same proto for "Creating WebAPKs" and for "Updating WebAPKs" and have > the same proto for the "Create WebAPK response" and for the "Update WebAPK > response" As discussed offline, just keep the proto the same as what the server declared. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:32: optional string signed_market_url = 5; On 2016/08/03 01:07:06, pkotwicz wrote: > In the new world, Chrome is responsible for downloading the WebAPK not the > Google Play server. In this new world, I think that |signed_download_url| makes > more sense than |signed_market_url| > > Yes, the name of the field on the server needs to be updated Done. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:35: repeated string warnings = 4; On 2016/08/03 01:07:06, pkotwicz wrote: > We do not seem to be using this field. Let's skip it for now and use the > |reserved| keyword Done. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:43: optional string version = 2; On 2016/08/02 21:05:04, pkotwicz wrote: > You do not seem to send this field in webapk_installer.cc Good catch, added. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:112: // InitializeCreateRequestContextGetterOnUIThread() is called. On 2016/08/03 01:07:06, pkotwicz wrote: > I think you can use BrowserThread::PostTaskAndReply() so that you do not need to > have both WebApkInstaller::InitializeCreateRequestContextGetterOnUIThread() > and WebApkInstaller::InitializeUpdateRequestContextGetterOnUIThread() Good idea, thanks! https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:204: void WebApkInstaller::SendRequest(std::unique_ptr<RequestProto> request_proto) { On 2016/08/03 01:07:06, pkotwicz wrote: > Nit: You can change the type to std::unique_ptr<::google::protobuf::MessageLite> It is great to have the super class of the proto, so we don't need the template at all. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:265: On 2016/08/03 01:07:06, pkotwicz wrote: > I think that templates are generally discouraged in Chromium (though I can't > find a place in the style guide which says this) > > Here are my suggestions: > Change function to > std::unique_ptr<webapk::WebApk> WebApkInstaller::BuildWebApkProto() {} > and use CreateWebApkRequest::set_allocated_webapk() > > OR > > Change function to > void WebApkInstaller::PopulateWebApkProto(webapk::WebApk* webapk) {} > > The generated C++ code for the proto is located in > out/gn_Debug/gen/chrome/browser/android/webapk/webapk.pb.h Ok, I refereed some examples of the template in Chromium, so I don't know template isn't encouraged in Chromium code base. The void WebApkInstaller::PopulateWebApkProto(webapk::WebApk* webapk) sounds good to me. Adopt this and remove the template. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:6: #define CHROME_BROWSER_ANDROID_WEBAPK_WEBAPK_INSTALLER_H_ On 2016/08/03 01:07:06, pkotwicz wrote: > 😲 Thanks for catching this! I used a refactoring tool which is super cool to move c++ code: tools/git/move-source-file.py old_file_path new_file_path It makes these changes and even update the build file:) https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:57: const std::string& WebApkPackageName); On 2016/08/03 01:07:06, pkotwicz wrote: > Nit: |webapk_package_name| and |webapk_version_code|should be passed into > UpdateAsync(). (I think that it is slightly more intuitive) Done. https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:86: installer->UpdateAsync(base::Bind(&WebApkUpdateManager::OnBuiltWebApk)); On 2016/08/03 01:07:06, pkotwicz wrote: > Does WebApkInstaller::FinishCallback need to pass the package? > > Can we instead do: > installer->UpdateAsync(base::Bind(&WebApkUpdateManager::OnBuiltWebApk, > webapk_package)); Yes, we could, updated.
The code looks pretty good :) I thinks it is just nits after this https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:6: #define CHROME_BROWSER_ANDROID_WEBAPK_WEBAPK_INSTALLER_H_ Awesome! 😀 I will try it out some time https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:157: WebApkUpdateManager.checkUpdate(mWebappInfo.id(), mManifestUpgradeDetector); It is kind of weird that WebApkActivity owns an instance of ManifestUpgradeDetector and that WebApkUpdateManager is static. I think that it would make more sense if WebApkUpdateManager were not static and owned the ManifestUpgradeDetector. Currently, WebApkUpdateManager and ManifestUpgradeDetector are simple enough that they can be merged (especially with https://codereview.chromium.org/2161163004/). It is possible that WebApkUpdateManager will become more complicated in the future. (I am ok if we keep the WebApkUpdateManager and ManifestUpgradeDetector split. I am trying to express my thought process) https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:74: if (sinceLastCheckDuration > FULL_CHECK_UPDATE_DURATION) { Can we use |sinceLastRequestDuration| for this check as well? https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:104: webappInfo.backgroundColor(), data.webManifesUrl(), "", Can you get the manifest URL from WebappInfo? I know we want to change that. When we remove the "manifest URL" from WebappInfo, we should query the WebAPK's meta data once and plumb the meta data to WebApkUpdateManager and ManifestUpgradeDetector. https://codereview.chromium.org/2184913005/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:43: class WebApkResponse; This forward declaration does not seem necessary anymore
PTAL, thanks! https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:157: WebApkUpdateManager.checkUpdate(mWebappInfo.id(), mManifestUpgradeDetector); On 2016/08/03 19:09:34, pkotwicz wrote: > It is kind of weird that WebApkActivity owns an instance of > ManifestUpgradeDetector and that WebApkUpdateManager is static. > I think that it would make more sense if WebApkUpdateManager were not static and > owned the ManifestUpgradeDetector. > > Currently, WebApkUpdateManager and ManifestUpgradeDetector are simple enough > that they can be merged (especially with > https://codereview.chromium.org/2161163004/). It is possible that > WebApkUpdateManager will become more complicated in the future. (I am ok if we > keep the WebApkUpdateManager and ManifestUpgradeDetector split. I am trying to > express my thought process) As discussed offline, we keep the code as it is now, since static functions make JNI calls easier. https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:74: if (sinceLastCheckDuration > FULL_CHECK_UPDATE_DURATION) { On 2016/08/03 19:09:34, pkotwicz wrote: > Can we use |sinceLastRequestDuration| for this check as well? Done. https://codereview.chromium.org/2184913005/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:104: webappInfo.backgroundColor(), data.webManifesUrl(), "", On 2016/08/03 19:09:34, pkotwicz wrote: > Can you get the manifest URL from WebappInfo? I know we want to change that. > When we remove the "manifest URL" from WebappInfo, we should query the WebAPK's > meta data once and plumb the meta data to WebApkUpdateManager and > ManifestUpgradeDetector. Done. https://codereview.chromium.org/2184913005/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:43: class WebApkResponse; On 2016/08/03 19:09:34, pkotwicz wrote: > This forward declaration does not seem necessary anymore Except WebApk, other classes are removed.
Can you please rebase this CL on top of: https://codereview.chromium.org/2138973002/ and https://codereview.chromium.org/2161163004/ In particular there is now webapk_installer_unittest.cc We should have a test for the update flow (not sure if we need more than 1 test given that the update flow is very similar to the WebAPK creation flow)
Patchset #4 (id:300001) has been deleted
Patchset #4 (id:320001) has been deleted
Patchset #4 (id:340001) has been deleted
Hi Peter, I have rebased my CL, PTAL, thanks!
https://codereview.chromium.org/2184913005/diff/360001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:147: const base::android::ScopedJavaLocalRef<jstring>& java_package_name) { While reading the design docs I realized that we need to call a different Google Play API for installing the WebAPK and for updating the WebAPK. I think that the update flow should call Java_WebApkInstaller_updateAsyncFromNative() not Java_WebApkInstaller_installAsyncFromNative()
Patchset #5 (id:380001) has been deleted
Patchset #5 (id:400001) has been deleted
Patchset #5 (id:420001) has been deleted
Patchset #5 (id:440001) has been deleted
Hi Peter, I addressed all of your comments except adding a test. I will add tests ASAP. PTAL, thanks!
pkotwicz@chromium.org changed reviewers: + hartmanng@chromium.org, yfriedman@chromium.org
Thank you for rebasing the CL. Some more comments. I think that the CL is almost ready! Adding Glenn as a reviewer with respect to my questions in webapk_installer.cc Adding Yaron as a reviewer because he has lots of ideas about what the update architecture should look like https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:160: WebApkUpdateManager.checkUpdate(mWebappInfo.id(), mManifestUpgradeDetector); I just talked to Yaron. He said that it is OK for us to call WebApkUpdateManager.checkUpdate() on the UI thread. If I understand correctly, AsyncTasks are for expensive things such as encoding a Bitmap. I don't think that WebApkUpdateManager does anything expensive. WebappRegistry#getWebappDataStorage() already kicks off an Async task. ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() does a bunch of non-expensive things on the IO thread. I am going to change this so that only the expensive things are done on a background thread. https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:28: static boolean installAsyncFromNative(String filePath, String packageName) { Nit: I think that most people make @CalledByNative functions private (Which would impact the ordering of the functions in this file) https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:50: WebApkInstaller apkInstaller = ((ChromeApplication) context).createWebApkInstaller(); Super nit: Can this logic be moved to a function used by both installAsyncFromNative() and updateAsyncFromNative(). Perhaps: private static WebApkInstaller createWebApkInstaller() {} https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: && !storage.getDoesLastRequestForNewShellApkVersionSucceed()) { Sorry for the confusion. I was suggesting changing this if() statement to: if (sinceLastRequestDuration > FULL_CHECK_UPDATE_DURATION || (sinceLastRequestDuration > RETRY_UPDATE_DURATION && !storage.getDoesLastRequestForNewShellApkVersionSucceed())) {} https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:87: taskType_(UNDEFINED), Nit: Camel case for variables in C++ code https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:150: taskType_ = UNDEFINED; Nit: Don't bother resetting |task_type_|. If you want you can check whether |task_type_| is UNDEFINED in InstallAsync() & UpdateAsync(). https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:155: return Java_WebApkInstaller_updateAsyncFromNative( Thank you for adding the Java calls for updating! https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:215: The server expects a webapk::WebApk proto not a webapk::UpdateWebApkRequest proto (You reviewed https://codereview.chromium.org/2223443002/ 😎) https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:228: + kDefaultWebApkServerUrlResponseType); Glenn can you comment? I thought the server URL would be the same for the "create" and "update" requests. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:229: SendRequest(std::move(request), net::URLFetcher::PUT, server_url); I am unsure whether we want to use PUT here. Are PUT requests cached? https://stormpath.com/blog/put-or-post suggests that they might. We do not want update requests to be cached because even if |webapk| is the same as a previous request, the ShellAPK version may have changed https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:171: int version_; Nit: Rename to |webapk_version_| for the sake of consistency. Also to make clear that the variable refers to the WebAPK version, not the Chrome version https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:63: << "Sent request to update WebAPK to server. Seems to have worked."; Nit: Use DVLOG(1) for the success case. DVLOG(1) is only printed when --v=1 (or greater) is passed to the command line I was dinged for this in https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:75: static void UpdateAsync( Nit: I have been trying to follow the ordering of arguments in ShortcutHelper#addWebapp() - Can you change the order of the |java_icon_url| and |java_icon_bitmap| arguments? https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:78: const JavaParamRef<jstring>& java_webapk_package, Nit: Keep |java_webapk_package| and |java_webapk_version| together https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:79: const JavaParamRef<jstring>& java_url, Nit: java_url -> java_start_url https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:93: GURL web_manifest_url(ConvertJavaStringToUTF8(env, java_web_manifest_url)); |web_manifest_url| is unused? https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:99: info.display = (blink::WebDisplayMode) java_display_mode; Nit: Use static_cast<blink::WebDisplayMode> (Or static_cast<int> if the compiler throws an error when using static_cast<blink::WebDisplayMode>) https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.h (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:24: static void OnBuiltWebApk(const std::string& webapk_package, bool sucess); Nit: sucess -> success https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:27: DISALLOW_COPY_AND_ASSIGN(WebApkUpdateManager); We use DISALLOW_IMPLICIT_CONSTRUCTORS() for static only classes
hartmanng@chromium.org changed reviewers: + scottkirkwood@chromium.org
Adding Scott as well, because I only have some of the answers. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:228: + kDefaultWebApkServerUrlResponseType); On 2016/08/08 18:59:15, pkotwicz wrote: > Glenn can you comment? I thought the server URL would be the same for the > "create" and "update" requests. Xi is correct - the server currently expects "<host>/v1alpha.webApk/<name of apk to update>/?alt=proto" (see the Update entry in go/webapk-api). https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:229: SendRequest(std::move(request), net::URLFetcher::PUT, server_url); On 2016/08/08 18:59:15, pkotwicz wrote: > I am unsure whether we want to use PUT here. Are PUT requests cached? > https://stormpath.com/blog/put-or-post suggests that they might. We do not want > update requests to be cached because even if |webapk| is the same as a previous > request, the ShellAPK version may have changed Again, this is what the server is expecting currently. I don't know much about PUT and caching, but you bring up a good point. +scott to comment on his reasoning for using PUT in the original API - should we be changing this one to POST as well?
https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:228: + kDefaultWebApkServerUrlResponseType); On 2016/08/08 19:09:51, hartmanng wrote: > On 2016/08/08 18:59:15, pkotwicz wrote: > > Glenn can you comment? I thought the server URL would be the same for the > > "create" and "update" requests. > > Xi is correct - the server currently expects "<host>/v1alpha.webApk/<name of apk > to update>/?alt=proto" (see the Update entry in go/webapk-api). Sorry, the period above was a typo, I really meant: "<host>/v1alpha/webApk/<name of apk to update>/?alt=proto"
Patchset #6 (id:480001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:160: WebApkUpdateManager.checkUpdate(mWebappInfo.id(), mManifestUpgradeDetector); On 2016/08/08 18:59:15, pkotwicz wrote: > I just talked to Yaron. He said that it is OK for us to call > WebApkUpdateManager.checkUpdate() on the UI thread. If I understand correctly, > AsyncTasks are for expensive things such as encoding a Bitmap. I don't think > that WebApkUpdateManager does anything expensive. > WebappRegistry#getWebappDataStorage() already kicks off an Async task. > > ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() does a bunch of > non-expensive things on the IO thread. I am going to change this so that only > the expensive things are done on a background thread. As a second thought, move the call of |callUpdate| to onDeferredStartup() and call it on UI thread. It won't delay the launching of WebAPK any longer. https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:28: static boolean installAsyncFromNative(String filePath, String packageName) { On 2016/08/08 18:59:15, pkotwicz wrote: > Nit: I think that most people make @CalledByNative functions private (Which > would impact the ordering of the functions in this file) I moved the public |updateAsync| before the |installAsyncFromNative|:) https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:50: WebApkInstaller apkInstaller = ((ChromeApplication) context).createWebApkInstaller(); On 2016/08/08 18:59:15, pkotwicz wrote: > Super nit: Can this logic be moved to a function used by both > installAsyncFromNative() and updateAsyncFromNative(). Perhaps: > private static WebApkInstaller createWebApkInstaller() {} Done. https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: && !storage.getDoesLastRequestForNewShellApkVersionSucceed()) { On 2016/08/08 18:59:15, pkotwicz wrote: > Sorry for the confusion. I was suggesting changing this if() statement to: > > if (sinceLastRequestDuration > FULL_CHECK_UPDATE_DURATION > || (sinceLastRequestDuration > RETRY_UPDATE_DURATION && > !storage.getDoesLastRequestForNewShellApkVersionSucceed())) {} As discussed offline, we will keep these two timestamps. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:87: taskType_(UNDEFINED), On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: Camel case for variables in C++ code Updated. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:150: taskType_ = UNDEFINED; On 2016/08/08 18:59:15, pkotwicz wrote: > Nit: Don't bother resetting |task_type_|. If you want you can check whether > |task_type_| is UNDEFINED in InstallAsync() & UpdateAsync(). Removed. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:155: return Java_WebApkInstaller_updateAsyncFromNative( On 2016/08/08 18:59:16, pkotwicz wrote: > Thank you for adding the Java calls for updating! Thanks for catching this! I missed the difference before the meeting with Play. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:215: On 2016/08/08 18:59:15, pkotwicz wrote: > The server expects a webapk::WebApk proto not a webapk::UpdateWebApkRequest > proto (You reviewed https://codereview.chromium.org/2223443002/ 😎) I thought there was technical issue that we need to send webapk::WebApk proto for now, but not sure whether to abandon both webapk::UpdateWebApkRequest and webapk::CreateWebApkRequest entirely. Ok, if that is the case, removed both. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:228: + kDefaultWebApkServerUrlResponseType); On 2016/08/08 19:10:49, hartmanng wrote: > On 2016/08/08 19:09:51, hartmanng wrote: > > On 2016/08/08 18:59:15, pkotwicz wrote: > > > Glenn can you comment? I thought the server URL would be the same for the > > > "create" and "update" requests. > > > > Xi is correct - the server currently expects "<host>/v1alpha.webApk/<name of > apk > > to update>/?alt=proto" (see the Update entry in go/webapk-api). > > Sorry, the period above was a typo, I really meant: > > "<host>/v1alpha/webApk/<name of apk > to update>/?alt=proto" Thank you Glenn for clarifying the url. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:171: int version_; On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: Rename to |webapk_version_| for the sake of consistency. Also to make clear > that the variable refers to the WebAPK version, not the Chrome version Done. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:63: << "Sent request to update WebAPK to server. Seems to have worked."; On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: Use DVLOG(1) for the success case. > > DVLOG(1) is only printed when --v=1 (or greater) is passed to the command line > > I was dinged for this in > https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... That makes sense, we shouldn't log warning for the successful case. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:75: static void UpdateAsync( On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: I have been trying to follow the ordering of arguments in > ShortcutHelper#addWebapp() > - Can you change the order of the |java_icon_url| and |java_icon_bitmap| > arguments? Done. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:78: const JavaParamRef<jstring>& java_webapk_package, On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: Keep |java_webapk_package| and |java_webapk_version| together Done. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:79: const JavaParamRef<jstring>& java_url, On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: java_url -> java_start_url Done. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:93: GURL web_manifest_url(ConvertJavaStringToUTF8(env, java_web_manifest_url)); On 2016/08/08 18:59:16, pkotwicz wrote: > |web_manifest_url| is unused? Good catch, add it to |info|. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:99: info.display = (blink::WebDisplayMode) java_display_mode; On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: Use static_cast<blink::WebDisplayMode> (Or static_cast<int> if the compiler > throws an error when using static_cast<blink::WebDisplayMode>) static_cast<blink::WebDisplayMode> works, updated. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.h (right): https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:24: static void OnBuiltWebApk(const std::string& webapk_package, bool sucess); On 2016/08/08 18:59:16, pkotwicz wrote: > Nit: sucess -> success Done. https://codereview.chromium.org/2184913005/diff/460001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:27: DISALLOW_COPY_AND_ASSIGN(WebApkUpdateManager); On 2016/08/08 18:59:16, pkotwicz wrote: > We use DISALLOW_IMPLICIT_CONSTRUCTORS() for static only classes Done.
Nits and some questions about threading https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:21: */ How about: "WebApkUpdateManager manages when to check for updates to the WebAPK's Web Manifest, and sends update request to the WebAPK Server when an update is needed." In webapk_installer.cc I refer to the server as the "WebAPK server" not the "WebAPK Minting Server". I suggest to keep on doing this for the sake of consistency (and because I landed by code first 😉) https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:23: /** Represents a period of 3 days in milliseconds */ The comment does not provide any information which is not obvious from line 24. How about: "Number of milliseconds between checks for whether the WebAPK's web manifest has changed." Maybe FULL_CHECK_UPDATE_INTERVAL is more accurate than FULL_CHECK_UPDATE_DURATION (The "full update" does not take 3 days to finish) https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:26: /** Represents a period of 12 hours in milliseconds */ The comment does not provide any information which is not obvious from line 27 How about: "Number of milliseconds to wait before re-requesting an updated WebAPK from the WebAPK server if the previous update attempt failed." https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:33: * {@link RETRY_UPDATE_DURATION} but failed, resends the update request to the server. The specific times (FULL_CHECK_UPDATE_DURATION, RETRY_UPDATE_DURATION) are an implementation detail How about: "Checks whether the WebAPK's Web Manifest has changed. Requests an updated WebAPK if the Web Manifest has changed. Skips the check if the check was done recently." https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:35: * @param detector The ManfiestUpgradeDetector owned by the WebAPK. Nit ManfiestUpgradeDetector -> ManifestUpgradeDetector https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:37: public static void checkUpdate(String webApkId, final ManifestUpgradeDetector detector) { Can you please add checks w.r.t to which thread things a run on. My understanding is that this function is not run on the UI thread https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: private static void checkUpdate(WebappDataStorage storage, ManifestUpgradeDetector detector) { My understanding is that this function is run on the UI thread https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:61: storage.updateLastCheckForWebManifestUpdateTime(); We call WebappDataStorage#updateLastCheckForWebManifestUpdateTime() when we start the UpgradeManifestDetectorFetcher not when the UpdateManifestDetectorFetcher finds the Web Manifest. I am concerned that some WebAPK-able sites link to the Web Manifest only from the start_url. If a user usually launches a WebAPK via a deep link, it is unlikely that we will do the upgrade check when the user launches the WebAPK via the app list shortcut. This might cause the WebAPK never to be updated. I think that it is worth filing a bug with this concern. We should investigate whether sites link to the Web Manifest from *all* of the pages within the Web Manifest scope. The washingtonpost.com/pwa does do so. I do not know of any other PWAs https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:66: * Sends request to WebAPK Minting Server to update WebAPK. "WebAPK Minting Server" -> "WebAPK server" https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:93: */ Nit: "the creation process of a new WebAPK fails." -> "update process fails." https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:109: private static native void nativeUpdateAsync(String url, String scope, String name, Nit: |url| -> |startUrl| https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:183: * Deletes the URL and scope, and sets last used time to 0 in SharedPreferences. How about: "and sets all timestamps to 0 in SharedPreferences." https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:421: * Updates the time of the last check of whether the WebAPK's Web Manifest is updated. Nits: - "of whether" -> "for whether" - "is updated" -> "was updated" Perhaps updateTimeOfLastCheckForUpdatedWebManifest() is a less confusing name. I know that my suggestion does not follow the pattern of updateLastUsedTime() https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:429: * Returns the time of the last check of whether the WebAPK's Web Manifest is updated. Nits: - "of whether" -> "for whether" - "is updated" -> "was updated" https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:437: * ShellAPK version. How about: "Updates the time that the last WebAPK update request completed (successfully or unsuccessfully)." (I am suggesting renaming the function to updateLastWebApkUpdateRequestCompletionTime() or updateTimeOfLastWebApkUpdateRequestCompletion()) I think the fact that the time is actually the time that an update was requested from Phonesky is an implementation detail. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:37: // The response format type expected from WebAPK server. Nit: "from WebAPK server" -> "from the WebAPK server" https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:139: int version) { Nit: |version| -> |webapk_version| https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:159: const base::android::ScopedJavaLocalRef<jstring>& java_package_name) { Can you move this logic to OnWebApkDownloaded()? This function exists so that WebApkInstallerTest can stub it out. Since it is stubbed out in tests the function should be short. I am suggesting renaming this function to: WebApkInstaller::StartInstallingDownloadedWebApk() and adding a new function: WebApkInstaller::StartUpdateUsingDownloadedWebApk() https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:210: std::unique_ptr<webapk::WebApk> webapk = BuildWebApkProto(); Nit: Remove the comment. It just reiterates what is on line 213 https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:225: // + |kDeaultWebApkServerUrlResponseType| Nit: The comment is not useful. It just reiterates what is on line 226-227. Is the WebAPK version code supposed to be part of the server URL? The comment in webapk_service.proto suggests that it is If you want you can add a new function to clarify things: GURL GetServerUrlForUpdate(const std::string& webapk_package_name) {} Computing |server_url| in SendUpdateWebApkRequest() is fine by me though Aside: It would be nice if we could change the server and simplify the server URL. (Maybe log a bug?) https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:41: // - whether the request succeeds. How about: "Called when either a request for creating/updating a WebAPK has been sent to Google Play or the create/update process fails. Parameters: - whether the request succeeds." I think that my version makes it clearer what "or" is referring to. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:104: // download and install the generated WebAPK. Nit: The comment is incorrect. WebApkInstaller does the download not Google Play. How about: "Sends a request to WebAPK server to create/update WebAPK. During a successful request the WebAPK server responds with the URL of the generated WebAPK." https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:177: // WebAPK. How about: "Indicates whether the installer ..." My version is shorter :) https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:21: namespace { Nit: New line https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:91: GURL url(ConvertJavaStringToUTF8(env, java_start_url)); Nit: |url| -> |start_url| https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.h (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:13: // to WebAPK Minting Server. Nits: - "send update request" -> "send an update request" - "WebAPK Minting Server" -> "WebAPK server". (At least I refer to the server as the "WebAPK server in webapk_installer.cc) https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:20: // creation process of the new WebAPK fails. How about: ", or the creation process of the new WebAPK fails." -> "or the update process fails." https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:23: // installed. Nit: "installed" -> "updated"
https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:46: getMetaDataFromAndroidManifest(); why was this moved? It seems to only set mStartUrl which is only referenced after it's started? https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:144: private void upgrade(WebappInfo newInfo) { nit: inline this function https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:151: if (mManifestUpgradeDetector == null) { It's a little strange the the activity news up a ManifestUpgradeDetector but then never does anything with it - it just hands it off to WebApkUpdateManager. And then ManifestUpgradeDetector internally knows which exact function to call on WebApkUpdateManager. It seems like all three classes are tightly coupled. Perhaps another way to approach this to instead have a lazily constructed (not just static) WebApkUpdateManager (WAUM). It could be owned by ChromeApplication. When a WebApkActivity is resumed, it registers with the WAUM which then indicates to the WAUM to kick off a check. If the activity is stopped, it tells the WAUM to stop for that. Then WAUM, could own the WAUM and pass a callback to it when it's completed some work on its behalf. This would mean: - WebApkActivity depends on WAUM and not ManifestUpgradeDetector (which was an implementation detail) - WAUM knows how to check storage and use MUD to monitor for updates - MUD doesn't depend on WAUM One aspect I don't like is it's adding more stuff to ChromeApplication. :/ At a minimum, it's not a "..UpdateManager" as-is and is just "...UpdateUtils" (it doesn't keep any state) so should be renamed if we don't change this What do you both think of this? https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:21: */ On 2016/08/09 14:35:42, pkotwicz wrote: > How about: "WebApkUpdateManager manages when to check for updates to the > WebAPK's Web Manifest, and sends update request to the WebAPK Server when an > update is needed." > > In webapk_installer.cc I refer to the server as the "WebAPK server" not the > "WebAPK Minting Server". I suggest to keep on doing this for the sake of > consistency (and because I landed by code first 😉) +1 https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:37: public static void checkUpdate(String webApkId, final ManifestUpgradeDetector detector) { I find this method name overloading confusing. Also the method name should be more of an action. Perhaps rename checkIfUpdateNeeded https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:37: public static void checkUpdate(String webApkId, final ManifestUpgradeDetector detector) { On 2016/08/09 14:35:41, pkotwicz wrote: > Can you please add checks w.r.t to which thread things a run on. My > understanding is that this function is not run on the UI thread +1 https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:42: if (storage == null) { you dereference storage in the next function which will NPE. Is it really possible to not find the metadata? Does that happen if you clear chrome's data and then launch a webapk? Or would we have repopulated it somewhere? https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: private static void checkUpdate(WebappDataStorage storage, ManifestUpgradeDetector detector) { On 2016/08/09 14:35:42, pkotwicz wrote: > My understanding is that this function is run on the UI thread yes. also please rename updateCheckFinished
Patchset #7 (id:520001) has been deleted
Patchset #7 (id:540001) has been deleted
Patchset #7 (id:560001) has been deleted
Patchset #7 (id:580001) has been deleted
Patchset #7 (id:600001) has been deleted
Patchset #9 (id:660001) has been deleted
Patchset #8 (id:640001) has been deleted
Patchset #8 (id:680001) has been deleted
Patchset #7 (id:620001) has been deleted
Patchset #8 (id:720001) has been deleted
Description was changed from ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. This CL depends on CL (https://codereview.chromium.org/2138973002/). BUG=631025 ========== to ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. This CL depends on CL (https://codereview.chromium.org/2228273002/). BUG=631025 ==========
Patchset #7 (id:700001) has been deleted
Patchset #7 (id:740001) has been deleted
Hi Yaron and Peter, PTAL, thanks! https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:46: getMetaDataFromAndroidManifest(); On 2016/08/10 02:26:27, Yaron (back but slow) wrote: > why was this moved? It seems to only set mStartUrl which is only referenced > after it's started? We will guarantee that this function is only called once. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:144: private void upgrade(WebappInfo newInfo) { On 2016/08/10 02:26:27, Yaron (back but slow) wrote: > nit: inline this function Replace it with a callback. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:151: if (mManifestUpgradeDetector == null) { On 2016/08/10 02:26:28, Yaron (back but slow) wrote: > It's a little strange the the activity news up a ManifestUpgradeDetector but > then never does anything with it - it just hands it off to WebApkUpdateManager. > And then ManifestUpgradeDetector internally knows which exact function to call > on WebApkUpdateManager. It seems like all three classes are tightly coupled. > > Perhaps another way to approach this to instead have a lazily constructed (not > just static) WebApkUpdateManager (WAUM). It could be owned by ChromeApplication. > When a WebApkActivity is resumed, it registers with the WAUM which then > indicates to the WAUM to kick off a check. If the activity is stopped, it tells > the WAUM to stop for that. > > Then WAUM, could own the WAUM and pass a callback to it when it's completed some > work on its behalf. This would mean: > - WebApkActivity depends on WAUM and not ManifestUpgradeDetector (which was an > implementation detail) > - WAUM knows how to check storage and use MUD to monitor for updates > - MUD doesn't depend on WAUM > > One aspect I don't like is it's adding more stuff to ChromeApplication. :/ > > At a minimum, it's not a "..UpdateManager" as-is and is just "...UpdateUtils" > (it doesn't keep any state) so should be renamed if we don't change this > > What do you both think of this? As discussed offline, adopted the above solution. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:21: */ On 2016/08/09 14:35:42, pkotwicz wrote: > How about: "WebApkUpdateManager manages when to check for updates to the > WebAPK's Web Manifest, and sends update request to the WebAPK Server when an > update is needed." > > In webapk_installer.cc I refer to the server as the "WebAPK server" not the > "WebAPK Minting Server". I suggest to keep on doing this for the sake of > consistency (and because I landed by code first 😉) Acknowledged. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:23: /** Represents a period of 3 days in milliseconds */ On 2016/08/09 14:35:42, pkotwicz wrote: > The comment does not provide any information which is not obvious from line 24. > How about: "Number of milliseconds between checks for whether the WebAPK's web > manifest has changed." > > Maybe FULL_CHECK_UPDATE_INTERVAL is more accurate than > FULL_CHECK_UPDATE_DURATION (The "full update" does not take 3 days to finish) Sounds good, updated. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:26: /** Represents a period of 12 hours in milliseconds */ On 2016/08/09 14:35:42, pkotwicz wrote: > The comment does not provide any information which is not obvious from line 27 > > How about: "Number of milliseconds to wait before re-requesting an updated > WebAPK from the WebAPK server if the previous update attempt failed." Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:33: * {@link RETRY_UPDATE_DURATION} but failed, resends the update request to the server. On 2016/08/09 14:35:42, pkotwicz wrote: > The specific times (FULL_CHECK_UPDATE_DURATION, RETRY_UPDATE_DURATION) are an > implementation detail > > How about: "Checks whether the WebAPK's Web Manifest has changed. Requests an > updated WebAPK if the Web Manifest has changed. Skips the check if the check was > done recently." Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:35: * @param detector The ManfiestUpgradeDetector owned by the WebAPK. On 2016/08/09 14:35:42, pkotwicz wrote: > Nit ManfiestUpgradeDetector -> ManifestUpgradeDetector Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:37: public static void checkUpdate(String webApkId, final ManifestUpgradeDetector detector) { On 2016/08/10 02:26:28, Yaron (back but slow) wrote: > On 2016/08/09 14:35:41, pkotwicz wrote: > > Can you please add checks w.r.t to which thread things a run on. My > > understanding is that this function is not run on the UI thread > > +1 Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:42: if (storage == null) { On 2016/08/10 02:26:28, Yaron (back but slow) wrote: > you dereference storage in the next function which will NPE. Is it really > possible to not find the metadata? Does that happen if you clear chrome's data > and then launch a webapk? Or would we have repopulated it somewhere? We repopulate the storage during the launching process, but updating the SharedPerference is always an Async task. I add the null check to prevent any crash due to the storage is null. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: private static void checkUpdate(WebappDataStorage storage, ManifestUpgradeDetector detector) { On 2016/08/09 14:35:42, pkotwicz wrote: > My understanding is that this function is run on the UI thread Add the assert. Rename it to startCheckUpdate, since we just start the detector and the check hasn't finished yet. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:61: storage.updateLastCheckForWebManifestUpdateTime(); On 2016/08/09 14:35:42, pkotwicz wrote: > We call WebappDataStorage#updateLastCheckForWebManifestUpdateTime() when we > start the UpgradeManifestDetectorFetcher not when the > UpdateManifestDetectorFetcher finds the Web Manifest. > > I am concerned that some WebAPK-able sites link to the Web Manifest only from > the start_url. If a user usually launches a WebAPK via a deep link, it is > unlikely that we will do the upgrade check when the user launches the WebAPK via > the app list shortcut. This might cause the WebAPK never to be updated. > > I think that it is worth filing a bug with this concern. We should investigate > whether sites link to the Web Manifest from *all* of the pages within the Web > Manifest scope. The washingtonpost.com/pwa does do so. I do not know of any > other PWAs Your concern is right. Filed a bug 636525. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:66: * Sends request to WebAPK Minting Server to update WebAPK. On 2016/08/09 14:35:42, pkotwicz wrote: > "WebAPK Minting Server" -> "WebAPK server" Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:93: */ On 2016/08/09 14:35:42, pkotwicz wrote: > Nit: "the creation process of a new WebAPK fails." -> "update process fails." Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:109: private static native void nativeUpdateAsync(String url, String scope, String name, On 2016/08/09 14:35:41, pkotwicz wrote: > Nit: |url| -> |startUrl| Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:183: * Deletes the URL and scope, and sets last used time to 0 in SharedPreferences. On 2016/08/09 14:35:42, pkotwicz wrote: > How about: "and sets all timestamps to 0 in SharedPreferences." Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:421: * Updates the time of the last check of whether the WebAPK's Web Manifest is updated. On 2016/08/09 14:35:42, pkotwicz wrote: > Nits: > - "of whether" -> "for whether" > - "is updated" -> "was updated" > > Perhaps updateTimeOfLastCheckForUpdatedWebManifest() is a less confusing name. I > know that my suggestion does not follow the pattern of updateLastUsedTime() Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:429: * Returns the time of the last check of whether the WebAPK's Web Manifest is updated. On 2016/08/09 14:35:42, pkotwicz wrote: > Nits: > - "of whether" -> "for whether" > - "is updated" -> "was updated" Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:437: * ShellAPK version. On 2016/08/09 14:35:42, pkotwicz wrote: > How about: "Updates the time that the last WebAPK update request completed > (successfully or unsuccessfully)." > > (I am suggesting renaming the function to > updateLastWebApkUpdateRequestCompletionTime() or > updateTimeOfLastWebApkUpdateRequestCompletion()) > > I think the fact that the time is actually the time that an update was requested > from Phonesky is an implementation detail. Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:37: // The response format type expected from WebAPK server. On 2016/08/09 14:35:42, pkotwicz wrote: > Nit: "from WebAPK server" -> "from the WebAPK server" Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:139: int version) { On 2016/08/09 14:35:42, pkotwicz wrote: > Nit: |version| -> |webapk_version| Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:159: const base::android::ScopedJavaLocalRef<jstring>& java_package_name) { On 2016/08/09 14:35:42, pkotwicz wrote: > Can you move this logic to OnWebApkDownloaded()? This function exists so that > WebApkInstallerTest can stub it out. Since it is stubbed out in tests the > function should be short. > > I am suggesting renaming this function to: > WebApkInstaller::StartInstallingDownloadedWebApk() > and adding a new function: > WebApkInstaller::StartUpdateUsingDownloadedWebApk() Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:210: std::unique_ptr<webapk::WebApk> webapk = BuildWebApkProto(); On 2016/08/09 14:35:42, pkotwicz wrote: > Nit: Remove the comment. It just reiterates what is on line 213 Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:225: // + |kDeaultWebApkServerUrlResponseType| On 2016/08/09 14:35:42, pkotwicz wrote: > Nit: The comment is not useful. It just reiterates what is on line 226-227. > > Is the WebAPK version code supposed to be part of the server URL? The comment in > webapk_service.proto suggests that it is I think it is part of the package name in the comments. > If you want you can add a new function to clarify things: > GURL GetServerUrlForUpdate(const std::string& webapk_package_name) {} > Computing |server_url| in SendUpdateWebApkRequest() is fine by me though I would prefer it inline since it is very simple. > Aside: It would be nice if we could change the server and simplify the server > URL. (Maybe log a bug?) Filed a bug 636552. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:41: // - whether the request succeeds. On 2016/08/09 14:35:43, pkotwicz wrote: > How about: > "Called when either a request for creating/updating a WebAPK has been sent to > Google Play or the create/update process fails. > Parameters: > - whether the request succeeds." > > I think that my version makes it clearer what "or" is referring to. Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:104: // download and install the generated WebAPK. On 2016/08/09 14:35:43, pkotwicz wrote: > Nit: The comment is incorrect. WebApkInstaller does the download not Google > Play. > > How about: "Sends a request to WebAPK server to create/update WebAPK. During a > successful request the WebAPK server responds with the URL of the generated > WebAPK." Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:177: // WebAPK. On 2016/08/09 14:35:43, pkotwicz wrote: > How about: "Indicates whether the installer ..." > > My version is shorter :) Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:21: namespace { On 2016/08/09 14:35:43, pkotwicz wrote: > Nit: New line Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:91: GURL url(ConvertJavaStringToUTF8(env, java_start_url)); On 2016/08/09 14:35:43, pkotwicz wrote: > Nit: |url| -> |start_url| Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.h (right): https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:13: // to WebAPK Minting Server. On 2016/08/09 14:35:43, pkotwicz wrote: > Nits: > - "send update request" -> "send an update request" > - "WebAPK Minting Server" -> "WebAPK server". (At least I refer to the server as > the "WebAPK server in webapk_installer.cc) Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:20: // creation process of the new WebAPK fails. On 2016/08/09 14:35:43, pkotwicz wrote: > How about: ", or the creation process of the new WebAPK fails." -> "or the > update process fails." Done. https://codereview.chromium.org/2184913005/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.h:23: // installed. On 2016/08/09 14:35:43, pkotwicz wrote: > Nit: "installed" -> "updated" Done.
Looking at the CL right now!
LGTM!! We now get updates! Thank you for bearing with me and all my comments and nits https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:24: public interface CheckIfUpgradeNeededCallback { Nits: - Rename this class to Callback .You can get away with such a simple name because this class has only one callback - I think that onUpdateNeededCheckFinished() is slightly clearer than onCheckIfUpdateNeededFinished() because makes it clear that "the check has finished". https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:25: public void onCheckIfUpdateNeededFinished(boolean isUpgraded, WebappInfo newInfo); Nit: "public" in interfaces is redundant. See http://stackoverflow.com/questions/161633/should-methods-in-a-java-interface-... Looks like I made this very mistake in ManifestUpgradeDetectorFetcher https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:47: private CheckIfUpgradeNeededCallback mUpgradeCheckCallback; Nit: Just rename this member to "mCallback" https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: return mTab == null || mTab.getWebContents() == null; Why are you checking whether |mTab| is null? Isn't it a bug to initialize ManifestUpgradeDetector() with a null tab? https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:123: boolean isUpgraded = requireUpgrade(newInfo); Nit: I think that |upgradeRequired| is a more appropriate name than |isUpgraded| (Otherwise it would have been a super fast upgrade) https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:152: mUpdateManager.checkIfUpdateNeeded(getActivityTab(), mWebappInfo); Nit: The method name is deceiving. This method not only checks whether an update is needed but also does the update if it is needed. Perhaps rename the method to updateIfNeeded() ? https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:21: * WebApkUpdateManager manages when to check for updates to the WebAPK's Web Manifest, and sends Nit: "sends update request" -> "sends an update request" https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:43: ThreadUtils.assertOnUiThread(); I am stupid. I thought that checkIfUpdateNeeded() would run on a background thread but it does not. If everything runs on a background thread, you don't need the asserts https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:70: ThreadUtils.assertOnUiThread(); I don't think that this assert is useful https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:122: * Called after the either a request to update the WebAPK has been sent or the update process Nit: "the either" -> "either" https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:49: // The last time that Chrome checks Web Manifest updates for a WebAPK. How about: "The last time that Chrome checked for Web Manifest updates for a WebAPK." https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:58: // Whether the last WebAPK update request succeeds. Nit: succeeds -> succeeded https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:455: * Updates the result of whether the last update request to WebAPK Server succeeds. Nits: - "succeeds" -> "succeeded" - updateDoesLastWebApkUpdateRequestSucceed() -> updateDidLastWebApkUpdateRequestSucceed() https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:464: * Returns whether the last update request to WebAPK Server succeeds. Nits: - "succeeds" -> "succeeded" - getDoesLastWebApkUpdateRequestSucceed() -> getDidLastWebApkUpdateRequestSucceed() https://codereview.chromium.org/2184913005/diff/760001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (left): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:103: mIsUpgraded = true; Doesn't |mIsUpgraded| need to be set to true in the new code? https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:166: Nit: You can steal GetBackgroundTaskRunner() from https://codereview.chromium.org/2236033003/diff/20001/chrome/browser/android/... https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:62: // after the request to download and install the WebAPK is sent to Nit: "request to download and install" -> "request to install" https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:89: // Starts updating of the downloaded WebAPK. Returns whether the updating Nit: "updating of the downloaded WebAPK" -> "update using the downloaded WebAPK" You are updating to the downloaded WebAPK. The "downloaded WebAPK" does not need to be updated. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:52: // WebApkInstaller::StartInstallingDownloadedWebApk() is stubbed out. Nit: "WebApkInstaller::StartInstallingDownloadedWebApk() is stubbed out. WebApkInstaller::StartUpdateUsingDownloadedWebApk() is stubbed out." -> "WebApkInstaller::StartInstallingDownloadedWebApk() and WebApkInstaller::StartUpdateUsingDownloadedWebApk() are stubbed out." https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:78: // Runs the WebApkInstaller installation process and blocks till done. Nit: "installation" -> "installation/update" https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:97: void RunUpdateWebApk() { Nit: "int webapk_version" -> "const int kWebApkVersion" https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:111: WebApkInstaller* createWebApkInstaller() { Functions start with uppercase in C++ https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:120: void setupCallback() { I think that Run() would be a more appropriate name for this function. (This function blocks till the callback is called) https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:217: std::string GetServerUrlForCreateWebApk() { Nit: This should be a const function https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:221: std::string GetServerUrlForUpdateWebApk() { Nit: This should be a const function https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:301: } Thanks for adding the test! https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:62: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Nit: I don't think that the DCHECK is useful (everything occurs on the UI thread) https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:95: static_cast<content::BrowserContext*>(profile), Is the static_cast necessary?
https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:123: boolean isUpgraded = requireUpgrade(newInfo); On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: I think that |upgradeRequired| is a more appropriate name than |isUpgraded| > (Otherwise it would have been a super fast upgrade) same with the method name (I thought I commented on this on the last patchset) https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:127: onComplete(); i would remove this as well. the callback is likely to handle this logic now. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:64: return false; i'm confused - why is this disabled? enough moving parts? https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:72: if (mUpgradeDetector.isWebContentsGone()) return; How about: if (upgradeDetector.start()) { storage.updateTimeOfLastCheckForUpdatedWebManifest(); } (i.e. merge the two functions) https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:76: storage.updateTimeOfLastCheckForUpdatedWebManifest(); this will still NPE in the first case of storage == null (L51) https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:81: public void onCheckIfUpdateNeededFinished(boolean isUpgraded, WebappInfo newInfo) { s/isUpgraded/needsUpgrade/ https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:126: public static void onBuiltWebApk(final boolean success, String webapkPackage) { make private if only called by native https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:91: return GURL(server_url.spec() + webapk_package + "/" + i see what you're saying now about the url. I think this was cause the api team was trying to make this a restful api but it's not anyway, so perhaps we can get it removed. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:116: InstallAsyncWithURLRequestContextGetter( there's no thread-hopping anymore - can we just inline these two functions (the install one and the update one?) https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:88: if (profile == nullptr) { nit: you should do early-outs (precontions) first
Patchset #8 (id:780001) has been deleted
Patchset #8 (id:800001) has been deleted
Patchset #8 (id:820001) has been deleted
Patchset #8 (id:840001) has been deleted
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:24: public interface CheckIfUpgradeNeededCallback { On 2016/08/11 22:02:43, pkotwicz wrote: > Nits: > - Rename this class to Callback .You can get away with such a simple name > because this class has only one callback > - I think that onUpdateNeededCheckFinished() is slightly clearer than > onCheckIfUpdateNeededFinished() because makes it clear that "the check has > finished". Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:25: public void onCheckIfUpdateNeededFinished(boolean isUpgraded, WebappInfo newInfo); On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: "public" in interfaces is redundant. See > http://stackoverflow.com/questions/161633/should-methods-in-a-java-interface-... > > Looks like I made this very mistake in ManifestUpgradeDetectorFetcher Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:47: private CheckIfUpgradeNeededCallback mUpgradeCheckCallback; On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: Just rename this member to "mCallback" Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:60: return mTab == null || mTab.getWebContents() == null; On 2016/08/11 22:02:43, pkotwicz wrote: > Why are you checking whether |mTab| is null? Isn't it a bug to initialize > ManifestUpgradeDetector() with a null tab? As discussed offline, move this check to |ManifestUpgradeDetectorFetcher|. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:123: boolean isUpgraded = requireUpgrade(newInfo); On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > On 2016/08/11 22:02:43, pkotwicz wrote: > > Nit: I think that |upgradeRequired| is a more appropriate name than > |isUpgraded| > > (Otherwise it would have been a super fast upgrade) > > same with the method name (I thought I commented on this on the last patchset) Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:127: onComplete(); On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > i would remove this as well. the callback is likely to handle this logic now. Hmm, this function is used by ManifestUpgradeDetectorTest. Since |upgrade()| is also removed, remove the |onComplete| but add two local variables to track the states. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:152: mUpdateManager.checkIfUpdateNeeded(getActivityTab(), mWebappInfo); On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: The method name is deceiving. This method not only checks whether an update > is needed but also does the update if it is needed. > > Perhaps rename the method to updateIfNeeded() ? Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:64: return false; On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > i'm confused - why is this disabled? enough moving parts? It returns false since I didn't add a logic to update an installed WebAPK. It is unclear to me whether we installl the downloaded WebAPK. or it is possible to just "update". Peter has looked into this part. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:21: * WebApkUpdateManager manages when to check for updates to the WebAPK's Web Manifest, and sends On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: "sends update request" -> "sends an update request" Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:43: ThreadUtils.assertOnUiThread(); On 2016/08/11 22:02:43, pkotwicz wrote: > I am stupid. I thought that checkIfUpdateNeeded() would run on a background > thread but it does not. If everything runs on a background thread, you don't > need the asserts Removed. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:70: ThreadUtils.assertOnUiThread(); On 2016/08/11 22:02:43, pkotwicz wrote: > I don't think that this assert is useful Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:72: if (mUpgradeDetector.isWebContentsGone()) return; On 2016/08/12 21:16:17, Yaron (limited availability) wrote: > How about: > if (upgradeDetector.start()) { > storage.updateTimeOfLastCheckForUpdatedWebManifest(); > } > > (i.e. merge the two functions) Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:76: storage.updateTimeOfLastCheckForUpdatedWebManifest(); On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > this will still NPE in the first case of storage == null (L51) Miss this part in last round, good catch. I think your previous commends make sense, it seems it is impossible that the storage is null. Remove the storage == null case. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:81: public void onCheckIfUpdateNeededFinished(boolean isUpgraded, WebappInfo newInfo) { On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > s/isUpgraded/needsUpgrade/ Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:122: * Called after the either a request to update the WebAPK has been sent or the update process On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: "the either" -> "either" Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:126: public static void onBuiltWebApk(final boolean success, String webapkPackage) { On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > make private if only called by native Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:49: // The last time that Chrome checks Web Manifest updates for a WebAPK. On 2016/08/11 22:02:44, pkotwicz wrote: > How about: "The last time that Chrome checked for Web Manifest updates for a > WebAPK." Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:58: // Whether the last WebAPK update request succeeds. On 2016/08/11 22:02:43, pkotwicz wrote: > Nit: succeeds -> succeeded Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:455: * Updates the result of whether the last update request to WebAPK Server succeeds. On 2016/08/11 22:02:44, pkotwicz wrote: > Nits: > - "succeeds" -> "succeeded" > - updateDoesLastWebApkUpdateRequestSucceed() -> > updateDidLastWebApkUpdateRequestSucceed() Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:464: * Returns whether the last update request to WebAPK Server succeeds. On 2016/08/11 22:02:44, pkotwicz wrote: > Nits: > - "succeeds" -> "succeeded" > - getDoesLastWebApkUpdateRequestSucceed() -> > getDidLastWebApkUpdateRequestSucceed() Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:91: return GURL(server_url.spec() + webapk_package + "/" + On 2016/08/12 21:16:17, Yaron (limited availability) wrote: > i see what you're saying now about the url. I think this was cause the api team > was trying to make this a restful api but it's not anyway, so perhaps we can get > it removed. Yes, I added the bug number back. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:116: InstallAsyncWithURLRequestContextGetter( On 2016/08/12 21:16:17, Yaron (limited availability) wrote: > there's no thread-hopping anymore - can we just inline these two functions (the > install one and the update one?) The |InstallAsyncWithURLRequestContextGetter| is written for the tests when the URLRequestContextGetter is passed in as a parameter. Same as the |UpdatelAsyncWithURLRequestContextGetter|. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:166: On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: You can steal GetBackgroundTaskRunner() from > https://codereview.chromium.org/2236033003/diff/20001/chrome/browser/android/... Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:62: // after the request to download and install the WebAPK is sent to On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: "request to download and install" -> "request to install" Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:89: // Starts updating of the downloaded WebAPK. Returns whether the updating On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: "updating of the downloaded WebAPK" -> "update using the downloaded WebAPK" > > You are updating to the downloaded WebAPK. The "downloaded WebAPK" does not need > to be updated. Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:52: // WebApkInstaller::StartInstallingDownloadedWebApk() is stubbed out. On 2016/08/11 22:02:45, pkotwicz wrote: > Nit: "WebApkInstaller::StartInstallingDownloadedWebApk() is stubbed out. > WebApkInstaller::StartUpdateUsingDownloadedWebApk() is stubbed out." -> > "WebApkInstaller::StartInstallingDownloadedWebApk() and > WebApkInstaller::StartUpdateUsingDownloadedWebApk() are stubbed out." Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:78: // Runs the WebApkInstaller installation process and blocks till done. On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: "installation" -> "installation/update" Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:97: void RunUpdateWebApk() { On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: "int webapk_version" -> "const int kWebApkVersion" Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:111: WebApkInstaller* createWebApkInstaller() { On 2016/08/11 22:02:44, pkotwicz wrote: > Functions start with uppercase in C++ Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:120: void setupCallback() { On 2016/08/11 22:02:45, pkotwicz wrote: > I think that Run() would be a more appropriate name for this function. (This > function blocks till the callback is called) Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:217: std::string GetServerUrlForCreateWebApk() { On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: This should be a const function Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:221: std::string GetServerUrlForUpdateWebApk() { On 2016/08/11 22:02:44, pkotwicz wrote: > Nit: This should be a const function Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:301: } On 2016/08/11 22:02:45, pkotwicz wrote: > Thanks for adding the test! The tests are well written, so it is not difficult to add more:) https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:62: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2016/08/11 22:02:45, pkotwicz wrote: > Nit: I don't think that the DCHECK is useful (everything occurs on the UI > thread) I would prefer to keep it since ProfileManager::GetLastUsedProfile() has to be called on UI thread. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:88: if (profile == nullptr) { On 2016/08/12 21:16:17, Yaron (limited availability) wrote: > nit: you should do early-outs (precontions) first Done. https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_update_manager.cc:95: static_cast<content::BrowserContext*>(profile), On 2016/08/11 22:02:45, pkotwicz wrote: > Is the static_cast necessary? It seems it isn't anymore. I am sure it is needed before.
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: This issue passed the CQ dry run.
mostly lg - just a few lat comments https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:64: return false; On 2016/08/15 21:38:44, Xi Han wrote: > On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > > i'm confused - why is this disabled? enough moving parts? > > It returns false since I didn't add a logic to update an installed WebAPK. It is > unclear to me whether we installl the downloaded WebAPK. or it is possible to > just "update". Peter has looked into this part. you have to trigger the install the same way as install does or nothing will happen. i'm fine doing this as a follow-up though https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:116: InstallAsyncWithURLRequestContextGetter( On 2016/08/15 21:38:45, Xi Han wrote: > On 2016/08/12 21:16:17, Yaron (limited availability) wrote: > > there's no thread-hopping anymore - can we just inline these two functions > (the > > install one and the update one?) > > The |InstallAsyncWithURLRequestContextGetter| is written for the tests when the > URLRequestContextGetter is passed in as a parameter. Same as the > |UpdatelAsyncWithURLRequestContextGetter|. got it, thanks https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:56: private boolean mIsUpdteCheckComplete = false; mIsUpdateCheckComplete. Also I see we have a mix of Upgrade and Update throughout. At least the callback above should be onUpgradeNeededCheckFinished. https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java (right): https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:48: if (mTab == null || mTab.getWebContents() == null) return; but then you'll never get onDataAvailable, right? I had thought this would move to start() in ManifestUpgradeDetector and return false i that case (so we know to handle it). Alternatively this can return a boolean https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: } hmm, thinking more about this it seems like now would be prime opportunity to report why we failed to start (if it returns false). I'm ok with TODO/bug since this is already big enoguh
PTAL, thanks! https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2184913005/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:64: return false; On 2016/08/16 00:32:03, Yaron (limited availability) wrote: > On 2016/08/15 21:38:44, Xi Han wrote: > > On 2016/08/12 21:16:16, Yaron (limited availability) wrote: > > > i'm confused - why is this disabled? enough moving parts? > > > > It returns false since I didn't add a logic to update an installed WebAPK. It > is > > unclear to me whether we installl the downloaded WebAPK. or it is possible to > > just "update". Peter has looked into this part. > > you have to trigger the install the same way as install does or nothing will > happen. i'm fine doing this as a follow-up though Acknowledged. Since this isn't the focus of this CL, will add it in the follow up one. https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:56: private boolean mIsUpdteCheckComplete = false; On 2016/08/16 00:32:03, Yaron (limited availability) wrote: > mIsUpdateCheckComplete. > > Also I see we have a mix of Upgrade and Update throughout. At least the callback > above should be onUpgradeNeededCheckFinished. Change some "update" to "upgrade." I will be more carefully about the naming. https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java (right): https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcher.java:48: if (mTab == null || mTab.getWebContents() == null) return; On 2016/08/16 00:32:03, Yaron (limited availability) wrote: > but then you'll never get onDataAvailable, right? I had thought this would move > to start() in ManifestUpgradeDetector and return false i that case (so we know > to handle it). Alternatively this can return a boolean I put the logic in the fetcher since it won't break ManifestUpgradeDetectorTest in which we can simply pass a null tab and won't fail due to a null tab and null webcontents. Change the return value to boolean, so we can catch it the ManifestUpgradeDetector::start(). https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2184913005/diff/860001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:60: } On 2016/08/16 00:32:03, Yaron (limited availability) wrote: > hmm, thinking more about this it seems like now would be prime opportunity to > report why we failed to start (if it returns false). I'm ok with TODO/bug since > this is already big enoguh Acknowledged.
lgtm
dominickn@ and dfalcantara@ can you please take a look? Thanks!
dominickn@ and dfalcantara@ ping!
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org, dominickn@chromium.org
dominickn@ and dfalcantara@: I realized that I didn't add you on the reviewer list:(. Could you please take a look? Thanks!
https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:68: public boolean upgradeRequired() { Drive by: Can you remove upgradeRequired() and isUpgradeCheckComplete() and get the values from the callback in the JUnit test?
lgtm % nit. https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:50: public static final String KEY_LAST_CHECK_WEB_MANIFEST_UPDATE_TIME = Do these need to be public? It looks like the only usage is the accessors below.
Patchset #10 (id:900001) has been deleted
Hi Peter: PTAL, thanks! https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:68: public boolean upgradeRequired() { On 2016/08/18 03:24:35, pkotwicz wrote: > Drive by: Can you remove upgradeRequired() and isUpgradeCheckComplete() and get > the values from the callback in the JUnit test? Done. https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2184913005/diff/880001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:50: public static final String KEY_LAST_CHECK_WEB_MANIFEST_UPDATE_TIME = On 2016/08/18 04:39:43, dominickn wrote: > Do these need to be public? It looks like the only usage is the accessors below. Done.
Still LGTM https://codereview.chromium.org/2184913005/diff/920001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/920001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:188: if (upgradeRequired) { ShortcutHelper.encodeBitmapAsString() is expensive and unnecessary. In WebAPKUpdateManager#updateAsync() you call WebappInfo#icon() which decodes the icon. Maybe pass FetchedData to the callback? I am OK if you do this change in a follow up CL https://codereview.chromium.org/2184913005/diff/920001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2184913005/diff/920001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:77: public boolean mIsComplete; Nit: Maybe rename |mIsComplete| to |mWasCalled| ? I think that |mWasCalled| is a more appropriate name for the callback
Description was changed from ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. This CL depends on CL (https://codereview.chromium.org/2228273002/). BUG=631025 ========== to ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 ==========
https://codereview.chromium.org/2184913005/diff/920001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java (right): https://codereview.chromium.org/2184913005/diff/920001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetector.java:188: if (upgradeRequired) { On 2016/08/18 16:18:58, pkotwicz wrote: > ShortcutHelper.encodeBitmapAsString() is expensive and unnecessary. In > WebAPKUpdateManager#updateAsync() you call WebappInfo#icon() which decodes the > icon. Maybe pass FetchedData to the callback? > > I am OK if you do this change in a follow up CL Good catch, fired a bug 639000 to track this. https://codereview.chromium.org/2184913005/diff/920001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java (right): https://codereview.chromium.org/2184913005/diff/920001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:77: public boolean mIsComplete; On 2016/08/18 16:18:58, pkotwicz wrote: > Nit: Maybe rename |mIsComplete| to |mWasCalled| ? I think that |mWasCalled| is a > more appropriate name for the callback Done.
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: Exceeded global retry quota
lgtm % null check https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:45: new WebappRegistry.FetchWebappDataStorageCallback() { Nit: Indentation is deep here; can you pull out the callback construction? https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:69: updateAsync(newInfo); Nit: can just put this on the same line as the if. https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:71: mUpgradeDetector.destroy(); This should be null checked like it is below. https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/browser/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/browser/... chrome/browser/android/webapk/webapk_installer_unittest.cc:92: void RunCreateWebApk() { nit: Should be consistent about Installing vs Creating
https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:45: new WebappRegistry.FetchWebappDataStorageCallback() { On 2016/08/18 20:51:02, dfalcantara wrote: > Nit: Indentation is deep here; can you pull out the callback construction? It doesn't seem very helpful, but I pull out the construction anyway. https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:69: updateAsync(newInfo); On 2016/08/18 20:51:02, dfalcantara wrote: > Nit: can just put this on the same line as the if. Done. https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:71: mUpgradeDetector.destroy(); On 2016/08/18 20:51:02, dfalcantara wrote: > This should be null checked like it is below. Good catch, thanks! https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/browser/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://chromiumcodereview.appspot.com/2184913005/diff/940001/chrome/browser/... chrome/browser/android/webapk/webapk_installer_unittest.cc:92: void RunCreateWebApk() { On 2016/08/18 20:51:02, dfalcantara wrote: > nit: Should be consistent about Installing vs Creating Done.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, yfriedman@chromium.org, pkotwicz@chromium.org, dfalcantara@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2184913005/#ps960001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 ========== to ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:960001)
Message was sent while issue was closed.
Description was changed from ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 ========== to ========== Add calls to the server to request WebAPK updates. Introduce WebApkUpdateManager to manage when to check updates and calls WebApkInstaller in C++ to send update request to WebAPK Minting Server. BUG=631025 Committed: https://crrev.com/4ebb44de7b424be3116c02bf685c7cfa9a38aded Cr-Commit-Position: refs/heads/master@{#412977} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/4ebb44de7b424be3116c02bf685c7cfa9a38aded Cr-Commit-Position: refs/heads/master@{#412977} |
