Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2373713003:
Retry WebAPK download if fails for the first time. (Closed)
DescriptionRetry WebAPK download if fails for the first time.
The WebAPK server may send us the URL of the WebAPK to download prior to the
WebAPK being available at that URL. WebApkInstaller will sleep 2 seconds and
retry the download if the download fails.
BUG=649704
Patch Set 1 #
Total comments: 10
Patch Set 2 : pkotwicz@'s comments. #Patch Set 3 : Rebase #
Messages
Total messages: 24 (17 generated)
Description was changed from ========== Retry WebAPK download if we get a 404. BUG=649704 ========== to ========== Retry WebAPK download if we get a 404. BUG=649704 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Retry WebAPK download if we get a 404. BUG=649704 ========== to ========== Retry WebAPK download if fails for the first time. The WebAPK server may send us the URL of the WebAPK to download prior to the WebAPK being available at that URL. We should retry the download if the download fails. BUG=649704 ==========
Description was changed from ========== Retry WebAPK download if fails for the first time. The WebAPK server may send us the URL of the WebAPK to download prior to the WebAPK being available at that URL. We should retry the download if the download fails. BUG=649704 ========== to ========== Retry WebAPK download if fails for the first time. The WebAPK server may send us the URL of the WebAPK to download prior to the WebAPK being available at that URL. WebApkInstaller will sleep 2 seconds and retry the download if the download fails. BUG=649704 ==========
Description was changed from ========== Retry WebAPK download if fails for the first time. The WebAPK server may send us the URL of the WebAPK to download prior to the WebAPK being available at that URL. WebApkInstaller will sleep 2 seconds and retry the download if the download fails. BUG=649704 ========== to ========== Retry WebAPK download if fails for the first time. The WebAPK server may send us the URL of the WebAPK to download prior to the WebAPK being available at that URL. WebApkInstaller will sleep 2 seconds and retry the download if the download fails. BUG=649704 ==========
Patchset #1 (id:60001) has been deleted
Hi Peter, could you please take a look? Thanks!
LGTM with nits https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:363: void WebApkInstaller::StartDownloadingWebApk(const base::FilePath& output_path, Nit: Rename this function to DownloadWebApk() I think that this function name is more consistent with OnWebApkDownloaded() ... and your comment in the .h file https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:381: timer_.Stop(); Nit: Maybe reset |downloader_| for clarity https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:389: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(2)); Use BrowserThread::PostDelayedTask() instead https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:152: // Downloads the WebAPK from the given |donwload_url|. Nit: |donwload_url| -> |download_url|. https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:160: // If |retry_if_fails| is true, will sleep 2 seconds and retry the download. You should post a delayed task instead of sleeping. A friendly reminder to update the documentation here
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, I need OWNERS review for this CL, please take a look. Thanks! https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:363: void WebApkInstaller::StartDownloadingWebApk(const base::FilePath& output_path, On 2016/09/27 19:47:28, pkotwicz wrote: > Nit: Rename this function to DownloadWebApk() > > I think that this function name is more consistent with OnWebApkDownloaded() ... > and your comment in the .h file Done. https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:381: timer_.Stop(); On 2016/09/27 19:47:28, pkotwicz wrote: > Nit: Maybe reset |downloader_| for clarity Done. https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:389: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(2)); On 2016/09/27 19:47:28, pkotwicz wrote: > Use BrowserThread::PostDelayedTask() instead Done. https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:152: // Downloads the WebAPK from the given |donwload_url|. On 2016/09/27 19:47:28, pkotwicz wrote: > Nit: |donwload_url| -> |download_url|. Done. https://codereview.chromium.org/2373713003/diff/80001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.h:160: // If |retry_if_fails| is true, will sleep 2 seconds and retry the download. On 2016/09/27 19:47:28, pkotwicz wrote: > You should post a delayed task instead of sleeping. A friendly reminder to > update the documentation here Done.
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by dfalcantara@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2373713003/#ps120001 (title: "pkotwicz@'s comments.")
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
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, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2373713003/#ps140001 (title: "Rebase")
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
Exceeded global retry quota |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
