Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650753003:
[ReadingList iOS] Cleanup Offline directory on startup (Closed)
Description[ReadingList iOS] Cleanup Offline directory on startup
At startup, list the ReadingList entries and the directories in Offline
directory.
- Delete all directories that do not correspond to PROCESSED Reading List
entries
- Schedule all pending Reading List entries for processing.
BUG=684446
Review-Url: https://codereview.chromium.org/2650753003
Cr-Commit-Position: refs/heads/master@{#446036}
Committed: https://chromium.googlesource.com/chromium/src/+/d927d42c252a5d2b72e73d7b514e4d6a9e0a05e1
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 28
Patch Set 3 : feedback #
Total comments: 2
Patch Set 4 : comment #
Messages
Total messages: 14 (5 generated)
olivierrobin@chromium.org changed reviewers: + gambard@chromium.org, jif@chromium.org
https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.cc (right): https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.cc:87: DownloadAllEntries(); Don't you need to update this? https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.h (right): https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:30: // Any calls made to DownloadAllEntries/DownloadEntry before the model is Update this comment. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:31: // loaded will be ignored. When the model is loaded, DownloadAllEntries will be Same https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:70: // Initiates a cleanup of |OfflineRoot| directory removing sub_directories s/OfflineRoot/OfflineRoot()/ https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:74: // Scans |OfflineRoot| directory and deletes all subdirectories not listed in s/OfflineRoot/OfflineRoot()/ https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:75: // processed_directories. s/processed_directories/|processed_directories|/ https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:77: void CleanUpFiles(const std::set<std::string>& processed_directories); Based on the name of the function I would expect it to clear the data listed in |processed_directories|. Maybe CleanUpExcept()? Or change processed_directories to something like directories_to_keep, exception_directories? https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:78: // Schedules all emtries in |unprocessed_entries| for distillation. s/emtries/entries/ This sentence looks weird to me but I have trouble finding a better one. Maybe it is because we are not scheduling an entry for distillation but scheduling a download? https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:80: // Processes a new entry and schedule a download if needed. s/schedule/schedules
https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.cc (right): https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.cc:128: if (entry->DistilledState() == ReadingListEntry::PROCESSED) { You know the drill. Can you do a: switch (entry->DistilledState()) { with all the states explicitly handled (i.e. no "default")? https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.cc:130: processed_directories.insert(directory_name); can you do: processed_directories.insert(reading_list::OfflineURLDirectoryID(url)); or: { std::string directory_name = reading_list::OfflineURLDirectoryID(url); processed_directories.insert(std::move(directory_name)); } https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.h (right): https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:68: // Checks the model and determine what entries are processed and what entries s/what entries/which entries/ https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:68: // Checks the model and determine what entries are processed and what entries s/determine/determines/ https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:69: // needs to be processed. need
Thanks PTAL. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.cc (right): https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.cc:87: DownloadAllEntries(); On 2017/01/25 09:02:07, gambard wrote: > Don't you need to update this? mmm, yes, I think it would work better if this is updated. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.cc:128: if (entry->DistilledState() == ReadingListEntry::PROCESSED) { On 2017/01/25 09:15:51, jif wrote: > You know the drill. Can you do a: > > switch (entry->DistilledState()) { > > with all the states explicitly handled (i.e. no "default")? Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.cc:130: processed_directories.insert(directory_name); On 2017/01/25 09:15:51, jif wrote: > can you do: > > processed_directories.insert(reading_list::OfflineURLDirectoryID(url)); > > or: > > { > std::string directory_name = reading_list::OfflineURLDirectoryID(url); > processed_directories.insert(std::move(directory_name)); > } Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.h (right): https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:30: // Any calls made to DownloadAllEntries/DownloadEntry before the model is On 2017/01/25 09:02:07, gambard wrote: > Update this comment. SyncWithModel is private. Removed from comment. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:31: // loaded will be ignored. When the model is loaded, DownloadAllEntries will be On 2017/01/25 09:02:07, gambard wrote: > Same Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:68: // Checks the model and determine what entries are processed and what entries On 2017/01/25 09:15:51, jif wrote: > s/what entries/which entries/ Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:68: // Checks the model and determine what entries are processed and what entries On 2017/01/25 09:15:51, jif wrote: > s/determine/determines/ Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:69: // needs to be processed. On 2017/01/25 09:15:51, jif wrote: > need Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:70: // Initiates a cleanup of |OfflineRoot| directory removing sub_directories On 2017/01/25 09:02:07, gambard wrote: > s/OfflineRoot/OfflineRoot()/ Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:74: // Scans |OfflineRoot| directory and deletes all subdirectories not listed in On 2017/01/25 09:02:07, gambard wrote: > s/OfflineRoot/OfflineRoot()/ Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:75: // processed_directories. On 2017/01/25 09:02:07, gambard wrote: > s/processed_directories/|processed_directories|/ Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:77: void CleanUpFiles(const std::set<std::string>& processed_directories); On 2017/01/25 09:02:07, gambard wrote: > Based on the name of the function I would expect it to clear the data listed in > |processed_directories|. > Maybe CleanUpExcept()? Or change processed_directories to something like > directories_to_keep, exception_directories? Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:78: // Schedules all emtries in |unprocessed_entries| for distillation. On 2017/01/25 09:02:07, gambard wrote: > s/emtries/entries/ > This sentence looks weird to me but I have trouble finding a better one. Maybe > it is because we are not scheduling an entry for distillation but scheduling a > download? Done. https://codereview.chromium.org/2650753003/diff/20001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:80: // Processes a new entry and schedule a download if needed. On 2017/01/25 09:02:07, gambard wrote: > s/schedule/schedules Done.
lgtm https://codereview.chromium.org/2650753003/diff/40001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.h (right): https://codereview.chromium.org/2650753003/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:71: // Schedule unprocessed entries for distillation. Schedules
lgtm
https://codereview.chromium.org/2650753003/diff/40001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_download_service.h (right): https://codereview.chromium.org/2650753003/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_download_service.h:71: // Schedule unprocessed entries for distillation. On 2017/01/25 15:52:28, jif wrote: > Schedules Done.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org, jif@chromium.org Link to the patchset: https://codereview.chromium.org/2650753003/#ps60001 (title: "comment")
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": 60001, "attempt_start_ts": 1485361974049220,
"parent_rev": "7d6ec5cb69733e7bae2b5e70911df655de1e76a8", "commit_rev":
"d927d42c252a5d2b72e73d7b514e4d6a9e0a05e1"}
Message was sent while issue was closed.
Description was changed from ========== [ReadingList iOS] Cleanup Offline directory on startup At startup, list the ReadingList entries and the directories in Offline directory. - Delete all directories that do not correspond to PROCESSED Reading List entries - Schedule all pending Reading List entries for processing. BUG=684446 ========== to ========== [ReadingList iOS] Cleanup Offline directory on startup At startup, list the ReadingList entries and the directories in Offline directory. - Delete all directories that do not correspond to PROCESSED Reading List entries - Schedule all pending Reading List entries for processing. BUG=684446 Review-Url: https://codereview.chromium.org/2650753003 Cr-Commit-Position: refs/heads/master@{#446036} Committed: https://chromium.googlesource.com/chromium/src/+/d927d42c252a5d2b72e73d7b514e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d927d42c252a5d2b72e73d7b514e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
