Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641973003:
Implement server-suggested update check backoff (Closed)
DescriptionImplement server-suggested update check backoff
When updating a certified WebAPK whose Clank-fetched content no longer
matches its Harpoon-fetched content on the server, server will return
a certified WebAPK with resources from the Harpoon-fetched content, and
set the filed "infrequent_updates" field in the Response proto as true
to direct Clank to check for updates less frequently for this WebAPK.
On the Clank, once we see such a flag during updates, we will cache it
in the WebAppDataStorage to avoid frequently request updates, since
Clank will always think the WebAPK's resources don't match its Web
Manifest.
BUG=680128
Review-Url: https://codereview.chromium.org/2641973003
Cr-Commit-Position: refs/heads/master@{#455636}
Committed: https://chromium.googlesource.com/chromium/src/+/35d7ec71638671b99f9490e84612bbfddd7b67b7
Patch Set 1 #
Total comments: 10
Patch Set 2 : 2 #
Total comments: 46
Patch Set 3 : dominickn@ and pkotwicz@'s comments. #
Total comments: 7
Patch Set 4 : dominickn@'s comments. #Patch Set 5 : Rebase. #
Total comments: 41
Patch Set 6 : Nits. #
Total comments: 8
Patch Set 7 : Nits and rebase. #
Total comments: 5
Patch Set 8 : Fix tests failure. #Patch Set 9 : Nits. #
Total comments: 2
Messages
Total messages: 56 (33 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implement server-suggested update check backoff BUG=680128 ========== to ========== Implement server-suggested update check backoff When updating a certified WebAPK whose Clank-fetched content no longer matches its Harpoon-fetched content on the server, server will return a certified WebAPK with resources from the Harpoon-fetched content, and set the filed "infrequent_updates" field in the Response proto as true to direct Clank to check for updates less frequently for this WebAPK. On the Clank, once we see such a flag during updates, we will cache it in the WebAppDataStorage to avoid frequently request updates, since Clank will always think the WebAPK's resources don't match its Web Manifest. BUG=680128 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, this is a follow up CL for CL (https://codereview.chromium.org/2632243004/). Could you please take a look? Thanks!
Still looking at the CL. Some preliminary comments https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:420: We should set |less_updates_| from within the if (task_type_ == UPDATE && signed_download_url.is_empty()) {} statement in https://codereview.chromium.org/2632243004 https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:44: struct FinishCallbackData { Can we change FinishCallback to have three arguments instead of introducing this struct? https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:284: // Whether to update an WebAPK less frequently. How about: "Whether the server wants the WebAPK to request updates less frequently." How about renaming |infrequent_updates_| to |less_updates_| I prefer "less" than "infrequent" because "less" is a command. "Less" is what the server is requesting from the client.
I think that's it for comments for this round of review https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:255: storage.updateInfrequentUpdates(isInfrequentUpdates.booleanValue()); Nit: You should get the type conversion for free. booleanValue() is redundant https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:409: * Updates the result of whether to check updates less frequently. Nit: "Updates the result of whether" -> "Updates whether"
Your CL generally looks good
Patchset #2 (id:40001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:255: storage.updateInfrequentUpdates(isInfrequentUpdates.booleanValue()); On 2017/02/10 20:53:14, pkotwicz wrote: > Nit: You should get the type conversion for free. booleanValue() is redundant Done. https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:409: * Updates the result of whether to check updates less frequently. On 2017/02/10 20:53:14, pkotwicz wrote: > Nit: "Updates the result of whether" -> "Updates whether" Done. https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:420: On 2017/02/10 02:51:22, pkotwicz wrote: > We should set |less_updates_| from within the > > if (task_type_ == UPDATE && signed_download_url.is_empty()) {} statement in > https://codereview.chromium.org/2632243004 Done. https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:44: struct FinishCallbackData { On 2017/02/10 02:51:22, pkotwicz wrote: > Can we change FinishCallback to have three arguments instead of introducing this > struct? Done. https://codereview.chromium.org/2641973003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:284: // Whether to update an WebAPK less frequently. On 2017/02/10 02:51:22, pkotwicz wrote: > How about: "Whether the server wants the WebAPK to request updates less > frequently." > > How about renaming |infrequent_updates_| to |less_updates_| > I prefer "less" than "infrequent" because "less" is a command. "Less" is what > the server is requesting from the client. Done.
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
Dominick, can you please take a look? The C++ looks good to me. I am unsure about the variables names in the Java files. Other than that the CL looks good https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:408: private static void onBuiltWebApk(String id, boolean success, boolean isInfrequentUpdates) { Nit: isInfrequentUpdates -> lessUpdates https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:436: void updateInfrequentUpdates(boolean isInfrequentUpdates) { I can't think of a good name for the setter. Perhaps store how often to do the update checks instead? updateWebApkUpdateRequestInterval() perhaps? https://codereview.chromium.org/2641973003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:413: * Test that if a WebAPK is expected to check update less frequently, the is-update-needed Nit: "update" -> "updates" https://codereview.chromium.org/2641973003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:421: getStorage().delete(); Why do you delete the storage? https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:421: less_updates_ = response->infrequent_updates(); Nit: Rename the field in the proto to |less_updates| https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:47: // WebAPK server. Nit: "don't update so frequently" -> "request updates less frequently"
https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:35: public static final long FULL_CHECK_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(3L); Rename this to UPDATE_INTERVAL https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:50: * crbug.com/680128. Number of milliseconds between checks of updates for a WebAPK that is Nit: misaligned * at the start of the comment Nit: I would put the crbug link after the comment explaining what it is. Nit: move this below UPDATE_INTERVAL https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:327: boolean isInfrequentUpdates = storage.getInfrequentUpdates(); I feel like this computation should be pushed into the WebappDataStorage. That way, the update manager can just do: return storage.shouldUpdate(); This would mean moving all of the intervals into WebappDataStorage as constants. WDYT? https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:344: WebappDataStorage storage, boolean success, Boolean isInfrequentUpdates) { Why does this need to be a Boolean object that's nullable as opposed to a boolean value? Don't we assume a regular update schedule unless explicitly instructed otherwise? https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:350: storage.updateInfrequentUpdates(isInfrequentUpdates); storage.shouldRelaxUpdates(shouldRelaxUpdates). This would work better with the WebappDataStorage just having a shouldUpdate() method so all of the update interval calculations happen inside it. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:436: void updateInfrequentUpdates(boolean isInfrequentUpdates) { setRelaxedUpdates()? https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:443: boolean getInfrequentUpdates() { shouldRelaxUpdates()? https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:382: bool less_updates) { relax_updates https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:124: void OnWebApkInstallFinished(bool success, An aside: the argument ordering here is a little unintuitive. At some point, can the webapk_package_name be first, and the bools together (not necessarily in this CL)? https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:126: bool less_updates); relax_updates https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_install_service.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_install_service.cc:68: bool less_updates) { relax_updates https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_install_service.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_install_service.h:65: bool less_updates); relax_updates https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:421: less_updates_ = response->infrequent_updates(); On 2017/02/14 00:15:13, pkotwicz wrote: > Nit: Rename the field in the proto to |less_updates| I'd call it relax_updates https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:48: using FinishCallback = base::Callback<void(bool, const std::string&, bool)>; Urgh this bool string bool argument ordering is not nice... :) https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:276: bool less_updates_; relax_updates_ https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_update_manager.cc:34: bool less_updates) { relax_updates https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_update_manager.cc:121: base::Bind(&WebApkUpdateManager::OnBuiltWebApk, id, false, "", false)); Can you annotate the bool arguments here with an inline comment explaining what they are?
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
I think I have addressed all the comments except moving the logic for updates to WebappDataStorage, since I would like to keep WebappDataStorage as a place to store values, not with complex logic. PTAL, thanks! https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:35: public static final long FULL_CHECK_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(3L); On 2017/02/14 04:13:33, dominickn wrote: > Rename this to UPDATE_INTERVAL Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:50: * crbug.com/680128. Number of milliseconds between checks of updates for a WebAPK that is On 2017/02/14 04:13:33, dominickn wrote: > Nit: misaligned * at the start of the comment > > Nit: I would put the crbug link after the comment explaining what it is. > > Nit: move this below UPDATE_INTERVAL Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:327: boolean isInfrequentUpdates = storage.getInfrequentUpdates(); On 2017/02/14 04:13:34, dominickn wrote: > I feel like this computation should be pushed into the WebappDataStorage. That > way, the update manager can just do: > > return storage.shouldUpdate(); > > This would mean moving all of the intervals into WebappDataStorage as constants. > WDYT? Personally I would prefer to leave all these logic in WebApkUpdateManager, and use WebAppDataStorage as a place to cache values. Especially all existing tests are written for WebApkUpdateManager, we have to move tests to test these logic. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:344: WebappDataStorage storage, boolean success, Boolean isInfrequentUpdates) { On 2017/02/14 04:13:33, dominickn wrote: > Why does this need to be a Boolean object that's nullable as opposed to a > boolean value? Don't we assume a regular update schedule unless explicitly > instructed otherwise? Hmmm, I guess a boolean works too. Updated. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:350: storage.updateInfrequentUpdates(isInfrequentUpdates); On 2017/02/14 04:13:33, dominickn wrote: > storage.shouldRelaxUpdates(shouldRelaxUpdates). This would work better with the > WebappDataStorage just having a shouldUpdate() method so all of the update > interval calculations happen inside it. I would prefer to keep the logic in here. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:408: private static void onBuiltWebApk(String id, boolean success, boolean isInfrequentUpdates) { On 2017/02/14 00:15:12, pkotwicz wrote: > Nit: isInfrequentUpdates -> lessUpdates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:436: void updateInfrequentUpdates(boolean isInfrequentUpdates) { On 2017/02/14 04:13:34, dominickn wrote: > setRelaxedUpdates()? Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:443: boolean getInfrequentUpdates() { On 2017/02/14 04:13:34, dominickn wrote: > shouldRelaxUpdates()? I would still prefer to name it getRelaxUpdates(). https://codereview.chromium.org/2641973003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:413: * Test that if a WebAPK is expected to check update less frequently, the is-update-needed On 2017/02/14 00:15:13, pkotwicz wrote: > Nit: "update" -> "updates" Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:421: getStorage().delete(); On 2017/02/14 00:15:12, pkotwicz wrote: > Why do you delete the storage? Reverted. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:382: bool less_updates) { On 2017/02/14 04:13:34, dominickn wrote: > relax_updates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:124: void OnWebApkInstallFinished(bool success, On 2017/02/14 04:13:34, dominickn wrote: > An aside: the argument ordering here is a little unintuitive. At some point, can > the webapk_package_name be first, and the bools together (not necessarily in > this CL)? I move the webapk_package_name to be the last one, and put the two bool together. Is it ok? https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:126: bool less_updates); On 2017/02/14 04:13:34, dominickn wrote: > relax_updates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_install_service.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_install_service.cc:68: bool less_updates) { On 2017/02/14 04:13:34, dominickn wrote: > relax_updates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_install_service.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_install_service.h:65: bool less_updates); On 2017/02/14 04:13:34, dominickn wrote: > relax_updates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:421: less_updates_ = response->infrequent_updates(); On 2017/02/14 04:13:34, dominickn wrote: > On 2017/02/14 00:15:13, pkotwicz wrote: > > Nit: Rename the field in the proto to |less_updates| > > I'd call it relax_updates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:47: // WebAPK server. On 2017/02/14 00:15:13, pkotwicz wrote: > Nit: "don't update so frequently" -> "request updates less frequently" Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:48: using FinishCallback = base::Callback<void(bool, const std::string&, bool)>; On 2017/02/14 04:13:34, dominickn wrote: > Urgh this bool string bool argument ordering is not nice... :) Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:276: bool less_updates_; On 2017/02/14 04:13:34, dominickn wrote: > relax_updates_ Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_update_manager.cc (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_update_manager.cc:34: bool less_updates) { On 2017/02/14 04:13:34, dominickn wrote: > relax_updates Done. https://codereview.chromium.org/2641973003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_update_manager.cc:121: base::Bind(&WebApkUpdateManager::OnBuiltWebApk, id, false, "", false)); On 2017/02/14 04:13:34, dominickn wrote: > Can you annotate the bool arguments here with an inline comment explaining what > they are? 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:327: boolean isInfrequentUpdates = storage.getInfrequentUpdates(); On 2017/02/14 22:25:46, Xi Han(OOO till Mar. 3) wrote: > On 2017/02/14 04:13:34, dominickn wrote: > > I feel like this computation should be pushed into the WebappDataStorage. That > > way, the update manager can just do: > > > > return storage.shouldUpdate(); > > > > This would mean moving all of the intervals into WebappDataStorage as > constants. > > WDYT? > > Personally I would prefer to leave all these logic in WebApkUpdateManager, and > use WebAppDataStorage as a place to cache values. Especially all existing tests > are written for WebApkUpdateManager, we have to move tests to test these logic. I'm not sure why the tests would need to be rewritten? The only thing to change is that you'd have WebappDataStorage.UPDATE_INTERVAL instead of WebApkManager.UPDATE_INTERVAL, RETRY_UPDATE_DURATION, etc. In fact, the mock clock used in tests is from WebappDataStorage, which further suggests that all the time based calculations should be in there. https://codereview.chromium.org/2641973003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:350: storage.updateInfrequentUpdates(isInfrequentUpdates); On 2017/02/14 22:25:46, Xi Han(OOO till Mar. 3) wrote: > On 2017/02/14 04:13:33, dominickn wrote: > > storage.shouldRelaxUpdates(shouldRelaxUpdates). This would work better with > the > > WebappDataStorage just having a shouldUpdate() method so all of the update > > interval calculations happen inside it. > > I would prefer to keep the logic in here. I think it makes much more sense for this logic to be in WebappDataStorage. For instance, whether or not a webapp was launched recently has its computation done in there. https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:41: public static final long INFREQUENT_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(30L); Make this RELAXED_UPDATE_INTERVAL (sorry I missed this previously!) https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:326: long now = currentTimeMillis(); currentTimeMillis() is overridden in tests to call the WebappDataStorage clock method. Makes more sense to just put all the time delta calculations directly in WebappDataStorage - then you can remove this method and the overrides and it should just work. https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:443: boolean getRelaxUpdates() { shouldRelaxUpdates reads more naturally than getRelaxUpdates. https://codereview.chromium.org/2641973003/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:415: public void testInfrequentUpdateIntervalIfNoPriorWebApkUpdate() { testRelaxedUpdateIntervalIfNoPriorWebApkUpdate()
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
Hi Dominick, I have moved the logic of whether to update a WebAPK into WebappDataStorage as discussed offline. PTAL, thanks! https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:41: public static final long INFREQUENT_UPDATE_INTERVAL = TimeUnit.DAYS.toMillis(30L); On 2017/02/15 05:58:32, dominickn wrote: > Make this RELAXED_UPDATE_INTERVAL (sorry I missed this previously!) Done. https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:443: boolean getRelaxUpdates() { On 2017/02/15 05:58:32, dominickn wrote: > shouldRelaxUpdates reads more naturally than getRelaxUpdates. Done. https://codereview.chromium.org/2641973003/diff/140001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/140001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:415: public void testInfrequentUpdateIntervalIfNoPriorWebApkUpdate() { On 2017/02/15 05:58:32, dominickn wrote: > testRelaxedUpdateIntervalIfNoPriorWebApkUpdate() Done.
Just nits! https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:402: long getLastWebApkUpdateRequestCompletionTime() { Can you make some of the time getters private now? https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:448: boolean didPreviousUpdateSucceed() { Nit: Rename this to didPreviousWebApkUpdateSucceed(). The comment explicitly mentions WebAPKs https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:458: * Sets whether we should check updates less frequently. "check for updates" here too I am unsure whether you should have "WebApk" in the new method names e.g. setRelaxedWebApkUpdates(). I will leave this up to Dominick to decide. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:464: /** Nit: - "check updates" -> "check for updates" - I think you can put the comment on one line https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:472: * Returns whether we should check update. Nits: - "check update." -> "check for update." - You can put the comment on one line /** Returns whether we should check for update. */ https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:474: boolean shouldUpdate() { Nit: Maybe rename this function to shouldCheckForUpdate()? https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:198: private WebappDataStorage getStorage() { Some of the tests in this file look like duplicates of the tests in WebappDataStorageTest Can you return a subclass of WebappDataStorage which mocks out WebappDataStorage#shouldUpdate() and WebappDataStorage#didPreviousUpdateSucceed() TestWebappDataStorage would have TestWebappDataStorage#setShouldUpdate() WebappDataStorage#didPreviousUpdateSucceed() would just return WebappDataStorage#getDidLastWebApkUpdateRequestSucceed() You can set a custom factory for WebappDataStorage via WebappDatStorage#setFactoryForTests() https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:329: Nit: Remove new line https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:333: storage.updateTimeOfLastCheckForUpdatedWebManifest(); Nit: Can you please add a new line? This way it is clear that the comment on line 332 does not apply to line 334 https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:349: Nit: Remove new line https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:363: assertTrue(WebappDataStorage.RELAXED_UPDATE_INTERVAL > WebappDataStorage.UPDATE_INTERVAL); Is this assertion relevant to this test? https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:367: Nit: Remove new line https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:33: // update requests too often. This is not really correct. The client can determine whether the WebAPK needs an update. However, sometimes the server is not able to fulfill an update request for technical reasons How about: "This flag may be used, for example, if the WebAPK server is unable to fulfill an update request and likely won't be able to fulfill subsequent update requests from this WebAPK. One case where the server is unable to fulfill update requests is if the Web Manifest is dynamically generated based on the user's country etc. Therefore, we want the client ..." https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:356: relax_updates_(false), Nit: Can you initialize |webapk_version_| ? https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:425: relax_updates_ = response->relax_updates(); Nit: Move the setting of |relax_updates_| after the comment
https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:91: mPreviousUpdateSucceeded = storage.didPreviousUpdateSucceed(); crrev.com/2725813004 makes the WebappDataStorage a member of this class. I'll let you decide which should land first, but when that change comes through, you won't need to store mPreviousUpdateSucceeded any more (just call mStorage.didPreviousUpdatedSucceed() in onGotManifestData()). https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:158: recordUpdate(storage, true, false); Nit: can you annotate call to recordUpdate with an explanation of what the variable are? recordUpdate(storage, true, /* success */ false /* relaxUpdates */); https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:402: long getLastWebApkUpdateRequestCompletionTime() { On 2017/03/04 00:20:47, pkotwicz wrote: > Can you make some of the time getters private now? Yes, I think all of the time stuff can be private to this class now. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:448: boolean didPreviousUpdateSucceed() { On 2017/03/04 00:20:46, pkotwicz wrote: > Nit: Rename this to didPreviousWebApkUpdateSucceed(). The comment explicitly > mentions WebAPKs I think we can leave the WebAPK bit out of the method name. WebAPKs are the only thing that can update right now, and if we have something in the future that also updates we can differentiate then. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:458: * Sets whether we should check updates less frequently. On 2017/03/04 00:20:46, pkotwicz wrote: > "check for updates" here too > > I am unsure whether you should have "WebApk" in the new method names e.g. > setRelaxedWebApkUpdates(). I will leave this up to Dominick to decide. See above comment - we can probably leave WebApk out of the method name since it's the only thing using this code path. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:474: boolean shouldUpdate() { On 2017/03/04 00:20:46, pkotwicz wrote: > Nit: Maybe rename this function to shouldCheckForUpdate()? shouldCheckForUpdate SGTM https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:198: private WebappDataStorage getStorage() { Agreed. WebappDataStorageTest tests that the update checking returns the right thing when it should. This class should just check that the right thing is done when we are and are not checking for updates. Having a TestWebappDataStorage is the right approach. https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_install_service.h (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_install_service.h:30: using FinishCallback = WebApkInstaller::FinishCallback; Minor nit: instead of having this defined in two places, it would be nice just to have one definition, and then use that.
Patchset #6 (id:200001) has been deleted
I think I have addressed all the comments. PTAL, thanks! https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:91: mPreviousUpdateSucceeded = storage.didPreviousUpdateSucceed(); On 2017/03/06 02:28:36, dominickn wrote: > crrev.com/2725813004 makes the WebappDataStorage a member of this class. I'll > let you decide which should land first, but when that change comes through, you > won't need to store mPreviousUpdateSucceeded any more (just call > mStorage.didPreviousUpdatedSucceed() in onGotManifestData()). Thanks for letting me know. I will talk to Felix. For my CL, I will remove the |mPreviousUpdateSucceeded| too. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:158: recordUpdate(storage, true, false); On 2017/03/06 02:28:36, dominickn wrote: > Nit: can you annotate call to recordUpdate with an explanation of what the > variable are? > > recordUpdate(storage, true, /* success */ false /* relaxUpdates */); Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:402: long getLastWebApkUpdateRequestCompletionTime() { On 2017/03/06 02:28:36, dominickn wrote: > On 2017/03/04 00:20:47, pkotwicz wrote: > > Can you make some of the time getters private now? > > Yes, I think all of the time stuff can be private to this class now. I would love to, but some of them are called in tests, like WebApkUpdateManagerTest. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:448: boolean didPreviousUpdateSucceed() { On 2017/03/06 02:28:36, dominickn wrote: > On 2017/03/04 00:20:46, pkotwicz wrote: > > Nit: Rename this to didPreviousWebApkUpdateSucceed(). The comment explicitly > > mentions WebAPKs > > I think we can leave the WebAPK bit out of the method name. WebAPKs are the only > thing that can update right now, and if we have something in the future that > also updates we can differentiate then. Acknowledged. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:458: * Sets whether we should check updates less frequently. On 2017/03/06 02:28:36, dominickn wrote: > On 2017/03/04 00:20:46, pkotwicz wrote: > > "check for updates" here too > > > > I am unsure whether you should have "WebApk" in the new method names e.g. > > setRelaxedWebApkUpdates(). I will leave this up to Dominick to decide. > > See above comment - we can probably leave WebApk out of the method name since > it's the only thing using this code path. Acknowledged. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:464: /** On 2017/03/04 00:20:46, pkotwicz wrote: > Nit: > - "check updates" -> "check for updates" > - I think you can put the comment on one line Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:472: * Returns whether we should check update. On 2017/03/04 00:20:47, pkotwicz wrote: > Nits: > - "check update." -> "check for update." > - You can put the comment on one line /** Returns whether we should check for > update. */ Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:474: boolean shouldUpdate() { On 2017/03/06 02:28:36, dominickn wrote: > On 2017/03/04 00:20:46, pkotwicz wrote: > > Nit: Maybe rename this function to shouldCheckForUpdate()? > > shouldCheckForUpdate SGTM Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:198: private WebappDataStorage getStorage() { On 2017/03/06 02:28:36, dominickn wrote: > Agreed. WebappDataStorageTest tests that the update checking returns the right > thing when it should. This class should just check that the right thing is done > when we are and are not checking for updates. Having a TestWebappDataStorage is > the right approach. Thanks for the suggestions. I moved these tests to WebappDataStorageTest since they look redundant anyway. After the clean up, I don't think it is necessary to introduce a TestWebappDataStorage. What do you think? https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:329: On 2017/03/04 00:20:47, pkotwicz wrote: > Nit: Remove new line Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:333: storage.updateTimeOfLastCheckForUpdatedWebManifest(); On 2017/03/04 00:20:47, pkotwicz wrote: > Nit: Can you please add a new line? This way it is clear that the comment on > line 332 does not apply to line 334 Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:349: On 2017/03/04 00:20:47, pkotwicz wrote: > Nit: Remove new line Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:363: assertTrue(WebappDataStorage.RELAXED_UPDATE_INTERVAL > WebappDataStorage.UPDATE_INTERVAL); On 2017/03/04 00:20:47, pkotwicz wrote: > Is this assertion relevant to this test? Good catch, removed. https://codereview.chromium.org/2641973003/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:367: On 2017/03/04 00:20:47, pkotwicz wrote: > Nit: Remove new line Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:33: // update requests too often. On 2017/03/04 00:20:47, pkotwicz wrote: > This is not really correct. The client can determine whether the WebAPK needs an > update. However, sometimes the server is not able to fulfill an update request > for technical reasons > > How about: "This flag may be used, for example, if the WebAPK server is unable > to fulfill an update request and likely won't be able to fulfill subsequent > update requests from this WebAPK. One case where the server is unable to fulfill > update requests is if the Web Manifest is dynamically generated based on the > user's country etc. Therefore, we want the client ..." Makes sense, thanks for the better description. https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_install_service.h (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_install_service.h:30: using FinishCallback = WebApkInstaller::FinishCallback; On 2017/03/06 02:28:36, dominickn wrote: > Minor nit: instead of having this defined in two places, it would be nice just > to have one definition, and then use that. It is a little bit redundant, but I couldn't find a better place to put it:( Introducing another independent FinishCallback class will add complexity to the installer. Unless we can find an obvious way to clean it up, we may just keep it as it is now. https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:356: relax_updates_(false), On 2017/03/04 00:20:47, pkotwicz wrote: > Nit: Can you initialize |webapk_version_| ? Done. https://codereview.chromium.org/2641973003/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:425: relax_updates_ = response->relax_updates(); On 2017/03/04 00:20:47, pkotwicz wrote: > Nit: Move the setting of |relax_updates_| after the comment Done.
Mostly nits now https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:327: * The is-update-needed check is the first step in retrying to update the WebAPK. Nit: can you move the last line of this comment up to the end of the previous line https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:352: // Verifies that {@link WebappDataStorage#ShouldUpdate()} returns true no matter whether we Nit: shouldCheckForUpdate(). Nit: clarify that we are returning true because the previous update failed. https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:383: public void testShouldUpdateIfRelaxedUpdatesAndNoPriorWebApkUpdate() { Rename this to testRelaxedUpdates, since it also tests that when relaxed updates is true, the update is not requested within the normal update interval. https://codereview.chromium.org/2641973003/diff/220001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:357: webapk_version_(1), Nit: can you set the 1 as a constant in the anonymous namespace in this file?
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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 hanxi@chromium.org
Patchset #7 (id:240001) has been deleted
I am not sure why the WebappRegisterTest failed on bot linux_android_rel_ng(https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/245906/steps/chrome_junit_tests%20%28with%20patch%29.chrome_junit_tests%20%28with%20patch%29/logs/stdio), but it looks like the WebappDataStorage doesn't clean up after WebappDataStorageTest, and it has influences the WebappRegisterTest for that bot, since they all open a storage called "test". It shouldn't happen though. I can't reproduce it locally, so just try to fix it. PTAL, thanks! https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:327: * The is-update-needed check is the first step in retrying to update the WebAPK. On 2017/03/07 02:15:12, dominickn wrote: > Nit: can you move the last line of this comment up to the end of the previous > line Done. https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:352: // Verifies that {@link WebappDataStorage#ShouldUpdate()} returns true no matter whether we On 2017/03/07 02:15:12, dominickn wrote: > Nit: shouldCheckForUpdate(). > > Nit: clarify that we are returning true because the previous update failed. Done. https://codereview.chromium.org/2641973003/diff/220001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:383: public void testShouldUpdateIfRelaxedUpdatesAndNoPriorWebApkUpdate() { On 2017/03/07 02:15:12, dominickn wrote: > Rename this to testRelaxedUpdates, since it also tests that when relaxed updates > is true, the update is not requested within the normal update interval. Done. https://codereview.chromium.org/2641973003/diff/220001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2641973003/diff/220001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:357: webapk_version_(1), On 2017/03/07 02:15:12, dominickn wrote: > Nit: can you set the 1 as a constant in the anonymous namespace in this file? Done.
It seems my fix in patchset#8 doesn't work:(
LGTM with nits https://codereview.chromium.org/2641973003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:54: static final String KEY_LAST_WEBAPK_UPDATE_REQUEST_COMPLETE_TIME = Nit: Remove "WEBAPK" from the variable name for the sake of constency https://codereview.chromium.org/2641973003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:58: static final String KEY_DID_LAST_WEBAPK_UPDATE_REQUEST_SUCCEED = Nit: Remove "WEBAPK" from the variable name for the sake of constency https://codereview.chromium.org/2641973003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:402: long getLastWebApkUpdateRequestCompletionTime() { This can be private too? https://codereview.chromium.org/2641973003/diff/280001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2641973003/diff/280001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:355: // Verifies that {@link WebappDataStorage#ShouldCheckForUpdate()} returns true because the Nit: ShouldCheckForUpdate() -> shouldCheckForUpdate() https://codereview.chromium.org/2641973003/diff/280001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:381: * Test that if there was no previous WebAPK update attempt and the relax-update is set to true, Nit: "the relax-update" -> "the relax-update flag"
https://codereview.chromium.org/2641973003/diff/260001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/260001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:340: WebappDataStorage.setClockForTests(mClock); For the sake of our future sanity, can you reset the clock in tearDown() :) https://codereview.chromium.org/2641973003/diff/280001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java (right): https://codereview.chromium.org/2641973003/diff/280001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:113: mClock = clock; Nit: I don't think that |mClock| or the clock parameter is needed anymore
Patchset #8 (id:280001) has been deleted
Hi Dominick, PTAL, thanks! https://codereview.chromium.org/2641973003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:54: static final String KEY_LAST_WEBAPK_UPDATE_REQUEST_COMPLETE_TIME = On 2017/03/07 23:00:37, pkotwicz wrote: > Nit: Remove "WEBAPK" from the variable name for the sake of constency Done. https://codereview.chromium.org/2641973003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:58: static final String KEY_DID_LAST_WEBAPK_UPDATE_REQUEST_SUCCEED = On 2017/03/07 23:00:37, pkotwicz wrote: > Nit: Remove "WEBAPK" from the variable name for the sake of constency Done. https://codereview.chromium.org/2641973003/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:400: long getLastWebApkUpdateRequestCompletionTime() { This function is called by WebApkUpdateManagerTest, so we can't make it private.
Still LGTM https://codereview.chromium.org/2641973003/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2641973003/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:400: long getLastWebApkUpdateRequestCompletionTime() { Fair enough
lgtm, thanks
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1489022416934790,
"parent_rev": "67dd9f2795389cc3776481ff07740984b2389f43", "commit_rev":
"35d7ec71638671b99f9490e84612bbfddd7b67b7"}
Message was sent while issue was closed.
Description was changed from ========== Implement server-suggested update check backoff When updating a certified WebAPK whose Clank-fetched content no longer matches its Harpoon-fetched content on the server, server will return a certified WebAPK with resources from the Harpoon-fetched content, and set the filed "infrequent_updates" field in the Response proto as true to direct Clank to check for updates less frequently for this WebAPK. On the Clank, once we see such a flag during updates, we will cache it in the WebAppDataStorage to avoid frequently request updates, since Clank will always think the WebAPK's resources don't match its Web Manifest. BUG=680128 ========== to ========== Implement server-suggested update check backoff When updating a certified WebAPK whose Clank-fetched content no longer matches its Harpoon-fetched content on the server, server will return a certified WebAPK with resources from the Harpoon-fetched content, and set the filed "infrequent_updates" field in the Response proto as true to direct Clank to check for updates less frequently for this WebAPK. On the Clank, once we see such a flag during updates, we will cache it in the WebAppDataStorage to avoid frequently request updates, since Clank will always think the WebAPK's resources don't match its Web Manifest. BUG=680128 Review-Url: https://codereview.chromium.org/2641973003 Cr-Commit-Position: refs/heads/master@{#455636} Committed: https://chromium.googlesource.com/chromium/src/+/35d7ec71638671b99f9490e84612... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:320001) as https://chromium.googlesource.com/chromium/src/+/35d7ec71638671b99f9490e84612... |
