Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1305883002:
Show "storage almost full" message when a page is saved offline (Closed)
DescriptionShow "storage almost full" message when a page is saved offline
BUG=491352
Committed: https://crrev.com/f0f354c6874641783563bceaa59a15e261f25b95
Cr-Commit-Position: refs/heads/master@{#344885}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address feedback #
Total comments: 4
Patch Set 3 : Update string #Patch Set 4 : Fix trybots #
Messages
Total messages: 25 (10 generated)
jianli@chromium.org changed reviewers: + fgorski@chromium.org, kkimlabs@chromium.org
lgtm
lgtm https://codereview.chromium.org/1305883002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java (right): https://codereview.chromium.org/1305883002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:28: private static final long STORAGE_ALMOST_FULL_THRESHOLD_BYTES = 10485760L; // 10M 1L << 20 ? https://codereview.chromium.org/1305883002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:105: return Environment.getExternalStorageDirectory().getUsableSpace() do we know that in scenario with multiple SD cards/drives on the phone we are checking the one where the downloads folder is? Add a TODO and item to the tracker.
https://codereview.chromium.org/1305883002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java (right): https://codereview.chromium.org/1305883002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:28: private static final long STORAGE_ALMOST_FULL_THRESHOLD_BYTES = 10485760L; // 10M On 2015/08/20 22:57:22, fgorski wrote: > 1L << 20 ? Done. https://codereview.chromium.org/1305883002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:105: return Environment.getExternalStorageDirectory().getUsableSpace() On 2015/08/20 22:57:22, fgorski wrote: > do we know that in scenario with multiple SD cards/drives on the phone we are > checking the one where the downloads folder is? > > Add a TODO and item to the tracker. First I think only a very few devices that are mounted with more than one SD card. getExternalStorageDirectory should return the default storage directory which should also be where the download folder lives. So I don't think we want to tackle the unnecessary complicated thing.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkimlabs@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1305883002/#ps20001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305883002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305883002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jianli@chromium.org changed reviewers: + newt@chromium.org
newt for chrome/android changes
https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java (right): https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:105: return Environment.getExternalStorageDirectory().getUsableSpace() Is the "external storage directory" the same place where we store offline pages? I would have expected they'd be stored in /data/data/com.android.chrome/ along with our other app data. https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1802: Page saved offline, storage almost full The grammar here seems suspect ( https://en.wikipedia.org/wiki/Comma_splice ). Was this string blessed by a PM or designer?
https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java (right): https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:105: return Environment.getExternalStorageDirectory().getUsableSpace() On 2015/08/21 01:13:37, newt wrote: > Is the "external storage directory" the same place where we store offline pages? > I would have expected they'd be stored in /data/data/com.android.chrome/ along > with our other app data. We actually store it in the downloads folder, so that the archive files are available to other applications.
lgtm after the string is updated to "Page saved offline. Storage almost full." https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java (right): https://codereview.chromium.org/1305883002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageBridge.java:105: return Environment.getExternalStorageDirectory().getUsableSpace() On 2015/08/21 17:55:25, fgorski wrote: > On 2015/08/21 01:13:37, newt wrote: > > Is the "external storage directory" the same place where we store offline > pages? > > I would have expected they'd be stored in /data/data/com.android.chrome/ along > > with our other app data. > > We actually store it in the downloads folder, so that the archive files are > available to other applications. Gotcha. In that case, this looks good to me.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, kkimlabs@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1305883002/#ps40001 (title: "Update string")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305883002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, kkimlabs@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1305883002/#ps60001 (title: "Fix trybots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305883002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f0f354c6874641783563bceaa59a15e261f25b95 Cr-Commit-Position: refs/heads/master@{#344885} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
