Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(680)

Issue 2721933002: HttpCache::Transaction layer allowing parallel validation (Closed)

Created:
3 years, 9 months ago by shivanisha
Modified:
3 years, 6 months ago
CC:
cbentzel+watch_chromium.org, chromium-reviews, gavinp+disk_chromium.org, net-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

This CL is a precursor to allowing shared writing to fix cache lock. This CL allows transactions to continue to their validation phase even when another transaction is the active reader/writer. After the validation phase, if its a match the transaction might wait till the response is written to the cache by the active writer. If its not a match the transaction will doom the entry and go to the network. In a subsequent CL, the not matching case will create a new entry as well. BUG=472740, 715913, 715974, 715920, 715911, 713348 Review-Url: https://codereview.chromium.org/2721933002 Cr-Original-Commit-Position: refs/heads/master@{#467426} Committed: https://chromium.googlesource.com/chromium/src/+/1e2e347f957ef889aaee527bb757849f76e8a808 Review-Url: https://codereview.chromium.org/2721933002 Cr-Commit-Position: refs/heads/master@{#479204} Committed: https://chromium.googlesource.com/chromium/src/+/8061c420676998bda77caa74581ea8061860f438

Patch Set 1 : Initial patch #

Total comments: 75

Patch Set 2 : Feedback addressed, more tests and refactoring #

Total comments: 15

Patch Set 3 : Rough patch to discuss if ActiveEntry would be better designed with a state machine #

Total comments: 1

Patch Set 4 : ActiveEntry state machine (testing in progress, uploading for discussion) #

Patch Set 5 : Simplified ActiveEntry's state transitions #

Total comments: 42

Patch Set 6 : Feedback addresses + rebased till refs/heads/master@{#458081} #

Patch Set 7 : HttpCache::Transaction feedback addressed #

Patch Set 8 : DoLoop modified #

Patch Set 9 : Use TransitionToState (Rebased till refs/heads/master@{#459057}) #

Total comments: 7

Patch Set 10 : Fixed DoneWithEntry for headers_transaction #

Total comments: 9

Patch Set 11 : Fixed HttpCache.StopCachingWithAuthDeletesEntry to not invoke Read after receiving a 401 #

Patch Set 12 : Josh's latest feedback addressed. #

Total comments: 1

Patch Set 13 : HttpCache::IsCancelResponseBody added. #

Total comments: 36

Patch Set 14 : Josh's feedback on HttpCache::Transaction addressed #

Total comments: 16

Patch Set 15 : Feedback addressed (based on refs/heads/master@{#459057}) #

Total comments: 23

Patch Set 16 : Feedback addressed. #

Total comments: 2

Patch Set 17 : Tests. #

Patch Set 18 : Josh's latest feedback addressed #

Total comments: 9

Patch Set 19 : Randy's feedback addressed. #

Patch Set 20 : Rebased till refs/heads/master@{#462134} #

Total comments: 48

Patch Set 21 : Pause and restart network transaction framework, tests based on that. #

Patch Set 22 : Feedback addressed #

Patch Set 23 : Rebased with refs/heads/master@{#463004} #

Total comments: 19

Patch Set 24 : Remove loop from ProcessDoneHeadersQueue #

Patch Set 25 : Feedback addressed #

Total comments: 2

Patch Set 26 : Moved DeferNetworkStart, fixed a bot failure #

Total comments: 14

Patch Set 27 : Writer returns synchronously from DoneWithResponseHeaders #

Total comments: 4

Patch Set 28 : Fixed data race #

Total comments: 15

Patch Set 29 : Rebased + test-only changes for fixing the new Push Cache Lookup failing unit tests #

Total comments: 46

Patch Set 30 : Feedback addressed #

Patch Set 31 : Feedback addressed #

Total comments: 8

Patch Set 32 : Feedback addressed #

Total comments: 2

Patch Set 33 : Comments simplified #

Total comments: 2

Patch Set 34 : Cached HEAD request handling fixed #

Patch Set 35 : Fixed partial transactions handling in ActiveEntry #

Total comments: 3

Patch Set 36 : MetadataWriter to initialize request_info_ before transaction_ #

Patch Set 37 : Fixing HttpTransaction::Start()'s comment for |*request_info| for range requests. #

Patch Set 38 : Adding log to understand the webkit_tests failure. #

Patch Set 39 : Fixing resetting the cache_entry_status_. Should fix the webkit_tests failing in linux_trusty_blinkā€¦ #

Total comments: 2

Patch Set 40 : Made IsTransactionWritingIncomplete clearer for range requests #

Patch Set 41 : Simplified HttpTransaction::Start API contract for request info #

Total comments: 7

Patch Set 42 : Range requests tests + refactoring DoneWithEntry #

Patch Set 43 : Convert DCHECK -> CHECK for important invariants. #

Total comments: 6

Patch Set 44 : restart_info_.cache_entry_status to be set unconditionally #

Patch Set 45 : TODOs added for CHECKs and Transaction::Start #

Total comments: 4

Patch Set 46 : DoneReadingFromEntry replaced with DoneWithEntry #

Total comments: 9

Patch Set 47 : Rebased with refs/heads/master@{#475007} #

Total comments: 2

Patch Set 48 : rdsmith feedback. #

Total comments: 4

Patch Set 49 : jkarlin feedback + remove a dcheck based on a crash #

Total comments: 3

Patch Set 50 : IsTransactionExclusivelyWriting simplified and renamed to HasDependentTransactions #

Total comments: 8

Patch Set 51 : Truncation and StopCaching changes. #

Total comments: 34

Patch Set 52 : Feedback addressed #

Patch Set 53 : CanTransactionWriteResponseHeaders to take care of writer. #

Patch Set 54 : nit addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1968 lines, -488 lines) Patch
M net/http/http_cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 4 chunks +127 lines, -25 lines 0 comments Download
M net/http/http_cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 11 chunks +309 lines, -124 lines 0 comments Download
M net/http/http_cache_transaction.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 9 chunks +37 lines, -3 lines 0 comments Download
M net/http/http_cache_transaction.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 65 chunks +227 lines, -120 lines 0 comments Download
M net/http/http_cache_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 13 chunks +1051 lines, -193 lines 0 comments Download
M net/http/http_transaction.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 1 chunk +4 lines, -3 lines 0 comments Download
M net/http/http_transaction_test_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 1 chunk +2 lines, -0 lines 0 comments Download
M net/http/http_transaction_test_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 3 chunks +13 lines, -9 lines 0 comments Download
M net/http/mock_http_cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +10 lines, -0 lines 0 comments Download
M net/http/mock_http_cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +42 lines, -0 lines 0 comments Download
M net/url_request/url_request_http_job_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 2 chunks +134 lines, -0 lines 0 comments Download
M net/url_request/url_request_quic_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 2 chunks +9 lines, -8 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 286 (155 generated)
shivanisha
PTAL, Thanks! https://codereview.chromium.org/2721933002/diff/60001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (left): https://codereview.chromium.org/2721933002/diff/60001/net/http/http_cache_unittest.cc#oldcode6849 net/http/http_cache_unittest.cc:6849: // Tests that if a metadata writer ...
3 years, 9 months ago (2017-03-07 12:51:32 UTC) #9
jkarlin
Exciting. Here is a first round of comments. Something that makes this hard is that ...
3 years, 9 months ago (2017-03-07 16:45:42 UTC) #10
jkarlin
https://codereview.chromium.org/2721933002/diff/60001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2721933002/diff/60001/net/http/http_cache.h#newcode241 net/http/http_cache.h:241: class WorkItem; On 2017/03/07 16:45:41, jkarlin wrote: > This ...
3 years, 9 months ago (2017-03-07 17:23:00 UTC) #11
Randy Smith (Not in Mondays)
Shivani: The comments below don't really even count as a first pass, as a fair ...
3 years, 9 months ago (2017-03-07 22:54:30 UTC) #12
shivanisha
While I am working on the review comments, responding to the high level comments here: ...
3 years, 9 months ago (2017-03-08 16:47:46 UTC) #13
Randy Smith (Not in Mondays)
On 2017/03/08 16:47:46, shivanisha wrote: > While I am working on the review comments, responding ...
3 years, 9 months ago (2017-03-08 18:17:10 UTC) #14
shivanisha
On 2017/03/08 at 18:17:10, rdsmith wrote: > On 2017/03/08 16:47:46, shivanisha wrote: > > While ...
3 years, 9 months ago (2017-03-08 18:24:54 UTC) #15
shivanisha
On 2017/03/08 at 18:24:54, shivanisha wrote: > On 2017/03/08 at 18:17:10, rdsmith wrote: > > ...
3 years, 9 months ago (2017-03-08 19:05:14 UTC) #16
Randy Smith (Not in Mondays)
Shivani: A high level design suggestion I want to run by you. It might not ...
3 years, 9 months ago (2017-03-08 19:11:23 UTC) #17
shivanisha
This patch addresses most feedback except Randy's most recent high level design suggestion (thinking over ...
3 years, 9 months ago (2017-03-08 21:42:13 UTC) #18
shivanisha
On 2017/03/08 at 19:11:23, rdsmith wrote: > Shivani: A high level design suggestion I want ...
3 years, 9 months ago (2017-03-09 15:28:17 UTC) #19
Randy Smith (Not in Mondays)
On 2017/03/09 15:28:17, shivanisha wrote: > On 2017/03/08 at 19:11:23, rdsmith wrote: > > Shivani: ...
3 years, 9 months ago (2017-03-09 16:26:22 UTC) #20
shivanisha
On 2017/03/09 at 16:26:22, rdsmith wrote: > On 2017/03/09 15:28:17, shivanisha wrote: > > On ...
3 years, 9 months ago (2017-03-09 16:33:00 UTC) #21
Randy Smith (Not in Mondays)
On 2017/03/09 16:33:00, shivanisha wrote: > On 2017/03/09 at 16:26:22, rdsmith wrote: > > On ...
3 years, 9 months ago (2017-03-09 16:52:16 UTC) #22
shivanisha
On 2017/03/09 at 16:52:16, rdsmith wrote: > On 2017/03/09 16:33:00, shivanisha wrote: > > On ...
3 years, 9 months ago (2017-03-09 17:00:03 UTC) #23
Randy Smith (Not in Mondays)
On 2017/03/09 17:00:03, shivanisha wrote: > On 2017/03/09 at 16:52:16, rdsmith wrote: > > On ...
3 years, 9 months ago (2017-03-09 17:02:34 UTC) #24
jkarlin
A high level comment and some cleanup suggestions to http_cache.h https://codereview.chromium.org/2721933002/diff/80001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2721933002/diff/80001/net/http/http_cache.h#newcode282 ...
3 years, 9 months ago (2017-03-09 17:42:57 UTC) #25
jkarlin
https://codereview.chromium.org/2721933002/diff/100001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2721933002/diff/100001/net/http/http_cache.h#newcode279 net/http/http_cache.h:279: }; Suggest: NONE, WRITING_RESPONSE_HEADERS, WRITING_RESPONSE_BODY, READING VALIDATING doesn't make ...
3 years, 9 months ago (2017-03-09 18:38:33 UTC) #26
shivanisha
Patch 3 is very rough: not compiling, but I just wanted to make sure if ...
3 years, 9 months ago (2017-03-09 18:48:57 UTC) #27
shivanisha
This patch has the Active Entry state machine. Testing it is in progress but uploading ...
3 years, 9 months ago (2017-03-10 16:43:31 UTC) #28
shivanisha
This patch follows the f2f discussion that we had and has the following major changes: ...
3 years, 9 months ago (2017-03-14 17:34:45 UTC) #29
shivanisha
In addition to above, this patch also implements failing a headers_transaction if the writer fails. ...
3 years, 9 months ago (2017-03-16 00:54:54 UTC) #30
shivanisha
https://codereview.chromium.org/2721933002/diff/140001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2721933002/diff/140001/net/http/http_cache.h#newcode265 net/http/http_cache.h:265: bool IsReader(Transaction* transaction); Will remove this function since it ...
3 years, 9 months ago (2017-03-16 15:43:26 UTC) #31
Randy Smith (Not in Mondays)
I like this shift--thanks for implementing it! First round of comments. Two high level requests: ...
3 years, 9 months ago (2017-03-17 18:13:06 UTC) #32
shivanisha
This patch addresses Randy's latest feedback and some self review feedback. The latest code removes ...
3 years, 9 months ago (2017-03-20 16:34:52 UTC) #33
shivanisha
This patch addresses Randy's latest feedback in http_cache_transaction.cc This includes a new boolean variable is_wait_before_read_complete_ ...
3 years, 9 months ago (2017-03-20 18:14:49 UTC) #34
shivanisha
This patch (DoLoop modified) has the following major changes over earlier patches: - DoLoop does ...
3 years, 9 months ago (2017-03-22 20:55:30 UTC) #35
shivanisha
This patch uses TransitionToState after rebasing instead of setting next_state_ directly.
3 years, 9 months ago (2017-03-23 15:03:57 UTC) #36
shivanisha
On 2017/03/23 at 15:03:57, shivanisha wrote: > This patch uses TransitionToState after rebasing instead of ...
3 years, 9 months ago (2017-03-23 15:07:21 UTC) #37
shivanisha
This patch fixes DoneWithEntry for headers_transaction so that cancel flag can propagate to DoneWritingToEntry as ...
3 years, 9 months ago (2017-03-23 17:19:02 UTC) #38
jkarlin
Some comments after looking at http_cache_transaction. https://codereview.chromium.org/2721933002/diff/220001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/220001/net/http/http_cache_transaction.cc#newcode918 net/http/http_cache_transaction.cc:918: DCHECK(!entry_ || state ...
3 years, 9 months ago (2017-03-23 18:37:39 UTC) #39
shivanisha
https://codereview.chromium.org/2721933002/diff/220001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/220001/net/http/http_cache_transaction.cc#newcode918 net/http/http_cache_transaction.cc:918: DCHECK(!entry_ || state == STATE_WAIT_BEFORE_READ_COMPLETE); On 2017/03/23 at 18:37:38, ...
3 years, 8 months ago (2017-03-29 03:39:30 UTC) #44
shivanisha
https://codereview.chromium.org/2721933002/diff/240001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/240001/net/http/http_cache_transaction.cc#newcode232 net/http/http_cache_transaction.cc:232: cancel_request = !reading_ && created_entry_ && On 2017/03/29 at ...
3 years, 8 months ago (2017-03-29 03:41:43 UTC) #45
shivanisha
https://codereview.chromium.org/2721933002/diff/280001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/280001/net/http/http_cache_transaction.cc#newcode220 net/http/http_cache_transaction.cc:220: new_response_->headers->response_code() == 416))); This still has special case logic. ...
3 years, 8 months ago (2017-03-29 12:37:54 UTC) #46
shivanisha
A function added in HttpCache to compute whether destroying a given transaction will lead to ...
3 years, 8 months ago (2017-03-29 15:29:47 UTC) #47
jkarlin
HttpCacheTransaction is coming together. One last set of changes. The general idea is that the ...
3 years, 8 months ago (2017-03-30 14:25:26 UTC) #48
jkarlin
https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache.cc#newcode992 net/http/http_cache.cc:992: bool HttpCache::IsCancelResponseBody(ActiveEntry* entry, The name IsCancelResponseBody doesn't make sense ...
3 years, 8 months ago (2017-03-30 14:58:15 UTC) #49
shivanisha
Addressed Josh's feedback on HttpCache::Transaction. https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache_transaction.cc#newcode1001 net/http/http_cache_transaction.cc:1001: original_mode_ = mode_; On ...
3 years, 8 months ago (2017-03-30 16:38:41 UTC) #50
shivanisha
Also renamed state STATE_WAIT_BEFORE_READ to STATE_FINISH_HEADERS as discussed since it is also now reached when ...
3 years, 8 months ago (2017-03-30 16:45:59 UTC) #52
jkarlin
Love it! https://codereview.chromium.org/2721933002/diff/340001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2721933002/diff/340001/net/http/http_cache.h#newcode401 net/http/http_cache.h:401: int DoneResponseHeaders(ActiveEntry* entry, Suggest DoneWithResponseHeaders https://codereview.chromium.org/2721933002/diff/340001/net/http/http_cache_transaction.cc File ...
3 years, 8 months ago (2017-03-30 17:04:54 UTC) #53
Randy Smith (Not in Mondays)
My apologies that this has taken so long; this week's been both busy and stressful. ...
3 years, 8 months ago (2017-03-30 19:18:06 UTC) #54
shivanisha
Most recent feedback from all reviewers addressed. https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache.cc#newcode965 net/http/http_cache.cc:965: ProcessQueuedTransactions(entry); On ...
3 years, 8 months ago (2017-03-31 17:47:06 UTC) #55
Randy Smith (Not in Mondays)
Next round of comments. This is close, which is really awesome! Some notes on the ...
3 years, 8 months ago (2017-03-31 19:26:12 UTC) #56
jkarlin
https://codereview.chromium.org/2721933002/diff/360001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/360001/net/http/http_cache.cc#newcode1073 net/http/http_cache.cc:1073: // Transaction is done writing to entry in the ...
3 years, 8 months ago (2017-04-03 14:32:48 UTC) #57
shivanisha
Feedback addressed and added some ActiveEntry's state related details in the test framework. https://codereview.chromium.org/2721933002/diff/300001/net/http/http_cache.cc File ...
3 years, 8 months ago (2017-04-03 20:16:49 UTC) #58
jkarlin
https://codereview.chromium.org/2721933002/diff/380001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/380001/net/http/http_cache_transaction.cc#newcode1598 net/http/http_cache_transaction.cc:1598: !cache_->IsTransactionCurrentOrFutureWriter(entry_, this, I don't think the HCT should be ...
3 years, 8 months ago (2017-04-04 16:49:55 UTC) #59
shivanisha
Tests added.
3 years, 8 months ago (2017-04-04 16:53:03 UTC) #60
shivanisha
https://codereview.chromium.org/2721933002/diff/380001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/380001/net/http/http_cache_transaction.cc#newcode1598 net/http/http_cache_transaction.cc:1598: !cache_->IsTransactionCurrentOrFutureWriter(entry_, this, On 2017/04/04 at 16:49:55, jkarlin wrote: > ...
3 years, 8 months ago (2017-04-04 20:33:15 UTC) #62
Randy Smith (Not in Mondays)
Looking good. https://codereview.chromium.org/2721933002/diff/360001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/360001/net/http/http_cache.cc#newcode1020 net/http/http_cache.cc:1020: if (transaction == entry->done_headers_queue.front()) On 2017/04/03 20:16:48, ...
3 years, 8 months ago (2017-04-04 21:21:17 UTC) #67
shivanisha
https://codereview.chromium.org/2721933002/diff/360001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/360001/net/http/http_cache.cc#newcode1020 net/http/http_cache.cc:1020: if (transaction == entry->done_headers_queue.front()) On 2017/04/04 at 21:21:16, Randy ...
3 years, 8 months ago (2017-04-05 15:00:37 UTC) #68
Randy Smith (Not in Mondays)
I'm good with this CL except for the two tests I suggested. If you really ...
3 years, 8 months ago (2017-04-05 18:39:18 UTC) #85
shivanisha
On 2017/04/05 at 18:39:18, rdsmith wrote: > I'm good with this CL except for the ...
3 years, 8 months ago (2017-04-05 18:49:58 UTC) #86
Randy Smith (Not in Mondays)
On 2017/04/05 18:49:58, shivanisha wrote: > On 2017/04/05 at 18:39:18, rdsmith wrote: > > I'm ...
3 years, 8 months ago (2017-04-05 18:52:44 UTC) #87
jkarlin
First pass at http_cache.h/cc. Looks good. https://codereview.chromium.org/2721933002/diff/500001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/500001/net/http/http_cache.cc#newcode855 net/http/http_cache.cc:855: return OK; Once ...
3 years, 8 months ago (2017-04-06 17:37:14 UTC) #88
shivanisha
Added a patch that includes the framework for pausing and restarting a mock network transaction. ...
3 years, 8 months ago (2017-04-06 19:52:18 UTC) #89
shivanisha
rdsmith@, PTAL if you would like for tests added for headers_transaction state. Also, I ended ...
3 years, 8 months ago (2017-04-07 21:11:17 UTC) #91
shivanisha
This patch removes the loop from ProcessDoneHeadersQueue and reverts back to post a task for ...
3 years, 8 months ago (2017-04-11 14:20:39 UTC) #96
jkarlin
https://codereview.chromium.org/2721933002/diff/500001/net/http/http_cache.h File net/http/http_cache.h (right): https://codereview.chromium.org/2721933002/diff/500001/net/http/http_cache.h#newcode413 net/http/http_cache.h:413: // writing the response body. On 2017/04/07 21:11:16, shivanisha ...
3 years, 8 months ago (2017-04-11 14:38:28 UTC) #97
shivanisha
https://codereview.chromium.org/2721933002/diff/580001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/580001/net/http/http_cache.cc#newcode982 net/http/http_cache.cc:982: void HttpCache::ProcessAddToEntryQueue(ActiveEntry* entry) { On 2017/04/11 at 14:38:28, jkarlin ...
3 years, 8 months ago (2017-04-11 15:35:38 UTC) #99
Randy Smith (Not in Mondays)
Still LGTM. https://codereview.chromium.org/2721933002/diff/620001/net/http/http_transaction_test_util.cc File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/2721933002/diff/620001/net/http/http_transaction_test_util.cc#newcode494 net/http/http_transaction_test_util.cc:494: void MockNetworkTransaction::DeferNetworkStart(bool* defer) { nit, suggestion: This ...
3 years, 8 months ago (2017-04-11 19:56:45 UTC) #103
shivanisha
(There's a question in this message at the end regarding a failing bot test) The ...
3 years, 8 months ago (2017-04-12 16:46:03 UTC) #104
shivanisha
> I have a question about suggestions to fix another bot failure SpdyNetworkTransactionTest.TestRawHeaderSizeSuccessfullRequest > > ...
3 years, 8 months ago (2017-04-12 18:12:53 UTC) #105
shivanisha
On 2017/04/12 at 18:12:53, shivanisha wrote: > > I have a question about suggestions to ...
3 years, 8 months ago (2017-04-13 02:31:31 UTC) #108
jkarlin
> > Its failing now because there is a task posted before a transaction gets ...
3 years, 8 months ago (2017-04-13 13:03:14 UTC) #111
jkarlin
https://codereview.chromium.org/2721933002/diff/640001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/640001/net/http/http_cache.cc#newcode859 net/http/http_cache.cc:859: DCHECK(!(transaction->mode() & Transaction::WRITE)); DCHECK(entry->done_headers_queue.empty() || !(transaction->mode() & Transaction::WRITE)) https://codereview.chromium.org/2721933002/diff/640001/net/http/http_cache.cc#newcode940 ...
3 years, 8 months ago (2017-04-13 15:05:53 UTC) #112
jkarlin
https://codereview.chromium.org/2721933002/diff/640001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/640001/net/http/http_cache.cc#newcode971 net/http/http_cache.cc:971: void HttpCache::ProcessQueuedTransactions(ActiveEntry* entry) { On 2017/04/13 15:05:52, jkarlin wrote: ...
3 years, 8 months ago (2017-04-13 15:09:20 UTC) #113
shivanisha
This patch fixes the failing spdy test by making DoneWithResponseHeaders return OK for the writer ...
3 years, 8 months ago (2017-04-13 16:54:54 UTC) #116
shivanisha
Patch 28 fixes the data races that were caught by tsan . The fix includes ...
3 years, 8 months ago (2017-04-14 20:28:24 UTC) #125
shivanisha
On 2017/04/14 at 20:28:24, shivanisha wrote: > Patch 28 fixes the data races that were ...
3 years, 8 months ago (2017-04-18 15:10:30 UTC) #128
Randy Smith (Not in Mondays)
My only concern is about the apparent low probability problem being opened up by not ...
3 years, 8 months ago (2017-04-19 17:26:34 UTC) #129
jkarlin
Nice! A few more comments. https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache.cc#newcode1090 net/http/http_cache.cc:1090: return entry->writer ? true ...
3 years, 8 months ago (2017-04-20 15:05:41 UTC) #130
shivanisha
jkarlin@, rdsmith@, PTAL, thanks! This patch has all the latest feedback addressed. https://codereview.chromium.org/2721933002/diff/660001/net/http/http_cache.cc File net/http/http_cache.cc ...
3 years, 8 months ago (2017-04-21 23:06:02 UTC) #140
jkarlin
https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache.cc#newcode1090 net/http/http_cache.cc:1090: return entry->writer ? true : false; On 2017/04/21 23:06:00, ...
3 years, 8 months ago (2017-04-24 15:57:26 UTC) #141
shivanisha
https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache.cc#newcode1090 net/http/http_cache.cc:1090: return entry->writer ? true : false; On 2017/04/24 at ...
3 years, 8 months ago (2017-04-24 18:29:02 UTC) #144
jkarlin
One last thing and I think we're good. https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache_transaction.h File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2721933002/diff/680001/net/http/http_cache_transaction.h#newcode265 net/http/http_cache_transaction.h:265: // ...
3 years, 8 months ago (2017-04-25 16:00:52 UTC) #147
Randy Smith (Not in Mondays)
Again LGTM with the "Why is this safe for >2GB?" comment requested below. Thanks for ...
3 years, 8 months ago (2017-04-25 21:11:32 UTC) #148
asanka
https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc#newcode1208 net/http/http_cache_transaction.cc:1208: // data race since cache thread can also access ...
3 years, 8 months ago (2017-04-26 02:01:45 UTC) #150
Randy Smith (Not in Mondays)
On 2017/04/26 02:01:45, asanka wrote: > https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc#newcode1208 > ...
3 years, 8 months ago (2017-04-26 02:08:36 UTC) #151
shivanisha
https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache.cc#newcode126 net/http/http_cache.cc:126: if (index == kResponseContentIndex) On 2017/04/25 at 16:00:52, jkarlin ...
3 years, 8 months ago (2017-04-26 16:22:30 UTC) #156
shivanisha
Thank you Randy for a detailed and thoughtful review!
3 years, 7 months ago (2017-04-26 16:51:40 UTC) #157
jkarlin
lgtm! with nits https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc#newcode1208 net/http/http_cache_transaction.cc:1208: // data race since cache thread ...
3 years, 7 months ago (2017-04-26 17:11:02 UTC) #158
shivanisha
https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/760001/net/http/http_cache_transaction.cc#newcode1208 net/http/http_cache_transaction.cc:1208: // data race since cache thread can also access ...
3 years, 7 months ago (2017-04-26 17:48:38 UTC) #161
shivanisha
Thanks Josh for such a detailed and thoughtful review! https://codereview.chromium.org/2721933002/diff/780001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/780001/net/http/http_cache_transaction.cc#newcode1257 net/http/http_cache_transaction.cc:1257: ...
3 years, 7 months ago (2017-04-26 17:49:28 UTC) #162
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2721933002/800001
3 years, 7 months ago (2017-04-26 20:02:50 UTC) #167
commit-bot: I haz the power
Committed patchset #33 (id:800001) as https://chromium.googlesource.com/chromium/src/+/1e2e347f957ef889aaee527bb757849f76e8a808
3 years, 7 months ago (2017-04-26 20:09:37 UTC) #170
Randy Smith (Not in Mondays)
Congratulations!
3 years, 7 months ago (2017-04-26 20:39:13 UTC) #171
asanka
On 2017/04/26 20:39:13, Randy Smith (Not in Mondays) wrote: > Congratulations! Indeed. There are 170 ...
3 years, 7 months ago (2017-04-26 21:36:17 UTC) #172
Qiang(Joe) Xu
A revert of this CL (patchset #33 id:800001) has been created in https://codereview.chromium.org/2847653002/ by warx@chromium.org. ...
3 years, 7 months ago (2017-04-27 02:45:01 UTC) #173
shivanisha
Patches 34, 35 and 36 fix some of the crashes that came. I am looking ...
3 years, 7 months ago (2017-05-01 12:39:35 UTC) #181
shivanisha
Patch 37 fixes HttpTransaction::Start()'s comment for |*request_info| for range requests and fixes usage of request_ ...
3 years, 7 months ago (2017-05-01 15:37:39 UTC) #182
shivanisha
On 2017/05/01 at 15:37:39, shivanisha wrote: > Patch 37 fixes HttpTransaction::Start()'s comment for |*request_info| for ...
3 years, 7 months ago (2017-05-01 20:44:41 UTC) #187
Randy Smith (Not in Mondays)
Reviewed PS34-37; haven't reviewed after PS37 as there haven't been posts requesting review for those. ...
3 years, 7 months ago (2017-05-03 16:16:02 UTC) #188
shivanisha
PTAL, thanks! Patch 39 fixes the re-setting of cache_entry_status_ which was failing webkit tests in ...
3 years, 7 months ago (2017-05-03 19:50:18 UTC) #191
shivanisha
Patch 41 simplifies the lifetime requirement of request_info while sending it in HttpTransaction::Start(). Earlier the ...
3 years, 7 months ago (2017-05-03 20:04:33 UTC) #192
shivanisha
On 2017/05/03 at 20:04:33, shivanisha wrote: > Patch 41 simplifies the lifetime requirement of request_info ...
3 years, 7 months ago (2017-05-03 20:08:41 UTC) #193
shivanisha
Working on adding a few new unit tests, specifically for range requests. Will upload a ...
3 years, 7 months ago (2017-05-04 16:43:36 UTC) #195
shivanisha
rdsmith@, PTAL, thanks! The latest patch includes unit tests for range requests and some refactoring ...
3 years, 7 months ago (2017-05-08 19:15:21 UTC) #198
shivanisha
rdsmith@, PTAL, thanks! This patch converts the invariants of the Active Entry state transitions in ...
3 years, 7 months ago (2017-05-08 19:33:42 UTC) #201
shivanisha
On 2017/05/08 at 19:33:42, shivanisha wrote: > rdsmith@, PTAL, thanks! > > This patch converts ...
3 years, 7 months ago (2017-05-09 14:34:26 UTC) #204
Randy Smith (Not in Mondays)
If you're going to change the DCHECKs to CHECKs, can you add a TODO() to ...
3 years, 7 months ago (2017-05-09 23:20:32 UTC) #205
shivanisha
Randy, addressed your feedback, PTAL, Thanks. https://codereview.chromium.org/2721933002/diff/920001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/920001/net/http/http_cache_transaction.cc#newcode2135 net/http/http_cache_transaction.cc:2135: restart_info_.cache_entry_status = cache_entry_status_; ...
3 years, 7 months ago (2017-05-10 14:46:03 UTC) #206
Randy Smith (Not in Mondays)
Asymptotically approaching agreement :-}. (And I suspect the problem with the interface contract is that ...
3 years, 7 months ago (2017-05-10 21:34:26 UTC) #208
shivanisha
For the canary (60.0.3082.0) crashes, all but one of them can be speculated to be ...
3 years, 7 months ago (2017-05-11 17:50:45 UTC) #209
Randy Smith (Not in Mondays)
On 2017/05/11 17:50:45, shivanisha wrote: > For the canary (60.0.3082.0) crashes, all but one of ...
3 years, 7 months ago (2017-05-11 18:38:02 UTC) #210
Randy Smith (Not in Mondays)
On 2017/05/11 18:38:02, Randy Smith (Not in Mondays) wrote: > On 2017/05/11 17:50:45, shivanisha wrote: ...
3 years, 7 months ago (2017-05-11 18:51:07 UTC) #211
Randy Smith (Not in Mondays)
On 2017/05/11 18:51:07, Randy Smith (Not in Mondays) wrote: > On 2017/05/11 18:38:02, Randy Smith ...
3 years, 7 months ago (2017-05-11 18:52:58 UTC) #212
shivanisha
On 2017/05/11 at 18:38:02, rdsmith wrote: > On 2017/05/11 17:50:45, shivanisha wrote: > > For ...
3 years, 7 months ago (2017-05-11 19:01:23 UTC) #213
shivanisha
https://codereview.chromium.org/2721933002/diff/960001/net/http/http_transaction.h File net/http/http_transaction.h (right): https://codereview.chromium.org/2721933002/diff/960001/net/http/http_transaction.h#newcode56 net/http/http_transaction.h:56: // the consumer is alive; after that point, the ...
3 years, 7 months ago (2017-05-11 19:58:25 UTC) #214
Randy Smith (Not in Mondays)
On 2017/05/11 19:01:23, shivanisha wrote: > On 2017/05/11 at 18:38:02, rdsmith wrote: > > On ...
3 years, 7 months ago (2017-05-11 20:40:25 UTC) #215
shivanisha
On 2017/05/11 at 20:40:25, rdsmith wrote: > On 2017/05/11 19:01:23, shivanisha wrote: > > On ...
3 years, 7 months ago (2017-05-11 20:45:43 UTC) #216
Randy Smith (Not in Mondays)
On 2017/05/11 20:45:43, shivanisha wrote: > On 2017/05/11 at 20:40:25, rdsmith wrote: > > On ...
3 years, 7 months ago (2017-05-11 20:47:57 UTC) #217
Randy Smith (Not in Mondays)
https://codereview.chromium.org/2721933002/diff/960001/net/http/http_transaction.h File net/http/http_transaction.h (right): https://codereview.chromium.org/2721933002/diff/960001/net/http/http_transaction.h#newcode56 net/http/http_transaction.h:56: // the consumer is alive; after that point, the ...
3 years, 7 months ago (2017-05-11 21:47:10 UTC) #218
jkarlin
https://codereview.chromium.org/2721933002/diff/1000001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1000001/net/http/http_cache.cc#newcode847 net/http/http_cache.cc:847: CHECK(is_partial || (!entry->writer && entry->done_headers_queue.empty())); On 2017/05/11 21:47:09, Randy ...
3 years, 7 months ago (2017-05-16 17:25:52 UTC) #219
shivanisha
PTAL, Thanks. The latest patch adds the TODOs for converting CHECKs to DCHECKs and for ...
3 years, 7 months ago (2017-05-17 22:20:09 UTC) #222
shivanisha
PTAL, Thanks! The latest patch converts the calls to DoneReadingFromEntry to DoneWithEntry on sites where ...
3 years, 7 months ago (2017-05-18 19:07:05 UTC) #227
Randy Smith (Not in Mondays)
LGTM modulo linking TODOs to an actual bug for hopefully not happening future archaeology. https://codereview.chromium.org/2721933002/diff/1060001/net/http/http_cache.cc ...
3 years, 7 months ago (2017-05-21 21:32:03 UTC) #230
shivanisha
Rebased. https://codereview.chromium.org/2721933002/diff/1100001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2721933002/diff/1100001/net/http/http_cache_unittest.cc#newcode1193 net/http/http_cache_unittest.cc:1193: Starting from here till my next comment, these ...
3 years, 7 months ago (2017-05-26 16:10:37 UTC) #233
shivanisha
PTAL, Thanks! https://codereview.chromium.org/2721933002/diff/1060001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1060001/net/http/http_cache.cc#newcode835 net/http/http_cache.cc:835: // canary is clear of any crashes. ...
3 years, 7 months ago (2017-05-26 16:26:17 UTC) #236
Randy Smith (Not in Mondays)
Still LGTM. Thanks!
3 years, 6 months ago (2017-05-30 14:02:42 UTC) #239
jkarlin
Note that the comments are spread over two patchsets. https://codereview.chromium.org/2721933002/diff/1080001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2721933002/diff/1080001/net/http/http_cache_transaction.cc#newcode199 net/http/http_cache_transaction.cc:199: ...
3 years, 6 months ago (2017-05-30 14:42:38 UTC) #240
shivanisha
jkarlin, PTAL, Thanks! The latest patch has feedback addressed + a dcheck removed. The dcheck ...
3 years, 6 months ago (2017-05-31 17:32:13 UTC) #244
jkarlin
https://codereview.chromium.org/2721933002/diff/1160001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1160001/net/http/http_cache.cc#newcode872 net/http/http_cache.cc:872: bool cancel = false; Suggest: s/cancel/should_restart/ bool should_restart = ...
3 years, 6 months ago (2017-06-01 17:22:38 UTC) #247
shivanisha
Latest patch has josh's feedback addressed. https://codereview.chromium.org/2721933002/diff/1160001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1160001/net/http/http_cache.cc#newcode872 net/http/http_cache.cc:872: bool cancel = ...
3 years, 6 months ago (2017-06-01 18:28:05 UTC) #250
jkarlin
https://codereview.chromium.org/2721933002/diff/1180001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1180001/net/http/http_cache.cc#newcode891 net/http/http_cache.cc:891: // If the response is not written (cancel is ...
3 years, 6 months ago (2017-06-01 18:49:22 UTC) #251
shivanisha
PTAL, Thanks! Latest patch fixes truncation on destruction and StopCaching flows. https://codereview.chromium.org/2721933002/diff/1180001/net/http/http_cache.cc File net/http/http_cache.cc (right): ...
3 years, 6 months ago (2017-06-05 06:41:22 UTC) #262
jkarlin
Looking good! Just a few minor comments. https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc#newcode815 net/http/http_cache.cc:815: // Always ...
3 years, 6 months ago (2017-06-08 16:14:10 UTC) #265
shivanisha
https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc#newcode815 net/http/http_cache.cc:815: // Always add a new transaction to the queue ...
3 years, 6 months ago (2017-06-08 18:26:02 UTC) #267
jkarlin
lgtm with nit https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc#newcode999 net/http/http_cache.cc:999: Transaction* transaction) { On 2017/06/08 18:26:01, ...
3 years, 6 months ago (2017-06-13 13:03:32 UTC) #275
shivanisha
Thanks Josh and Randy! https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/2721933002/diff/1220001/net/http/http_cache.cc#newcode999 net/http/http_cache.cc:999: Transaction* transaction) { On 2017/06/13 ...
3 years, 6 months ago (2017-06-13 14:21:18 UTC) #278
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2721933002/1280001
3 years, 6 months ago (2017-06-13 23:27:44 UTC) #283
commit-bot: I haz the power
3 years, 6 months ago (2017-06-13 23:36:16 UTC) #286
Message was sent while issue was closed.
Committed patchset #54 (id:1280001) as
https://chromium.googlesource.com/chromium/src/+/8061c420676998bda77caa74581e...

Powered by Google App Engine
This is Rietveld 408576698