Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872933003:
Add base persistence interfaces for Prefetching Offline Pages. (Closed)
|
Created:
3 years, 7 months ago by carlosk Modified:
3 years, 7 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd base persistence interfaces for Prefetching Offline Pages.
BUG=701939
Review-Url: https://codereview.chromium.org/2872933003
Cr-Commit-Position: refs/heads/master@{#472166}
Committed: https://chromium.googlesource.com/chromium/src/+/e01ab251f17d5460f8f0865ef681f18ba1042c8b
Patch Set 1 #
Total comments: 27
Patch Set 2 : Addressed comments and other changes; added constants file. #
Total comments: 10
Patch Set 3 : Renamed constants file; removed ClientId dependency; and more. #
Total comments: 2
Patch Set 4 : Added unittest and operator!=; added string client_id; renamed members. #Patch Set 5 : Actually adding the unit test now. #Patch Set 6 : Moved constructors and destructor to implementation file. #
Total comments: 8
Patch Set 7 : Comment updates. #Patch Set 8 : Rebase. #Patch Set 9 : Rename NO_ERROR to SUCCESS because of Windows. #
Messages
Total messages: 55 (30 generated)
dimich@chromium.org changed reviewers: + dimich@chromium.org
https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:53: // to confirm that the same URL is not being reported anymore by its client. "is not being reported anymore" -> "is not being requested again"? https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:64: int64_t id = INVALID_ID; Should we do a guid here? We need a guid for Download API anyways. Or is there a need to 'inherit' the ID into the OfflinePageItem's id as we do for OfflienPageRequests? If we do guid here, we don't need download_id below. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:78: // |prefetch_url|. It will be empty if they are the same. typo: there is no prefetch_url https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:85: // the archive immediately available to download. Add: "Normally corresponds to the operation_name reported by GCM message when the archive is ready for download" ? https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:96: // Number of times an attempt was made to download the archive containing this Don't we just try it once? The Download API should take care about restartability.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:28: enum class State { Is there a good reason to keep the names of enums so short? Would there be much work to state things more explicitly?
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:19: // Instances of this class are mainly used for communicating with the Maybe this would be clearer as: Instances of this class are in-memory representations of items in (or to be insterted into) the persistent prefetching data store. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:28: enum class State { On 2017/05/09 22:31:31, fgorski wrote: > Is there a good reason to keep the names of enums so short? > Would there be much work to state things more explicitly? +1 This matches the DD but in code perhaps we should just use real full names. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:72: State state = NEW_REQ; It might be better to insist that state be required in the constructor. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:74: // The URL pointing to the page the client requested to be prefetched. URL of the page? https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:82: int generate_archive_attempt_count = 0; I think I prefer request_archive_attempt_count since Chrome isn't actually generating. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:93: // snapshot. The archive might also include other articles in a bundle. document what -1 and other negatives represent
Thanks. dimich@, fgorski@, dewittj@: PTAL. I created a new header to hold prefetcher constants, added the simplest item implementation and added new files to the build config. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:19: // Instances of this class are mainly used for communicating with the On 2017/05/09 22:41:24, dewittj wrote: > Maybe this would be clearer as: > Instances of this class are in-memory representations of items in (or to be > insterted into) the persistent prefetching data store. Done. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:28: enum class State { On 2017/05/09 22:41:24, dewittj wrote: > On 2017/05/09 22:31:31, fgorski wrote: > > Is there a good reason to keep the names of enums so short? > > Would there be much work to state things more explicitly? > > +1 This matches the DD but in code perhaps we should just use real full names. enums were moved to prefetch_constants.h and now have longer names. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:53: // to confirm that the same URL is not being reported anymore by its client. On 2017/05/09 20:42:25, Dmitry Titov wrote: > "is not being reported anymore" -> "is not being requested again"? Rephrased (but this text moved to prefetch_constants.h). https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:64: int64_t id = INVALID_ID; On 2017/05/09 20:42:25, Dmitry Titov wrote: > Should we do a guid here? We need a guid for Download API anyways. Or is there a > need to 'inherit' the ID into the OfflinePageItem's id as we do for > OfflienPageRequests? > > If we do guid here, we don't need download_id below. After reading the downloads design doc I was uncertain if we would be able to provide our own GUID with download requests. Any news on that? And I don't see a need to re-use this for the OfflinePageItem. I'm OK with whatever type as long as it's easy to create unique values for it. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:72: State state = NEW_REQ; On 2017/05/09 22:41:25, dewittj wrote: > It might be better to insist that state be required in the constructor. When would we create a new instance not in the NEW_REQ state? I'm considering the loading from the store special enough to deserve its own constructor. I'd prefer to add more constructors by the measure we have actual need for them. WDYT? https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:74: // The URL pointing to the page the client requested to be prefetched. On 2017/05/09 22:41:25, dewittj wrote: > URL of the page? Done. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:78: // |prefetch_url|. It will be empty if they are the same. On 2017/05/09 20:42:26, Dmitry Titov wrote: > typo: there is no prefetch_url Done. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:82: int generate_archive_attempt_count = 0; On 2017/05/09 22:41:24, dewittj wrote: > I think I prefer request_archive_attempt_count since Chrome isn't actually > generating. Done. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:85: // the archive immediately available to download. On 2017/05/09 20:42:26, Dmitry Titov wrote: > Add: "Normally corresponds to the operation_name reported by GCM message when > the archive is ready for download" ? It's also used when archiving failed. I rephrased it a little differently but kept the references to where it is used. WDYT? https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:93: // snapshot. The archive might also include other articles in a bundle. On 2017/05/09 22:41:24, dewittj wrote: > document what -1 and other negatives represent Done for this case and similarly the body name above. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:96: // Number of times an attempt was made to download the archive containing this On 2017/05/09 20:42:26, Dmitry Titov wrote: > Don't we just try it once? The Download API should take care about > restartability. I removed it. I'll remove this column from the design doc once this CL lands (along with other updates from what we agree here). Just confirming the implications I see in this: - We won't retry requesting downloads to the Downloads system. - The Downloads system will take care of limiting download retry attempts.
I think we should just go to guid, see the comment for additional info. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:64: int64_t id = INVALID_ID; On 2017/05/09 23:57:40, carlosk wrote: > On 2017/05/09 20:42:25, Dmitry Titov wrote: > > Should we do a guid here? We need a guid for Download API anyways. Or is there > a > > need to 'inherit' the ID into the OfflinePageItem's id as we do for > > OfflienPageRequests? > > > > If we do guid here, we don't need download_id below. > > After reading the downloads design doc I was uncertain if we would be able to > provide our own GUID with download requests. Any news on that? > > And I don't see a need to re-use this for the OfflinePageItem. I'm OK with > whatever type as long as it's easy to create unique values for it. Not sure what you mean, we have to pass guid in when we ask to start download - see DownloadParams struct. This is by design, since because if the browser dies before calling async callback it'd be impossible to tell if the item was taken for download. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:96: // Number of times an attempt was made to download the archive containing this On 2017/05/09 23:57:39, carlosk wrote: > On 2017/05/09 20:42:26, Dmitry Titov wrote: > > Don't we just try it once? The Download API should take care about > > restartability. > > I removed it. I'll remove this column from the design doc once this CL lands > (along with other updates from what we agree here). > > Just confirming the implications I see in this: > - We won't retry requesting downloads to the Downloads system. > - The Downloads system will take care of limiting download retry attempts. I think this is reasonable assumptions about Download API.
https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_constants.h (right): https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_constants.h:13: enum class PrefetchItemState { I don't thing my original problem was addressed. Here is what I meant: NEW_REQUEST, SENT_GENERATE_PAGE, AWAITING_GCM, RECEIVED_GCM, SENT_GET_OPERATION, RECEIVED_BUNDLE, DOWNLOADING, FINISHED, ZOMBIE // or SENTINEL https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_constants.h:34: // Item has finished processing, successfully or otherwise and is waiting to nit: put a , after otherwise https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.h:23: public: nit: struct implies public. https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.h:24: const int64_t INVALID_ID = -1; will you only allow positive numbers? If negative are OK, wouldn't 0 be a better invalid_id? (Also tests nicely as boolean) https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.h:88: // The reason why a the item was set to the FIN state. Until then its value is nit: "why a the"?
Thanks. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:64: int64_t id = INVALID_ID; On 2017/05/10 00:44:47, Dmitry Titov wrote: > On 2017/05/09 23:57:40, carlosk wrote: > > On 2017/05/09 20:42:25, Dmitry Titov wrote: > > > Should we do a guid here? We need a guid for Download API anyways. Or is > there > > a > > > need to 'inherit' the ID into the OfflinePageItem's id as we do for > > > OfflienPageRequests? > > > > > > If we do guid here, we don't need download_id below. > > > > After reading the downloads design doc I was uncertain if we would be able to > > provide our own GUID with download requests. Any news on that? > > > > And I don't see a need to re-use this for the OfflinePageItem. I'm OK with > > whatever type as long as it's easy to create unique values for it. > > Not sure what you mean, we have to pass guid in when we ask to start download - > see DownloadParams struct. This is by design, since because if the browser dies > before calling async callback it'd be impossible to tell if the item was taken > for download. Indeed and SGTM. Changed to GUID. https://codereview.chromium.org/2872933003/diff/1/components/offline_pages/co... components/offline_pages/core/prefetch/prefetch_item.h:96: // Number of times an attempt was made to download the archive containing this On 2017/05/10 00:44:47, Dmitry Titov wrote: > On 2017/05/09 23:57:39, carlosk wrote: > > On 2017/05/09 20:42:26, Dmitry Titov wrote: > > > Don't we just try it once? The Download API should take care about > > > restartability. > > > > I removed it. I'll remove this column from the design doc once this CL lands > > (along with other updates from what we agree here). > > > > Just confirming the implications I see in this: > > - We won't retry requesting downloads to the Downloads system. > > - The Downloads system will take care of limiting download retry attempts. > > I think this is reasonable assumptions about Download API. Extra note: I found this enum value in the DownloadsParam::StartResult: // The DownloadService has too many downloads. Backoff and retry. BACKOFF, This would be a non-retryable error for prefetching. https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_constants.h (right): https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_constants.h:13: enum class PrefetchItemState { On 2017/05/10 05:02:53, fgorski wrote: > I don't thing my original problem was addressed. Here is what I meant: > NEW_REQUEST, > SENT_GENERATE_PAGE, > AWAITING_GCM, > RECEIVED_GCM, > SENT_GET_OPERATION, > RECEIVED_BUNDLE, > DOWNLOADING, > FINISHED, > ZOMBIE // or SENTINEL Done. I didn't understand your initial concern. Previous names matched the design doc ones but they are way more legible like this. https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_constants.h:34: // Item has finished processing, successfully or otherwise and is waiting to On 2017/05/10 05:02:53, fgorski wrote: > nit: put a , after otherwise Done. https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.h:23: public: On 2017/05/10 05:02:53, fgorski wrote: > nit: struct implies public. Done. https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.h:24: const int64_t INVALID_ID = -1; On 2017/05/10 05:02:53, fgorski wrote: > will you only allow positive numbers? > If negative are OK, wouldn't 0 be a better invalid_id? (Also tests nicely as > boolean) As we moved to a string guid this was now removed. https://codereview.chromium.org/2872933003/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.h:88: // The reason why a the item was set to the FIN state. Until then its value is On 2017/05/10 05:02:53, fgorski wrote: > nit: "why a the"? Done.
please add prefetch_types.h to this CL
https://codereview.chromium.org/2872933003/diff/40001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_item.cc (right): https://codereview.chromium.org/2872933003/diff/40001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.cc:9: bool PrefetchItem::operator==(const PrefetchItem& other) const { If you have implementation, do you need tests?
On 2017/05/11 17:04:27, dewittj wrote: > please add prefetch_types.h to this CL Done! Thanks for the reminder. I also re-added the client specified ID as it will be needed for Zine. https://codereview.chromium.org/2872933003/diff/40001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_item.cc (right): https://codereview.chromium.org/2872933003/diff/40001/components/offline_page... components/offline_pages/core/prefetch/prefetch_item.cc:9: bool PrefetchItem::operator==(const PrefetchItem& other) const { On 2017/05/11 17:07:12, dewittj wrote: > If you have implementation, do you need tests? Done.
lgtm
The CQ bit was checked by carlosk@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by carlosk@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm with comments. (about comments and formatting) https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_item.cc (right): https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_item.cc:13: PrefetchItem::~PrefetchItem(){}; was git cl format OK with no space in (){}? https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_item.h:17: // from the moment a URL is requested by a client until its processing id done, s/id/is/ https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_item.h:85: // The reason why the item was set to the FIN state. Should be disregarded FINISHED https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_types.h (right): https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_types.h:11: // prefetching process. They follow somewhat the order below but some states add a comma before "but"
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, dewittj@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2872933003/#ps140001 (title: "Comment updates.")
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 carlosk@chromium.org
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2872933003/#ps160001 (title: "Comment updates.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_...)
Thanks! https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_item.cc (right): https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_item.cc:13: PrefetchItem::~PrefetchItem(){}; On 2017/05/12 22:54:06, fgorski wrote: > was git cl format OK with no space in (){}? That's what `git cl format` spits back... :/ https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_item.h (right): https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_item.h:17: // from the moment a URL is requested by a client until its processing id done, On 2017/05/12 22:54:06, fgorski wrote: > s/id/is/ Done. https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_item.h:85: // The reason why the item was set to the FIN state. Should be disregarded On 2017/05/12 22:54:06, fgorski wrote: > FINISHED Done. https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... File components/offline_pages/core/prefetch/prefetch_types.h (right): https://codereview.chromium.org/2872933003/diff/100001/components/offline_pag... components/offline_pages/core/prefetch/prefetch_types.h:11: // prefetching process. They follow somewhat the order below but some states On 2017/05/12 22:54:06, fgorski wrote: > add a comma before "but" Done.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2872933003/#ps180001 (title: "Comment updates.")
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by carlosk@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2872933003/#ps200001 (title: "Rename NO_ERROR to SUCCESS because of Windows.")
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
Try jobs failed on following builders: 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 carlosk@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": 200001, "attempt_start_ts": 1494955287613230,
"parent_rev": "b6e599fe4496e4f1aefdff8e73d1697f2d9da6b0", "commit_rev":
"e01ab251f17d5460f8f0865ef681f18ba1042c8b"}
Message was sent while issue was closed.
Description was changed from ========== Add base persistence interfaces for Prefetching Offline Pages. BUG=701939 ========== to ========== Add base persistence interfaces for Prefetching Offline Pages. BUG=701939 Review-Url: https://codereview.chromium.org/2872933003 Cr-Commit-Position: refs/heads/master@{#472166} Committed: https://chromium.googlesource.com/chromium/src/+/e01ab251f17d5460f8f0865ef681... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e01ab251f17d5460f8f0865ef681... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
