Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12310075:
Cache failover to LOAD_PREFERRING_CACHE if network response suggests offline. (Closed)
|
Created:
7 years, 10 months ago by Randy Smith (Not in Mondays) Modified:
7 years, 9 months ago Reviewers:
rvargas (doing something else) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, gavinp+disk_chromium.org, gavinp, droger Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCache failover to LOAD_PREFERRING_CACHE if network response suggests offline.
(Controlled by new load flag.)
BUG=2204
R=rvargas@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190020
Patch Set 1 #Patch Set 2 : Checkpoint. #Patch Set 3 : Removed debug code from diff. #Patch Set 4 : Test written and runs. #Patch Set 5 : Removed vestigial reference to new content flag. #
Total comments: 50
Patch Set 6 : Incorporated most comments; responded with questions to a few. #
Total comments: 14
Patch Set 7 : Incorporated comments. #Patch Set 8 : Moved new flag to nearer other flags in same file. #
Total comments: 2
Patch Set 9 : Removed blank line. #Patch Set 10 : Merged to revision r187927. #Patch Set 11 : Fixed net-internals test. #
Total comments: 4
Patch Set 12 : Rebase to r189155. #Patch Set 13 : Incorporated comments. #
Messages
Total messages: 28 (0 generated)
Ricardo: PTAL? This should look at least somewhat familiar from https://codereview.chromium.org/12086021/; I've added conditionalized based on a new flag, added a flag indicating when the failover happens and test code, and I'd like to do a round of review. I'm sure there are many ways I didn't do the optimal thing, and I'm looking forward to finding out what they were :-}. Gavin cc'd FHI.
[Sorry, meant to ask this in last post and forgot. ] One specific question: I'm currently conditionalizing on the cache entry being in READ_WRITE mode, but naively it would seem like any mode that includes READ would be good. But I'm not sure I understand the subtleties of the various modes; do you have an opinion as to the right conditionalization?
READ_WRITE looks fine. Just READ should not reach the point of receiving a network response, and UPDATE has a read bit that this code should ignore (as in not use the cache). https://codereview.chromium.org/12310075/diff/5007/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/12310075/diff/5007/net/base/load_flags_list.h... net/base/load_flags_list.h:121: LOAD_FLAG(CACHE_RETURN_IF_OFFLINE, 1 << 27) It would be nice if this were beside the other cache flags, but that would screw the net-internals page when looking at older versions. Sad that we have that unintended dependency. https://codereview.chromium.org/12310075/diff/5007/net/base/load_flags_list.h... net/base/load_flags_list.h:121: LOAD_FLAG(CACHE_RETURN_IF_OFFLINE, 1 << 27) FROM_CACHE_IF_OFFLINE? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:817: if (effective_load_flags_ & LOAD_CACHE_RETURN_IF_OFFLINE && nit: single space before &... and add () around this one for clarity https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:818: mode_ == READ_WRITE && entry_ && !partial_ && result != OK) { nit: move result != OK to the beginning of this line https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:818: mode_ == READ_WRITE && entry_ && !partial_ && result != OK) { it would be nice to make this work with byte ranges. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:819: switch (result) { if (IsOffline(result)) ? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:829: cache_->ConvertWriterToReader(entry_); maybe extract this and 1800-1818 into a new method. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1812: response_.was_cache_override = false; remove? (see other comments) https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1831: // We don't want to switch mode to WRITE here as in offline update the comment at 1822 instead of adding a new one here? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1971: response_.was_cache_override = true; Feels wrong to do this, especially on this method. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:168: This file defines a bunch of transactions... which means that something is fishy if the current test are passing (as in we need to find out all places that define transactions). https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1207: // network success, offline failure, and non-offline failure. sounds like three different tests. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1208: TEST(HttpCache, SimpleGET_CacheOverride) { could you move this next to a test that goes over behavior with load flags? Around 840? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1209: MockHttpCache cache(net::HttpCache::DefaultBackend::InMemory(1024 * 1024)); why a real cache? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1239: rv = callback.WaitForResult(); GetResult(rv) https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... File net/http/http_response_info.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.cc:86: RESPONSE_INFO_WAS_CACHE_OVERRIDE = 1 << 18, This enum is for storing info about this response on the storage, i.e. for stuff that tells us something about what we have stored in the cache (as opposed to a property of how we are fulfilling a request). https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_info.h File net/http/http_response_info.h (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.h:63: // because of a LOAD_PREFERRING_CACHE or LOAD_CACHE_RETURN_IF_OFFLINE Why set a flag when the request is issued with LOAD_PREFERRING_CACHE? If we do that, having a flag that says "cache_override" when we do exactly what the user is asking for is weird. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.h:65: bool was_cache_override; nit: move after was_cached, and maybe convey instead that the system is offline. https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... File net/http/http_transaction_unittest.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... net/http/http_transaction_unittest.cc:237: Should'n we be returning here if the transaction specifies an error code? https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... File net/http/http_transaction_unittest.h (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... net/http/http_transaction_unittest.h:65: net::Error error_return; nit: return_code ? Maybe with a comment saying that any value other than OK causes that code to be returned by MockNetworkTransaction
I've incorporated most of your comments, and included some questions about the remainder. PTAL? https://codereview.chromium.org/12310075/diff/5007/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/12310075/diff/5007/net/base/load_flags_list.h... net/base/load_flags_list.h:121: LOAD_FLAG(CACHE_RETURN_IF_OFFLINE, 1 << 27) On 2013/03/05 02:58:19, rvargas wrote: > It would be nice if this were beside the other cache flags, but that would screw > the net-internals page when looking at older versions. Sad that we have that > unintended dependency. Agreed. I don't see a way around the problem, though :-{. https://codereview.chromium.org/12310075/diff/5007/net/base/load_flags_list.h... net/base/load_flags_list.h:121: LOAD_FLAG(CACHE_RETURN_IF_OFFLINE, 1 << 27) On 2013/03/05 02:58:19, rvargas wrote: > FROM_CACHE_IF_OFFLINE? Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:817: if (effective_load_flags_ & LOAD_CACHE_RETURN_IF_OFFLINE && On 2013/03/05 02:58:19, rvargas wrote: > nit: single space before &... and add () around this one for clarity Whoops; done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:818: mode_ == READ_WRITE && entry_ && !partial_ && result != OK) { On 2013/03/05 02:58:19, rvargas wrote: > nit: move result != OK to the beginning of this line Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:818: mode_ == READ_WRITE && entry_ && !partial_ && result != OK) { On 2013/03/05 02:58:19, rvargas wrote: > it would be nice to make this work with byte ranges. I'm happy to take a swing at that, but I'm not completely sure looking at the code how it should be done. According to the comments at PartialData declaration, there can be many ranges specified by partial_. I'm inclined to think that the first offline network error (here) would result in switching over to offline mode. But it's not clear to me that we will have read in all the cache entries associated with all the ranges described by partial_ at this point, so we might need to loop more times through the state diagram to bring everything in. Is that about right? Any more guidance as to how I should implement this? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:819: switch (result) { On 2013/03/05 02:58:19, rvargas wrote: > if (IsOffline(result)) ? Done. Note that I took the opportunity to get rid of the result!=OK check (since error == OK is not an offline result :-}); let me know if you find that makes the code less understandable. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:829: cache_->ConvertWriterToReader(entry_); On 2013/03/05 02:58:19, rvargas wrote: > maybe extract this and 1800-1818 into a new method. Say more? I don't follow the suggestion, probably because I don't grok the cache architecture well. BeginCacheValidation() was called noticeably earlier in the state diagram (when we were deciding whether to send the network request or not), so I'm not sure how we combine code between the two locations. Could you sketch out in more detail what you're thinking? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1812: response_.was_cache_override = false; On 2013/03/05 02:58:19, rvargas wrote: > remove? (see other comments) As noted in other comment, I don't yet follow. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1831: // We don't want to switch mode to WRITE here as in offline On 2013/03/05 02:58:19, rvargas wrote: > update the comment at 1822 instead of adding a new one here? Done. Note that I changed the comment to only refer to switching mode to WRITE because I don't see how the old code would have switched to READ, so you should evaluate the new comment too. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:1971: response_.was_cache_override = true; On 2013/03/05 02:58:19, rvargas wrote: > Feels wrong to do this, especially on this method. ETA: One of your later comments made it clear your opinion on the question I ask below, so I've removed this code (option (a)). I left my comment in for context. Fair. I can see two alternatives: a) Don't set this flag for LOAD_PREFERRING_CACHE; the caller can pretty much assume the value was from cache in that case. They'll be wrong sometimes, but not most of the time. *And* I don't currently see a use for that functionality, it just "felt right" to me to set the output flag for LOAD_PREFERRING_CACHE if the cache was used. b) Re-plumb the logic so that was_cache_override is set at a higher level. I'm not sure the right way to do (b); maybe return another variable from this function indicating that the cache override path was taken (so the function stays non-side-effecting) and do the assignment at a higher level. Do you have a preference between those alternatives? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:168: On 2013/03/05 02:58:19, rvargas wrote: > This file defines a bunch of transactions... which means that something is fishy > if the current test are passing (as in we need to find out all places that > define transactions). While I completely don't guarantee that I saw everything, I tried to look at all defined transactions, and they all either copied from one of the template transactions defined in http_transaction_unittest.* or initialized with 0, both of which work with my new code. So it doesn't surprise me that the current tests pass. Can you point me at a single test that you think should fail, and I'll use that to refine my scanning filter? https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1207: // network success, offline failure, and non-offline failure. On 2013/03/05 02:58:19, rvargas wrote: > sounds like three different tests. Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1208: TEST(HttpCache, SimpleGET_CacheOverride) { On 2013/03/05 02:58:19, rvargas wrote: > could you move this next to a test that goes over behavior with load flags? > Around 840? Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1209: MockHttpCache cache(net::HttpCache::DefaultBackend::InMemory(1024 * 1024)); On 2013/03/05 02:58:19, rvargas wrote: > why a real cache? Naivete': I copy-pasted from the wrong test. Fixed. (Presuming "real cache" == not using the null constructor.) https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:1239: rv = callback.WaitForResult(); On 2013/03/05 02:58:19, rvargas wrote: > GetResult(rv) Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... File net/http/http_response_info.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.cc:86: RESPONSE_INFO_WAS_CACHE_OVERRIDE = 1 << 18, On 2013/03/05 02:58:19, rvargas wrote: > This enum is for storing info about this response on the storage, i.e. for stuff > that tells us something about what we have stored in the cache (as opposed to a > property of how we are fulfilling a request). The enum was used for pickling and unpickling. If you're sure we'll never need to get this info from an HttpResponseInfo that's been persisted, I'll yank out references to the enum; it just seems weird to me for a bit not to survive persisting. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_info.h File net/http/http_response_info.h (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.h:63: // because of a LOAD_PREFERRING_CACHE or LOAD_CACHE_RETURN_IF_OFFLINE On 2013/03/05 02:58:19, rvargas wrote: > Why set a flag when the request is issued with LOAD_PREFERRING_CACHE? > > If we do that, having a flag that says "cache_override" when we do exactly what > the user is asking for is weird. Well, that answers another question I asked you :-}. I'll change this. Having said that, my thought was that if we overrode the "normal" cache behavior, we should indicate it. As you've pointed out, LOAD_PREFERRING_CACHE doesn't always use the cache, so there is information returned in that case. But I have no use case for that information, so I'm happy to nuke it. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.h:65: bool was_cache_override; On 2013/03/05 02:58:19, rvargas wrote: > nit: move after was_cached, and maybe convey instead that the system is offline. Done, though I phrased it as "was unable to contact the server" since I'm also trying to allow for network partition or partial offline. Let me know if you'd like me to change the name of the flag as well. https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... File net/http/http_transaction_unittest.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... net/http/http_transaction_unittest.cc:237: On 2013/03/05 02:58:19, rvargas wrote: > Should'n we be returning here if the transaction specifies an error code? Good point. Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... File net/http/http_transaction_unittest.h (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_transaction_... net/http/http_transaction_unittest.h:65: net::Error error_return; On 2013/03/05 02:58:19, rvargas wrote: > nit: return_code ? Maybe with a comment saying that any value other than OK > causes that code to be returned by MockNetworkTransaction Done.
https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:818: mode_ == READ_WRITE && entry_ && !partial_ && result != OK) { On 2013/03/05 23:16:14, rdsmith wrote: > On 2013/03/05 02:58:19, rvargas wrote: > > it would be nice to make this work with byte ranges. > > I'm happy to take a swing at that, but I'm not completely sure looking at the > code how it should be done. According to the comments at PartialData > declaration, there can be many ranges specified by partial_. I'm inclined to > think that the first offline network error (here) would result in switching over > to offline mode. But it's not clear to me that we will have read in all the > cache entries associated with all the ranges described by partial_ at this > point, so we might need to loop more times through the state diagram to bring > everything in. Is that about right? Any more guidance as to how I should > implement this? I don't think we need to do that on the first version. The general idea would be that if the current logic says that the reason for revalidating a resource is because of http rules (as opposed of being truncated or part of the range not being available), then we can treat that as any other request and return cached data when offline. It is true that by the time we see a byte range request we should be already in offline mode, but that concept (of switching for the rest of the page) is beyond the scope of net code, so we should assume that seeking a video can be the first signal to detect offline. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:829: cache_->ConvertWriterToReader(entry_); On 2013/03/05 23:16:14, rdsmith wrote: > On 2013/03/05 02:58:19, rvargas wrote: > > maybe extract this and 1800-1818 into a new method. > > Say more? I don't follow the suggestion, probably because I don't grok the > cache architecture well. BeginCacheValidation() was called noticeably earlier > in the state diagram (when we were deciding whether to send the network request > or not), so I'm not sure how we combine code between the two locations. Could > you sketch out in more detail what you're thinking? I just meant something like: if (effective_flags...) { UpdateTxPattern(not_covered); was_override = true; return SetupEntryForRead(); } int SetupEntryForRead() { network_trans_.reset(); if (partial_.get()) { if (truncated_ || is_sparse_ || !invalid_range_) { // We are going to return the saved response headers to the caller, so // we may need to adjust them first. next_state_ = STATE_PARTIAL_HEADERS_RECEIVED; return OK; } else { partial_.reset(); } } cache_->ConvertWriterToReader(entry_); mode_ = READ; if (entry_->disk_entry->GetDataSize(kMetadataIndex)) next_state_ = STATE_CACHE_READ_METADATA; return OK; } ... if (skip_validation) { UpdateTxPattern(entry_used); return SetupEntryForRead(); } https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:168: On 2013/03/05 23:16:14, rdsmith wrote: > On 2013/03/05 02:58:19, rvargas wrote: > > This file defines a bunch of transactions... which means that something is > fishy > > if the current test are passing (as in we need to find out all places that > > define transactions). > > While I completely don't guarantee that I saw everything, I tried to look at all > defined transactions, and they all either copied from one of the template > transactions defined in http_transaction_unittest.* or initialized with 0, both > of which work with my new code. So it doesn't surprise me that the current > tests pass. Can you point me at a single test that you think should fail, and > I'll use that to refine my scanning filter? lines 377, 240 https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... File net/http/http_response_info.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.cc:86: RESPONSE_INFO_WAS_CACHE_OVERRIDE = 1 << 18, On 2013/03/05 23:16:14, rdsmith wrote: > On 2013/03/05 02:58:19, rvargas wrote: > > This enum is for storing info about this response on the storage, i.e. for > stuff > > that tells us something about what we have stored in the cache (as opposed to > a > > property of how we are fulfilling a request). > > The enum was used for pickling and unpickling. If you're sure we'll never need > to get this info from an HttpResponseInfo that's been persisted, I'll yank out > references to the enum; it just seems weird to me for a bit not to survive > persisting. But we don't really want to persist this flag. That would mean that after returning content from from the cache (offline), the entry would change on disk saying that the flag was set... and we would be reading that flag again later. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_info.h File net/http/http_response_info.h (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.h:65: bool was_cache_override; On 2013/03/05 23:16:14, rdsmith wrote: > On 2013/03/05 02:58:19, rvargas wrote: > > nit: move after was_cached, and maybe convey instead that the system is > offline. > > Done, though I phrased it as "was unable to contact the server" since I'm also > trying to allow for network partition or partial offline. Let me know if you'd > like me to change the name of the flag as well. yes, I think it is better to change the name too. cache_override is not really self explanatory. The information that we are trying to convey is that the rest of the page should be loaded assuming the machine cannot contact the server, right? https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1819: // no we won't be falling back to using the cache entry in the typo: no we https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:862: EXPECT_FALSE(response_info.was_cache_override); It would be better to prime the cache so that at least there is a chance for the code to pick the wrong one. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:872: // Prime cache nit: period at the end https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:880: EXPECT_EQ(1, cache.disk_cache()->create_count()); https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:884: AddMockTransaction(&transaction); We should be removing the previous mock before adding another one. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:894: EXPECT_EQ(net::OK, rv); nit: merge this line with the previous one. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:898: EXPECT_TRUE(response_info1->was_cache_override); + EXPECT_TRUE(response_info1->was_cached);
PTAL. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:818: mode_ == READ_WRITE && entry_ && !partial_ && result != OK) { On 2013/03/06 03:11:48, rvargas wrote: > On 2013/03/05 23:16:14, rdsmith wrote: > > On 2013/03/05 02:58:19, rvargas wrote: > > > it would be nice to make this work with byte ranges. > > > > I'm happy to take a swing at that, but I'm not completely sure looking at the > > code how it should be done. According to the comments at PartialData > > declaration, there can be many ranges specified by partial_. I'm inclined to > > think that the first offline network error (here) would result in switching > over > > to offline mode. But it's not clear to me that we will have read in all the > > cache entries associated with all the ranges described by partial_ at this > > point, so we might need to loop more times through the state diagram to bring > > everything in. Is that about right? Any more guidance as to how I should > > implement this? > > I don't think we need to do that on the first version. The general idea would be > that if the current logic says that the reason for revalidating a resource is > because of http rules (as opposed of being truncated or part of the range not > being available), then we can treat that as any other request and return cached > data when offline. > > It is true that by the time we see a byte range request we should be already in > offline mode, but that concept (of switching for the rest of the page) is beyond > the scope of net code, so we should assume that seeking a video can be the first > signal to detect offline. Ok, sounds good. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_transa... net/http/http_cache_transaction.cc:829: cache_->ConvertWriterToReader(entry_); On 2013/03/06 03:11:48, rvargas wrote: > On 2013/03/05 23:16:14, rdsmith wrote: > > On 2013/03/05 02:58:19, rvargas wrote: > > > maybe extract this and 1800-1818 into a new method. > > > > Say more? I don't follow the suggestion, probably because I don't grok the > > cache architecture well. BeginCacheValidation() was called noticeably earlier > > in the state diagram (when we were deciding whether to send the network > request > > or not), so I'm not sure how we combine code between the two locations. Could > > you sketch out in more detail what you're thinking? > > I just meant something like: > > if (effective_flags...) { > UpdateTxPattern(not_covered); > was_override = true; > return SetupEntryForRead(); > } > > int SetupEntryForRead() { > network_trans_.reset(); > if (partial_.get()) { > if (truncated_ || is_sparse_ || !invalid_range_) { > // We are going to return the saved response headers to the caller, so > // we may need to adjust them first. > next_state_ = STATE_PARTIAL_HEADERS_RECEIVED; > return OK; > } else { > partial_.reset(); > } > } > cache_->ConvertWriterToReader(entry_); > mode_ = READ; > > if (entry_->disk_entry->GetDataSize(kMetadataIndex)) > next_state_ = STATE_CACHE_READ_METADATA; > > return OK; > } > ... > > if (skip_validation) { > UpdateTxPattern(entry_used); > return SetupEntryForRead(); > } Ah, gotcha. I'm not used to thinking of things this way, so I'm relying on you and the tests to make sure I got it right. Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_cache_unitte... net/http/http_cache_unittest.cc:168: On 2013/03/06 03:11:48, rvargas wrote: > On 2013/03/05 23:16:14, rdsmith wrote: > > On 2013/03/05 02:58:19, rvargas wrote: > > > This file defines a bunch of transactions... which means that something is > > fishy > > > if the current test are passing (as in we need to find out all places that > > > define transactions). > > > > While I completely don't guarantee that I saw everything, I tried to look at > all > > defined transactions, and they all either copied from one of the template > > transactions defined in http_transaction_unittest.* or initialized with 0, > both > > of which work with my new code. So it doesn't surprise me that the current > > tests pass. Can you point me at a single test that you think should fail, and > > I'll use that to refine my scanning filter? > > lines 377, 240 Ah, got it. Filter failure was looking at codesearch "instantiations" and not scanning the references too (not sure why it classified those there, but hey). Got those two, as well as an extra one in url_request_job_unittest.cc. I also scanned the ScopedMockTransaction s, but they're all based off one of the constant ones, and hence are ok. The reasons the test were passing is, I assume, zero fill; the last entry in the ones I replaced was zero, and that presumably got extended into the return code. I didn't modify the initializations that look like: MockTransaction mock_network_response = { 0 }; They're as good (or dodgy) as they always were. Let me know if you think I've skipped anything. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... File net/http/http_response_info.cc (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.cc:86: RESPONSE_INFO_WAS_CACHE_OVERRIDE = 1 << 18, On 2013/03/06 03:11:48, rvargas wrote: > On 2013/03/05 23:16:14, rdsmith wrote: > > On 2013/03/05 02:58:19, rvargas wrote: > > > This enum is for storing info about this response on the storage, i.e. for > > stuff > > > that tells us something about what we have stored in the cache (as opposed > to > > a > > > property of how we are fulfilling a request). > > > > The enum was used for pickling and unpickling. If you're sure we'll never > need > > to get this info from an HttpResponseInfo that's been persisted, I'll yank out > > references to the enum; it just seems weird to me for a bit not to survive > > persisting. > > But we don't really want to persist this flag. That would mean that after > returning content from from the cache (offline), the entry would change on disk > saying that the flag was set... and we would be reading that flag again later. Done. https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_info.h File net/http/http_response_info.h (right): https://codereview.chromium.org/12310075/diff/5007/net/http/http_response_inf... net/http/http_response_info.h:65: bool was_cache_override; On 2013/03/06 03:11:48, rvargas wrote: > On 2013/03/05 23:16:14, rdsmith wrote: > > On 2013/03/05 02:58:19, rvargas wrote: > > > nit: move after was_cached, and maybe convey instead that the system is > > offline. > > > > Done, though I phrased it as "was unable to contact the server" since I'm also > > trying to allow for network partition or partial offline. Let me know if > you'd > > like me to change the name of the flag as well. > > yes, I think it is better to change the name too. cache_override is not really > self explanatory. The information that we are trying to convey is that the rest > of the page should be loaded assuming the machine cannot contact the server, > right? SG. I used "server_data_unavailable"; let me know if you'd like something different. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_trans... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_trans... net/http/http_cache_transaction.cc:1819: // no we won't be falling back to using the cache entry in the On 2013/03/06 03:11:48, rvargas wrote: > typo: no we Done. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:862: EXPECT_FALSE(response_info.was_cache_override); On 2013/03/06 03:11:48, rvargas wrote: > It would be better to prime the cache so that at least there is a chance for the > code to pick the wrong one. Good point. Done. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:872: // Prime cache On 2013/03/06 03:11:48, rvargas wrote: > nit: period at the end Done. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:880: On 2013/03/06 03:11:48, rvargas wrote: > EXPECT_EQ(1, cache.disk_cache()->create_count()); Done. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:884: AddMockTransaction(&transaction); On 2013/03/06 03:11:48, rvargas wrote: > We should be removing the previous mock before adding another one. Done. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:894: EXPECT_EQ(net::OK, rv); On 2013/03/06 03:11:48, rvargas wrote: > nit: merge this line with the previous one. Done. https://codereview.chromium.org/12310075/diff/17001/net/http/http_cache_unitt... net/http/http_cache_unittest.cc:898: EXPECT_TRUE(response_info1->was_cache_override); On 2013/03/06 03:11:48, rvargas wrote: > + EXPECT_TRUE(response_info1->was_cached); Done.
LGTM
Actually, I was chatting with Eric and he mentioned that all flag names used by net logs are actually saved into each dump, so that means that logging is not preventing us from reordering load flags to follow the natural order. Do you mind doing that?. Hopefully all tests should still pass :)
On 2013/03/06 23:47:37, rvargas wrote: > Actually, I was chatting with Eric and he mentioned that all flag names used by > net logs are actually saved into each dump, so that means that logging is not > preventing us from reordering load flags to follow the natural order. > > Do you mind doing that?. Hopefully all tests should still pass :) Done and dispatched try jobs. Do you want to take another look? [Separately: +droger FHI.]
lgtm https://codereview.chromium.org/12310075/diff/42001/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/12310075/diff/42001/net/base/load_flags_list.... net/base/load_flags_list.h:122: looks like now there's an extra line (but it could be a rietveld artifact)
https://codereview.chromium.org/12310075/diff/42001/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/12310075/diff/42001/net/base/load_flags_list.... net/base/load_flags_list.h:122: On 2013/03/13 19:40:16, rvargas wrote: > looks like now there's an extra line (but it could be a rietveld artifact) Whoops, no, that's my bad. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/48001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/48001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/72001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/72001
Ricardo: We have a test failure in net-internals, as we thought might happen: http://build.chromium.org/p/tryserver.chromium/builders/win7_aura/builds/1989... . Any objection to my reverting the recent flags re-order? I suspect there's not going to be a clever way around this--my guess is that what we persist in net-internals is the load flags integer, and we break it out on display. But I'm not sure where the code is.
On 2013/03/14 19:19:16, rdsmith wrote: > Ricardo: We have a test failure in net-internals, as we thought might happen: > http://build.chromium.org/p/tryserver.chromium/builders/win7_aura/builds/1989... > . > > Any objection to my reverting the recent flags re-order? I suspect there's not > going to be a clever way around this--my guess is that what we persist in > net-internals is the load flags integer, and we break it out on display. But > I'm not sure where the code is. Seems to be a case of adjusting the net-internals unit test. It does something like this: 'params': { 'load_flags': 68223104, 'method': 'GET', ... ' --> load_flags = 68223104 ' + '(ENABLE_LOAD_TIMING | MAIN_FRAME | MAYBE_USER_GESTURE ' + '| VERIFY_EV_CERT)\n' + when building the expected result. The fact that the actual result looks fine seem to imply that everything is working as intended (except the old hard-coded flag value)
Ricardo: PTAL?
LGTM after nits. https://codereview.chromium.org/12310075/diff/100001/chrome/test/data/webui/n... File chrome/test/data/webui/net_internals/log_view_painter.js (right): https://codereview.chromium.org/12310075/diff/100001/chrome/test/data/webui/n... chrome/test/data/webui/net_internals/log_view_painter.js:1042: 'ENABLE_LOAD_TIMING | MAIN_FRAME | MAYBE_USER_GESTURE | VERIFY_EV_CERT)\n' + nit: the rest of the file is careful to indent in this case, so it seems better to use an extra line but keep the indentation. https://codereview.chromium.org/12310075/diff/100001/chrome/test/data/webui/n... chrome/test/data/webui/net_internals/log_view_painter.js:1048: 'ENABLE_LOAD_TIMING | MAIN_FRAME | MAYBE_USER_GESTURE | VERIFY_EV_CERT)\n' + same here.
Thanks! https://codereview.chromium.org/12310075/diff/100001/chrome/test/data/webui/n... File chrome/test/data/webui/net_internals/log_view_painter.js (right): https://codereview.chromium.org/12310075/diff/100001/chrome/test/data/webui/n... chrome/test/data/webui/net_internals/log_view_painter.js:1042: 'ENABLE_LOAD_TIMING | MAIN_FRAME | MAYBE_USER_GESTURE | VERIFY_EV_CERT)\n' + On 2013/03/20 21:00:02, rvargas wrote: > nit: the rest of the file is careful to indent in this case, so it seems better > to use an extra line but keep the indentation. Good point; I hadn't noticed the usual pattern for this case. Done. https://codereview.chromium.org/12310075/diff/100001/chrome/test/data/webui/n... chrome/test/data/webui/net_internals/log_view_painter.js:1048: 'ENABLE_LOAD_TIMING | MAIN_FRAME | MAYBE_USER_GESTURE | VERIFY_EV_CERT)\n' + On 2013/03/20 21:00:02, rvargas wrote: > same here. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/112002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/112002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/112002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12310075/112002
Message was sent while issue was closed.
Change committed as 190020 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
