Improve readability of HttpCache::Transaction::DoLoop |
||||
Issue descriptionThere are a few things we can do for DoLoop readability: 1) Require each Do* state to set the next state, no defaulting to STATE_NONE. 2) DCHECK that when next_state_ is set from within a Do* function that it hasn't already been set in this state. This way the reader knows for sure what the next state is. 3) Require next_state_ to be written to from within Do* functions and not helper functions.
,
Mar 22 2017
,
Mar 22 2017
I like your #2 idea as well. I presume that would be done with a setter function for the state that DCHECK'd internally.
,
Mar 22 2017
Yep, it's up and running here: https://codereview.chromium.org/2767033003/
,
Mar 22 2017
,
Mar 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fa6b5af00244a43fcfd8ca0bacd7f44e11d590b2 commit fa6b5af00244a43fcfd8ca0bacd7f44e11d590b2 Author: jkarlin <jkarlin@chromium.org> Date: Wed Mar 22 16:08:59 2017 This change forces states called by DoLoop to set the next state before returning. The real difference is that before there was a default next_state_ = STATE_NONE, and now the state must set that itself, which improves readability and ensures that the state machine isn't escaping unexpectedly. BUG= 703923 Review-Url: https://codereview.chromium.org/2766953002 Cr-Commit-Position: refs/heads/master@{#458763} [modify] https://crrev.com/fa6b5af00244a43fcfd8ca0bacd7f44e11d590b2/net/http/http_cache_transaction.cc [modify] https://crrev.com/fa6b5af00244a43fcfd8ca0bacd7f44e11d590b2/net/http/http_cache_transaction.h
,
Mar 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5de449fdaae6cb58ce02aed512f383857ef6bcfe commit 5de449fdaae6cb58ce02aed512f383857ef6bcfe Author: jkarlin <jkarlin@chromium.org> Date: Wed Mar 22 19:31:43 2017 [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} [modify] https://crrev.com/5de449fdaae6cb58ce02aed512f383857ef6bcfe/net/http/http_cache_transaction.cc [modify] https://crrev.com/5de449fdaae6cb58ce02aed512f383857ef6bcfe/net/http/http_cache_transaction.h
,
May 3 2017
Not going to get to #3, but considering the issue fixed. |
||||
►
Sign in to add a comment |
||||
Comment 1 by jkarlin@chromium.org
, Mar 22 2017