Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113603003:
Exposing CacheEntryStatus (former TransactionPattern) via UrlRequest (Closed)
|
Created:
4 years, 5 months ago by jamartin Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, kinuko+cache_chromium.org, twifkak, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExposing CacheEntryStatus (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
Committed: https://crrev.com/19ff6f1bd9d1ea24e3104d0cbec6352640545d8c
Cr-Commit-Position: refs/heads/master@{#407971}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Added tests to http_cache_unittest.cc #Patch Set 3 : A bit of cleanup and more doc #
Total comments: 11
Patch Set 4 : Second batch of comments and question about auth #Patch Set 5 : gclient sync #Patch Set 6 : Test 401 before and after Read #
Total comments: 8
Patch Set 7 : Auth requests have various CacheEntryStatus #Patch Set 8 : Removed DCHECK message #
Messages
Total messages: 41 (22 generated)
jkarlin@chromium.org changed reviewers: + jkarlin@chromium.org
drive by: Thanks for writing this. This CL needs tests to verify that the pattern is set correctly since it's now being exposed to users and not just metrics. https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:422: HttpResponseInfo::CacheInfo new_transaction_pattern); newline after this line to give the comment space https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:422: HttpResponseInfo::CacheInfo new_transaction_pattern); This function needs to be renamed if it's no longer called a transaction pattern. https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:424: void SyncTransactionPattern(); Replace "Transaction" to reflect the name of the new enum. Perhaps: SyncEntryStatusToResponseInfo https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:471: HttpResponseInfo::CacheInfo transaction_pattern_; Rename transaction_pattern_ https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info.cc File net/http/http_response_info.cc (right): https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.cc:194: cache_info = CacheInfo::ENTRY_USED; It's too early to set ENTRY_USED here. HttpCacheTransaction will set this. Suggest deleting this line. https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info.h File net/http/http_response_info.h (right): https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.h:53: enum CacheInfo { CacheInfo is vague. How about CacheEntryStatus? https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.h:102: // How this response was handled by the http_cache. s/http_cache/HTTP cache/ https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.h:103: CacheInfo cache_info; similarly, prefer cache_entry_status
Description was changed from
==========
Exposing CacheInfo (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
to
==========
Exposing CacheInfo (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
jamartin@chromium.org changed reviewers: + mmenke@chromium.org
Description was changed from
==========
Exposing CacheInfo (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
to
==========
Exposing CacheEntryStatus (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
Thanks for the review. PTAL. Please pay attention to the unit tests. I've added assertions to some of the existing tests because I thought it made sense and I've added a test for each of the values of CacheEntryStatus. Please let me know if there is any additional case you want me to cover in the test and please make sure the implementation of those tests is as expected (navigating these transactions, state machines and network layers is not trivial). Thank you. https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:422: HttpResponseInfo::CacheInfo new_transaction_pattern); On 2016/06/30 19:01:12, jkarlin (slow) wrote: > newline after this line to give the comment space Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:422: HttpResponseInfo::CacheInfo new_transaction_pattern); On 2016/06/30 19:01:12, jkarlin (slow) wrote: > This function needs to be renamed if it's no longer called a transaction > pattern. Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:424: void SyncTransactionPattern(); On 2016/06/30 19:01:12, jkarlin (slow) wrote: > Replace "Transaction" to reflect the name of the new enum. > > Perhaps: SyncEntryStatusToResponseInfo Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.h:471: HttpResponseInfo::CacheInfo transaction_pattern_; On 2016/06/30 19:01:12, jkarlin (slow) wrote: > Rename transaction_pattern_ Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info.cc File net/http/http_response_info.cc (right): https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.cc:194: cache_info = CacheInfo::ENTRY_USED; On 2016/06/30 19:01:12, jkarlin (slow) wrote: > It's too early to set ENTRY_USED here. HttpCacheTransaction will set this. > Suggest deleting this line. Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info.h File net/http/http_response_info.h (right): https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.h:53: enum CacheInfo { On 2016/06/30 19:01:12, jkarlin (slow) wrote: > CacheInfo is vague. How about CacheEntryStatus? Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.h:102: // How this response was handled by the http_cache. On 2016/06/30 19:01:12, jkarlin (slow) wrote: > s/http_cache/HTTP cache/ Done. https://codereview.chromium.org/2113603003/diff/1/net/http/http_response_info... net/http/http_response_info.h:103: CacheInfo cache_info; On 2016/06/30 19:01:12, jkarlin (slow) wrote: > similarly, prefer cache_entry_status Done.
Thanks for writing the tests. Looking good. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2749: auth_response_.cache_entry_status = cache_entry_status_; To prevent an incorrect auth status, let's guard this with: if (auth_response_.headers.get()) { DCHECK_EQ(CacheEntryStatus::NOT_IN_CACHE, cache_entry_status_); auth_response_.cache_entry_status = cache_entry_status_; } https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:8096: Also add a test for an authentication request that winds up reading the authentication response info to make sure that we get the right status. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_response_... File net/http/http_response_info.h (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_response_... net/http/http_response_info.h:54: UNDEFINED, Please prefix the names with ENTRY_ e.g., ENTRY_UNDEFINED, ENTRY_OTHER, ENTRY_NOT_IN_CACHE, ENTRY_USED, ENTRY_VALIDATED, ENTRY_UPDATED, ENTRY_CANT_CONDITIONALIZE, ENTRY_MAX
jkarlin@chromium.org changed reviewers: + gavinp@chromium.org
+gavinp@ as a reviewer as he wrote the cache pattern enum in the first place.
PTAL. I need your help with the 401 test, though. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2749: auth_response_.cache_entry_status = cache_entry_status_; On 2016/07/06 17:09:41, jkarlin (slow) wrote: > To prevent an incorrect auth status, let's guard this with: > > if (auth_response_.headers.get()) { > DCHECK_EQ(CacheEntryStatus::NOT_IN_CACHE, cache_entry_status_); > auth_response_.cache_entry_status = cache_entry_status_; > } Done. Please note, though, that the status actually was ENTRY_OTHER, not ENTRY_NOT_IN_CACHE. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:8096: On 2016/07/06 17:09:41, jkarlin (slow) wrote: > Also add a test for an authentication request that winds up reading the > authentication response info to make sure that we get the right status. I need your help with this. Currently (see the new test) the cache_entry_status is being set to ENTRY_OTHER in the Read function. However, that function is being called by RunTransactionTestBase AFTER the response_info is retrieved, and thus it shows as ENTRY_UNDEFINED. What do we want here? (see the TODO for alternatives) If you want me to add or change any of the existing calls to UpdateCacheEntryStatus, please advise on its location. I don't have a sound understanding of the state machine in http_cache_transaction so I'm hesitant to alter the logic in non-isolated ways. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_response_... File net/http/http_response_info.h (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_response_... net/http/http_response_info.h:54: UNDEFINED, On 2016/07/06 17:09:41, jkarlin (slow) wrote: > Please prefix the names with ENTRY_ > > e.g., ENTRY_UNDEFINED, ENTRY_OTHER, ENTRY_NOT_IN_CACHE, ENTRY_USED, > ENTRY_VALIDATED, ENTRY_UPDATED, ENTRY_CANT_CONDITIONALIZE, ENTRY_MAX Done. Why? if I may ask. I considered the ENTRY_ prefix very much redundant given the CacheEntryStatus:: qualification.
Waiting on gainp@. jkarlin@ and mmenke@ are on holidays, so I hope they'll catch up with this soon after they are back :-) https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2749: auth_response_.cache_entry_status = cache_entry_status_; On 2016/07/07 21:36:38, jamartin wrote: > On 2016/07/06 17:09:41, jkarlin (slow) wrote: > > To prevent an incorrect auth status, let's guard this with: > > > > if (auth_response_.headers.get()) { > > DCHECK_EQ(CacheEntryStatus::NOT_IN_CACHE, cache_entry_status_); > > auth_response_.cache_entry_status = cache_entry_status_; > > } > > Done. Please note, though, that the status actually was ENTRY_OTHER, not > ENTRY_NOT_IN_CACHE. Josh has confirmed via email that he is OK with auth being ENTRY_OTHER.
On 2016/07/08 14:59:59, jamartin wrote: > Waiting on gainp@. > > jkarlin@ and mmenke@ are on holidays, so I hope they'll catch up with this soon > after they are back :-) > > https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:2749: auth_response_.cache_entry_status = > cache_entry_status_; > On 2016/07/07 21:36:38, jamartin wrote: > > On 2016/07/06 17:09:41, jkarlin (slow) wrote: > > > To prevent an incorrect auth status, let's guard this with: > > > > > > if (auth_response_.headers.get()) { > > > DCHECK_EQ(CacheEntryStatus::NOT_IN_CACHE, cache_entry_status_); > > > auth_response_.cache_entry_status = cache_entry_status_; > > > } > > > > Done. Please note, though, that the status actually was ENTRY_OTHER, not > > ENTRY_NOT_IN_CACHE. > > Josh has confirmed via email that he is OK with auth being ENTRY_OTHER. Please ignore the git cl try errors for now. They are caused by the DO NOT SUBMIT statements I have in place due to my questions to the reviewers.
Going to defer to Josh / Gavin on this one - I really don't know HttpCache::Transaction, and have a lot of other reviews in my queue.
jamartin@chromium.org changed reviewers: - mmenke@chromium.org
OK, then we'll wait for Gavin. Thanks for checking in Matt.
@gavinp: Friendly ping :-)
https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:8096: On 2016/07/07 21:36:38, jamartin wrote: > On 2016/07/06 17:09:41, jkarlin (slow) wrote: > > Also add a test for an authentication request that winds up reading the > > authentication response info to make sure that we get the right status. > > I need your help with this. Currently (see the new test) the cache_entry_status > is being set to ENTRY_OTHER in the Read function. However, that function is > being called by RunTransactionTestBase AFTER the response_info is retrieved, and > thus it shows as ENTRY_UNDEFINED. That's the correct behavior for each case. > > What do we want here? (see the TODO for alternatives) Let's test both cases. In one case use the RunTransactionTestBase and in the other you'll have to modify that code a bit to call Read first.
The CQ bit was checked by jamartin@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...
PTAL. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:8096: On 2016/07/18 17:24:46, jkarlin wrote: > On 2016/07/07 21:36:38, jamartin wrote: > > On 2016/07/06 17:09:41, jkarlin (slow) wrote: > > > Also add a test for an authentication request that winds up reading the > > > authentication response info to make sure that we get the right status. > > > > I need your help with this. Currently (see the new test) the > cache_entry_status > > is being set to ENTRY_OTHER in the Read function. However, that function is > > being called by RunTransactionTestBase AFTER the response_info is retrieved, > and > > thus it shows as ENTRY_UNDEFINED. > > That's the correct behavior for each case. > > > > > What do we want here? (see the TODO for alternatives) > > Let's test both cases. In one case use the RunTransactionTestBase and in the > other you'll have to modify that code a bit to call Read first. Done.
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 with changes but I'm not an owner. gavinp@ PTAL. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:8096: On 2016/07/19 16:02:46, jamartin wrote: > On 2016/07/18 17:24:46, jkarlin wrote: > > On 2016/07/07 21:36:38, jamartin wrote: > > > On 2016/07/06 17:09:41, jkarlin (slow) wrote: > > > > Also add a test for an authentication request that winds up reading the > > > > authentication response info to make sure that we get the right status. > > > > > > I need your help with this. Currently (see the new test) the > > cache_entry_status > > > is being set to ENTRY_OTHER in the Read function. However, that function is > > > being called by RunTransactionTestBase AFTER the response_info is retrieved, > > and > > > thus it shows as ENTRY_UNDEFINED. > > > > That's the correct behavior for each case. > > > > > > > > What do we want here? (see the TODO for alternatives) > > > > Let's test both cases. In one case use the RunTransactionTestBase and in the > > other you'll have to modify that code a bit to call Read first. > > Done. I was wrong, my apologies, the auth request can come up for various reasons and not just OTHER. Given that, please remove the DCHECK and these two unittests. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2765: DCHECK_EQ(CacheEntryStatus::ENTRY_OTHER, cache_entry_status_); I was wrong about putting this dcheck in. We can wind up with an auth response for lots of reasons, not just OTHER. Please remove. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.h:419: void set_response(const HttpResponseInfo& new_response); SetResponse as it does a bit more than just set the variable. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.h:420: void set_auth_response(const HttpResponseInfo& new_response); SetAuthResponse
The CQ bit was checked by jamartin@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...
@gavinp: PTAL. https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2113603003/diff/40001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:8096: On 2016/07/21 17:25:54, jkarlin wrote: > On 2016/07/19 16:02:46, jamartin wrote: > > On 2016/07/18 17:24:46, jkarlin wrote: > > > On 2016/07/07 21:36:38, jamartin wrote: > > > > On 2016/07/06 17:09:41, jkarlin (slow) wrote: > > > > > Also add a test for an authentication request that winds up reading the > > > > > authentication response info to make sure that we get the right status. > > > > > > > > I need your help with this. Currently (see the new test) the > > > cache_entry_status > > > > is being set to ENTRY_OTHER in the Read function. However, that function > is > > > > being called by RunTransactionTestBase AFTER the response_info is > retrieved, > > > and > > > > thus it shows as ENTRY_UNDEFINED. > > > > > > That's the correct behavior for each case. > > > > > > > > > > > What do we want here? (see the TODO for alternatives) > > > > > > Let's test both cases. In one case use the RunTransactionTestBase and in the > > > other you'll have to modify that code a bit to call Read first. > > > > Done. > > I was wrong, my apologies, the auth request can come up for various reasons and > not just OTHER. Given that, please remove the DCHECK and these two unittests. No worries. I was getting concerned by the failures in "git cl try", so this explains it. Done. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2765: DCHECK_EQ(CacheEntryStatus::ENTRY_OTHER, cache_entry_status_); On 2016/07/21 17:25:55, jkarlin wrote: > I was wrong about putting this dcheck in. We can wind up with an auth response > for lots of reasons, not just OTHER. Please remove. No worries. Done. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.h:419: void set_response(const HttpResponseInfo& new_response); On 2016/07/21 17:25:55, jkarlin wrote: > SetResponse as it does a bit more than just set the variable. Done. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.h:420: void set_auth_response(const HttpResponseInfo& new_response); On 2016/07/21 17:25:55, jkarlin wrote: > SetAuthResponse Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2751: << "Cannot call UpdateCacheEntryStatus with ENTRY_UNDEFINED."; I think this message isn't needed; it doesn't expand on the DCHECK itself.
Thanks for the review. I'll submit once the latest run for git cl try finishes. https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2113603003/diff/100001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:2751: << "Cannot call UpdateCacheEntryStatus with ENTRY_UNDEFINED."; On 2016/07/26 17:54:19, gavinp wrote: > I think this message isn't needed; it doesn't expand on the DCHECK itself. Done.
The CQ bit was checked by jamartin@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.
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org, gavinp@chromium.org Link to the patchset: https://codereview.chromium.org/2113603003/#ps140001 (title: "Removed DCHECK message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Exposing CacheEntryStatus (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
to
==========
Exposing CacheEntryStatus (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Exposing CacheEntryStatus (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
==========
to
==========
Exposing CacheEntryStatus (former TransactionPattern) via UrlRequest
This will allow to know how the UrlRequest affected the cache.
Currently, it was possible to retrieve:
cache used = was_cached && !network_accessed
cache validated = was_cached && network_accessed
not used/updated/cantconditionalize = !was_cached
This change allows to tell apart the not used/updated/cantconditionalize
sub-categories.
This change has the added benefit that HttpCacheTransaction::RecordHistograms
could be refactorized out of that class and moved to upper layers.
BUG=615434
Committed: https://crrev.com/19ff6f1bd9d1ea24e3104d0cbec6352640545d8c
Cr-Commit-Position: refs/heads/master@{#407971}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/19ff6f1bd9d1ea24e3104d0cbec6352640545d8c Cr-Commit-Position: refs/heads/master@{#407971} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
