Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790213004:
Don't try to update incessently when Shell APK version is out of date. (Closed)
DescriptionDon't try to update incessently when Shell APK version is out of date.
After the shell_apk_version increases in Chrome, it takes sometime before the
new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request
update each time it is launched, we add a minimum delay between each request
that is due to shell apk version is low.
BUG=707990
Review-Url: https://codereview.chromium.org/2790213004
Cr-Commit-Position: refs/heads/master@{#465580}
Committed: https://chromium.googlesource.com/chromium/src/+/6d0fd5239302fd191a795053515111df9f9fffbb
Patch Set 1 #
Total comments: 2
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 5
Patch Set 3 : Update the checkUpdate logic. #Patch Set 4 : Introduce LastRequestShellApkVersion. #
Total comments: 10
Patch Set 5 : Nits. #
Total comments: 12
Patch Set 6 : Move the shell apk version check back to WebApkUpdateManager. #
Messages
Total messages: 49 (24 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Don't try to update incessently when Shell APK version is out of date. After the shell_apk_version increases in Chrome, it takes sometime before the new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request update each time it is moved to foreground, we add a minimum delay between each request that is due to shell apk version is low. BUG=707990 ========== to ========== Don't try to update incessently when Shell APK version is out of date. After the shell_apk_version increases in Chrome, it takes sometime before the new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request update each time it is launched, we add a minimum delay between each request that is due to shell apk version is low. BUG=707990 ==========
Description was changed from ========== Don't try to update incessently when Shell APK version is out of date. After the shell_apk_version increases in Chrome, it takes sometime before the new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request update each time it is launched, we add a minimum delay between each request that is due to shell apk version is low. BUG=707990 ========== to ========== Don't try to update incessently when Shell APK version is out of date. After the shell_apk_version increases in Chrome, it takes sometime before the new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request update each time it is launched, we add a minimum delay between each request that is due to shell apk version is low. BUG=707990 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
https://codereview.chromium.org/2790213004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:290: if (isShellApkVersionOutOfDate(info) && mStorage.shouldRequestShellApkUpdate()) return true; Can we remove this if(){} statement entirely? I think that it is OK to only check updates every 3 days. I think that your CL changes the logic to try updates every 2 days if the ShellAPK is updated. A 2 day interval is very similar to a 3 day interval. Of course, if the update frequency is relaxed, it is possible that a user needs to wait for 30 days before getting an update. Right now the ShellAPK updates we are making are non critical. Waiting for 30 days does not sound that bad
PTAL, thanks! https://codereview.chromium.org/2790213004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:290: if (isShellApkVersionOutOfDate(info) && mStorage.shouldRequestShellApkUpdate()) return true; On 2017/04/04 22:29:57, pkotwicz wrote: > Can we remove this if(){} statement entirely? I think that it is OK to only > check updates every 3 days. > > I think that your CL changes the logic to try updates every 2 days if the > ShellAPK is updated. A 2 day interval is very similar to a 3 day interval. > > Of course, if the update frequency is relaxed, it is possible that a user needs > to wait for 30 days before getting an update. Right now the ShellAPK updates we > are making are non critical. Waiting for 30 days does not sound that bad I think 3 day is also good, but we'd better not wait for 30 days for the shell update.
Why is waiting 30 days bad from a user's point of view? Chrome updates every 45 days Something which is probably bad is that if the user clears Chrome's data often (i.e. WebappDataStorage is cleared) it is possible for WebAPKs to never update. That's a problem for a separate CL though
As discussed offline, we will keep using the regular update check interval for shell_version update, since WebAPK server will return a different APK for the request. I will fix the "not update issue" in another CL. PTAL, thanks!
https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:473: shouldRelaxUpdates() ? RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL; How about making |checkUpdatesInterval| depend on |isShellApkVersionOutOfDate| In particular: long checkUpdatesInterval = shouldRelaxUpdates && !isShellApkVersionOutOfDate ? RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL;
https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:473: shouldRelaxUpdates() ? RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL; On 2017/04/06 17:36:52, pkotwicz wrote: > How about making |checkUpdatesInterval| depend on |isShellApkVersionOutOfDate| > > In particular: > > long checkUpdatesInterval = shouldRelaxUpdates && !isShellApkVersionOutOfDate ? > RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL; I guess you want to merge the two checks together, right? I would love to, but these two checks are comparing different time. For shell version particular, the interval depends on when the last update failed time. I also don't think we want to merge the first check with the last check in line 478, since it will mess up the logic. It isn't clean to put the shell_version check between the two manifest checks.
https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:473: shouldRelaxUpdates() ? RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL; I see. If the previous update failed, your code will make us check for updates less frequently. RETRY_UPDATE_DURATION < UPDATE_INTERVAL
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:473: shouldRelaxUpdates() ? RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL; On 2017/04/06 19:12:59, pkotwicz wrote: > I see. If the previous update failed, your code will make us check for updates > less frequently. RETRY_UPDATE_DURATION < UPDATE_INTERVAL If the previous failure is for an update request caused by Web Manifest update, we still follow the existing time line (retry + regular update / relaxed update interval).
https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:473: shouldRelaxUpdates() ? RELAXED_UPDATE_INTERVAL : UPDATE_INTERVAL; On 2017/04/10 17:06:51, Xi Han wrote: > On 2017/04/06 19:12:59, pkotwicz wrote: > > I see. If the previous update failed, your code will make us check for updates > > less frequently. RETRY_UPDATE_DURATION < UPDATE_INTERVAL > > If the previous failure is for an update request caused by Web Manifest update, > we still follow the existing time line (retry + regular update / relaxed update > interval). Please ignore my comment here.
Patchset #3 (id:120001) has been deleted
Patchset #4 (id:160001) has been deleted
PTAL, thanks!
PTAL, thanks!
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.
LGTM. Thank you for bearing with me https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); Can this call be moved to recordUpdate() ? https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:404: */ How about: "Tests that a WebAPK update is requested immediately if: - the Shell APK is out of date AND - there wasn't a previous request for this ShellAPK version." https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:407: final TestClock clock = new TestClock(System.currentTimeMillis()); - Can you please call WebappDataStorage#updateLastRequestShellApkVersion() in the test case "set up"? - Don't you need to call WebappDataStorage#updateTimeOfLastWebApkUpdateRequestCompletion(). Otherwise the test is not very meaningful - Can you please add the comment: "There have not been any update requests for the current ShellAPK version. A WebAPK update should be requested immediately." https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:412: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); - Can you please add a new line. - Can you please add the comment: "A previous update request was made for the current ShellAPK version. A WebAPK update should be requested after the regular delay."
Patchset #5 (id:200001) has been deleted
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Thank you Peter for correcting my mistake in the previous patch:) Hi Dom, could you please take a look? Thanks! https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); On 2017/04/12 02:09:44, pkotwicz wrote: > Can this call be moved to recordUpdate() ? I have thought about it before, but I think it only needs to be called here. This is because recordUpdate() is called in another two places before the request goes to native code, the case that we care is: after the response is received from the WAM server and we know the shell APK isn't updated. https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:404: */ On 2017/04/12 02:09:44, pkotwicz wrote: > How about: > > "Tests that a WebAPK update is requested immediately if: > - the Shell APK is out of date > AND > - there wasn't a previous request for this ShellAPK version." Done. https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:407: final TestClock clock = new TestClock(System.currentTimeMillis()); On 2017/04/12 02:09:44, pkotwicz wrote: > - Can you please call WebappDataStorage#updateLastRequestShellApkVersion() in > the test case "set up"? It isn't necessary. The logic in shouldCheckForUpdate() can handle the case of no meta data; beside, the value in the meta data won't have impact on other tests since we always pass "false /*isShellApkVersionOutofdate*/" when calling shouldCheckForUpdate() in other tests. For clarify thing, we can call WebappDataStorage#updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION -1) after line 409. Do you think it is better? > - Don't you need to call > WebappDataStorage#updateTimeOfLastWebApkUpdateRequestCompletion(). > Otherwise the test is not very meaningful It has been called in getStorage(clock), so I don't want to duplicate. > - Can you please add the comment: > "There have not been any update requests for the current ShellAPK version. A > WebAPK update should be requested immediately." Done, thanks! https://codereview.chromium.org/2790213004/diff/180001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:412: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); On 2017/04/12 02:09:44, pkotwicz wrote: > - Can you please add a new line. > > - Can you please add the comment: > "A previous update request was made for the current ShellAPK version. A WebAPK > update should be requested after the regular delay." Done.
https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); I think that it would be clearer if the call was moved to recordUpdate(). It is hard to reason as to why this is not called in the other two places. You can make the same argument about "relax updates" only needing to be updated from onBuitWebApk()
https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); On 2017/04/12 14:33:14, pkotwicz wrote: > I think that it would be clearer if the call was moved to recordUpdate(). > It is hard to reason as to why this is not called in the other two places. > > You can make the same argument about "relax updates" only needing to be updated > from onBuitWebApk() The "relax updates" case isn't the same, since updating that value won't have impact on the update logic, while "last-request-shell-apk-version" does. For example, if shell apk is outofdate and a update failed before we get any response from the server (wasn't due to the new shell code unavailable), we will have to wait for 3 days before the next try, rather than a re-try time. If you look at the patch set#3, I update the "last-request-shell-apk-version" in the right place that it should be recorded. The reason of not adopting that solution is the complexity it added comparing with just recording the value in onBuiltWebApk().
I understand
I understand now
Hi Dom, could you please take a look? Thanks!
Really sorry about the delay on this one over Easter. Nit: incessently -> incessantly in the subject and description. https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:68: static final String KEY_LAST_REQUEST_SHELL_APK_VERSION = "last_request_shell_apk_version"; Nit: LAST_REQUESTED and last_requested https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:446: void updateLastRequestShellApkVersion(int shellApkVersion) { Nit: updateLastRequestedShellApkVersion https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:451: int getLastRequestShellApkVersion() { Nit: getLastRequestedShellApkVersion https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:480: long now = sClock.currentTimeMillis(); Nit: put this assignment back where it was originally https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:482: && WebApkVersion.CURRENT_SHELL_APK_VERSION > getLastRequestShellApkVersion()) { After some thought, I think you should move this check back up into WebApkUpdateManager. The main reason is that an out of date shell APK forces an update once we have the manifest data, as well as being a signal for whether we trigger an update. Whereas the main logic in here is to do with when the last update took place. So this logic should go into WebApkUpdateManager#shouldCheckIfWebManifestUpdated to keep the shell APK version check in the same place. That avoids spreading bits of shell APK checking in here as well (reducing complexity). What do you think / is this feasible?
Oops, missed one comment. https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); Since we only update this when an update succeeds, doesn't it mean that when the Shell APK version is out of date, we'll always try to update the WebAPK until we succeed (i.e. the shell version in WebappDataStorage is always < WebApkVersion.CURRENT_SHELL_APK_VERSION, and that's always checked before the last update interval is checked)? I read through the comments and didn't quite understand why this would work, so if you could clarify that would be great. :)
Patchset #6 (id:240001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Hi Dom, PTAL, thanks! https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); On 2017/04/18 07:24:48, dominickn wrote: > Since we only update this when an update succeeds, doesn't it mean that when the > Shell APK version is out of date, we'll always try to update the WebAPK until we > succeed (i.e. the shell version in WebappDataStorage is always < > WebApkVersion.CURRENT_SHELL_APK_VERSION, and that's always checked before the > last update interval is checked)? > > I read through the comments and didn't quite understand why this would work, so > if you could clarify that would be great. :) We call onBuildWebApk() when an update was either failed or succeeded. The thing that we want to know is: if we see the last update failed, whether we want to try the next update within re-try (12 hours) period or a regular period (3 days). We extend the interval to 3 hours only when the last update failure is caused by the fact that the shell APK isn't ready on the server. The server doesn't give us its current shell apk version in the request response (the version in the proto of the response is the app's android version), so we don't have an accurate way to know which failure falls into this category. When we see WebAPK shell apk wasn't updated, we want to distinguish the following two cases: 1) failed and shell apk isn't available on server. 2) failed but shell apk is available on server. We don't really know whether 1) happens, but we can narrow down its range by not setting the "last-requested-shell-apk-version" whenever we know the failure isn't caused by shell apk unenviable. When the value isn't set or less than the WebApkVersion.CURRENT_SHELL_APK_VERSION, we know we need to do a retry after 12 hours. The other two places which |recordUpdate| is called definitely don't mean the update failure caused by shell APK unavailable, so we should not record the "last-requested-shell-apk-version". For your question, we record "last-requested-shell-apk-version" only when it needs to, so we can avoid waiting for 3 days for another try if the failures doesn't caused by shell apk version unavailable. I agree this is a little bit confusing, but hope my explanation helps:) https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:68: static final String KEY_LAST_REQUEST_SHELL_APK_VERSION = "last_request_shell_apk_version"; On 2017/04/18 07:19:29, dominickn wrote: > Nit: LAST_REQUESTED and last_requested Done. https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:446: void updateLastRequestShellApkVersion(int shellApkVersion) { On 2017/04/18 07:19:29, dominickn wrote: > Nit: updateLastRequestedShellApkVersion Done. https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:451: int getLastRequestShellApkVersion() { On 2017/04/18 07:19:29, dominickn wrote: > Nit: getLastRequestedShellApkVersion Done. https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:480: long now = sClock.currentTimeMillis(); On 2017/04/18 07:19:29, dominickn wrote: > Nit: put this assignment back where it was originally Done.
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.
lgtm, thanks! https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2790213004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:364: storage.updateLastRequestShellApkVersion(WebApkVersion.CURRENT_SHELL_APK_VERSION); On 2017/04/18 19:19:43, Xi Han wrote: > On 2017/04/18 07:24:48, dominickn wrote: > > Since we only update this when an update succeeds, doesn't it mean that when > the > > Shell APK version is out of date, we'll always try to update the WebAPK until > we > > succeed (i.e. the shell version in WebappDataStorage is always < > > WebApkVersion.CURRENT_SHELL_APK_VERSION, and that's always checked before the > > last update interval is checked)? > > > > I read through the comments and didn't quite understand why this would work, > so > > if you could clarify that would be great. :) > > We call onBuildWebApk() when an update was either failed or succeeded. The thing > that we want to know is: if we see the last update failed, whether we want to > try the next update within re-try (12 hours) period or a regular period (3 > days). We extend the interval to 3 hours only when the last update failure is > caused by the fact that the shell APK isn't ready on the server. > > The server doesn't give us its current shell apk version in the request response > (the version in the proto of the response is the app's android version), so we > don't have an accurate way to know which failure falls into this category. > > When we see WebAPK shell apk wasn't updated, we want to distinguish the > following two cases: > 1) failed and shell apk isn't available on server. > 2) failed but shell apk is available on server. > > We don't really know whether 1) happens, but we can narrow down its range by not > setting the "last-requested-shell-apk-version" whenever we know the failure > isn't caused by shell apk unenviable. When the value isn't set or less than the > WebApkVersion.CURRENT_SHELL_APK_VERSION, we know we need to do a retry after 12 > hours. The other two places which |recordUpdate| is called definitely don't mean > the update failure caused by shell APK unavailable, so we should not record the > "last-requested-shell-apk-version". > > For your question, we record "last-requested-shell-apk-version" only when it > needs to, so we can avoid waiting for 3 days for another try if the failures > doesn't caused by shell apk version unavailable. I agree this is a little bit > confusing, but hope my explanation helps:) OK, I think I understand, thanks for the explanation. Actually, moving the logic from WebappDataStorage back to WebApkUpdateManager also helps clarify this a lot more too. :)
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2790213004/#ps260001 (title: "Move the shell apk version check back to WebApkUpdateManager.")
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": 260001, "attempt_start_ts": 1492608679829510,
"parent_rev": "18f3a62dabe898a03ad69569feb0ce37c53fc001", "commit_rev":
"6d0fd5239302fd191a795053515111df9f9fffbb"}
Message was sent while issue was closed.
Description was changed from ========== Don't try to update incessently when Shell APK version is out of date. After the shell_apk_version increases in Chrome, it takes sometime before the new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request update each time it is launched, we add a minimum delay between each request that is due to shell apk version is low. BUG=707990 ========== to ========== Don't try to update incessently when Shell APK version is out of date. After the shell_apk_version increases in Chrome, it takes sometime before the new shell APK template rolls out to the WebAPK Server. To avoid a WebAPK request update each time it is launched, we add a minimum delay between each request that is due to shell apk version is low. BUG=707990 Review-Url: https://codereview.chromium.org/2790213004 Cr-Commit-Position: refs/heads/master@{#465580} Committed: https://chromium.googlesource.com/chromium/src/+/6d0fd5239302fd191a7950535151... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001) as https://chromium.googlesource.com/chromium/src/+/6d0fd5239302fd191a7950535151... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
