Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767033003:
[HttpCache::Transaction] Ensure each state only sets the next state once (Closed)
|
Created:
3 years, 9 months ago by jkarlin Modified:
3 years, 9 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[HttpCache::Transaction] Ensure each state only sets the next state once
To aid in readability, ensure that the next state is only set once per state.
This way the reader knows for sure what will happen after the state has been
set.
BUG=703923
Review-Url: https://codereview.chromium.org/2767033003
Cr-Commit-Position: refs/heads/master@{#458840}
Committed: https://chromium.googlesource.com/chromium/src/+/5de449fdaae6cb58ce02aed512f383857ef6bcfe
Patch Set 1 #Patch Set 2 : Nits #Patch Set 3 : Rebase #Patch Set 4 : Nits #Patch Set 5 : Verify that we're in the do loop #Patch Set 6 : Rebase #Patch Set 7 : Better dcheck message #
Total comments: 11
Patch Set 8 : Address comments from PS7 #Patch Set 9 : AutoReset #
Depends on Patchset: Messages
Total messages: 40 (33 generated)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by jkarlin@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 checked by jkarlin@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 checked by jkarlin@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 checked by jkarlin@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 checked by jkarlin@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...
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
Thanks very much for doing this, Josh; it's small, but I think it'll be quite useful. https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:720: in_do_loop_ = true; Thought (not even a suggestion): AutoReset? https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1128: weak_factory_.GetWeakPtr(), entry_lock_waiting_since_)); Why? https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1608: SetNextStateFromDoLoop(STATE_UPDATE_CACHED_RESPONSE_COMPLETE); Slight preference for rewriting the above conditional s.t. we explicitly set state along each path. (The rewrite I imagine is storing the result of HasHeaderVAlue in a boolean, then doing a (!success && !reading_) test for the state setting, but just setting things everywhere is fine too. As is what you have; I'm just expressing a preference.) https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:439: void SetNextStateFromDoLoop(State state); Mild preference for something like "TransitionTo", just because it's shorter and I think it implies we're in the DoLoop() context, but I totally get if you don't think that's explicit enough.
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 jkarlin@chromium.org to run a CQ dry run
PTAL! https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:720: in_do_loop_ = true; On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > Thought (not even a suggestion): AutoReset? I think we want to explicitly set it back to false before calling the callback at the end of the function, else we have reentrancy issues. https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1128: weak_factory_.GetWeakPtr(), entry_lock_waiting_since_)); On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > Why? OnAddToEntryTimeout ultimately winds up calling DoLoop(). We don't want to do that while we're in a DoLoop. https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:1608: SetNextStateFromDoLoop(STATE_UPDATE_CACHED_RESPONSE_COMPLETE); On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > Slight preference for rewriting the above conditional s.t. we explicitly set > state along each path. (The rewrite I imagine is storing the result of > HasHeaderVAlue in a boolean, then doing a (!success && !reading_) test for the > state setting, but just setting things everywhere is fine too. As is what you > have; I'm just expressing a preference.) Agree, this is lame. Fixed. https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.h (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.h:439: void SetNextStateFromDoLoop(State state); On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > Mild preference for something like "TransitionTo", just because it's shorter and > I think it implies we're in the DoLoop() context, but I totally get if you don't > think that's explicit enough. Done. Renamed to TransitionToState.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:720: in_do_loop_ = true; On 2017/03/22 17:12:00, jkarlin wrote: > On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > > Thought (not even a suggestion): AutoReset? > > I think we want to explicitly set it back to false before calling the callback > at the end of the function, else we have reentrancy issues. It could be put inside the do{ } while, but understood and that's fine.
The CQ bit was checked by jkarlin@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 one more time. Thanks! https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:720: in_do_loop_ = true; On 2017/03/22 17:16:06, Randy Smith (Not in Mondays) wrote: > On 2017/03/22 17:12:00, jkarlin wrote: > > On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > > > Thought (not even a suggestion): AutoReset? > > > > I think we want to explicitly set it back to false before calling the callback > > at the end of the function, else we have reentrancy issues. > > It could be put inside the do{ } while, but understood and that's fine. Good point. It's a bit of overhead but I like the safety. Done.
Still LGTM. https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/2767033003/diff/120001/net/http/http_cache_tr... net/http/http_cache_transaction.cc:720: in_do_loop_ = true; On 2017/03/22 17:32:55, jkarlin wrote: > On 2017/03/22 17:16:06, Randy Smith (Not in Mondays) wrote: > > On 2017/03/22 17:12:00, jkarlin wrote: > > > On 2017/03/22 15:42:12, Randy Smith (Not in Mondays) wrote: > > > > Thought (not even a suggestion): AutoReset? > > > > > > I think we want to explicitly set it back to false before calling the > callback > > > at the end of the function, else we have reentrancy issues. > > > > It could be put inside the do{ } while, but understood and that's fine. > > Good point. It's a bit of overhead but I like the safety. Done. The AutoReset class doesn't have any volatile funkiness, so I think there's a reasonable chance that the compiler will optimize the overhead away.
The CQ bit was unchecked by jkarlin@chromium.org
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490204296118120,
"parent_rev": "a97fcab9dfabffc950aa20b685208a92f1cacb8a", "commit_rev":
"5de449fdaae6cb58ce02aed512f383857ef6bcfe"}
Message was sent while issue was closed.
Description was changed from ========== [HttpCache::Transaction] Ensure each state only sets the next state once To aid in readability, ensure that the next state is only set once per state. This way the reader knows for sure what will happen after the state has been set. BUG=703923 ========== to ========== [HttpCache::Transaction] Ensure each state only sets the next state once To aid in readability, ensure that the next state is only set once per state. This way the reader knows for sure what will happen after the state has been set. BUG=703923 Review-Url: https://codereview.chromium.org/2767033003 Cr-Commit-Position: refs/heads/master@{#458840} Committed: https://chromium.googlesource.com/chromium/src/+/5de449fdaae6cb58ce02aed512f3... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5de449fdaae6cb58ce02aed512f3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
