Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1315443003:
ServiceWorkerWriteToCacheJob: refactor (Closed)
|
Created:
5 years, 4 months ago by Elly Fong-Jones Modified:
5 years, 3 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorkerWriteToCacheJob is a subclass of URLRequestJob that is responsible for filling and updating a write-through cache for ServiceWorker data. Specifically, ServiceWorkerWriteToCacheJob creates an underlying URLRequestJob which it uses to do the actual network request for the script, then returns the script body to the creator of the ServiceWorkerWriteToCacheJob while also storing it in the ServiceWorker cache if it has been updated.
In order to avoid spurious cache rewrites, ServiceWorkerWriteToCacheJob defers writing an entry for as long as possible: if there is an existing entry, it compares incoming network data to the cache entry's data, and only starts writing once there is a mismatch. Since the mismatch may occur after much data has already been handled, once this happens ServiceWorkerWriteToCacheJob must transparently copy all the existing data up to that point from the cache, then resume streaming network data into the cache and back to its consumer. This is the "compare-and-copy" process. If there is no existing entry, network data is streamed directly to the cache; this is the "passthrough" process.
This CL removes the code implementing the compare-and-copy and passthrough processes from ServiceWorkerWriteToCacheJob and moves it into a separate class, ServiceWorkerCacheWriter. ServiceWorkerCacheWriter exposes an asynchronous API for overwriting a single cache entry that is now used by ServiceWorkerWriteToCacheJob.
BUG=474859
Committed: https://crrev.com/d778a5a82cdccab2b3cbf4940749753c98e59dcc
Cr-Commit-Position: refs/heads/master@{#349669}
Patch Set 1 #Patch Set 2 : first draft #
Total comments: 16
Patch Set 3 : First round of fixes #Patch Set 4 : StateMachine -> DoLoop #
Total comments: 14
Patch Set 5 : Second round #
Total comments: 52
Patch Set 6 : Next round. Fixed unit tests & ReadRawData structure. #
Total comments: 4
Patch Set 7 : Last nits for falken #
Total comments: 1
Patch Set 8 : Annotate virtual methods #
Total comments: 18
Patch Set 9 : Proper DoLoop pattern #
Total comments: 21
Patch Set 10 : Documentation fixes #Patch Set 11 : Fix bogus DCHECK #Patch Set 12 : CONTENT_EXPORT ServiceWorkerCacheWriter #Patch Set 13 : Pull in CONTENT_EXPORT header #Patch Set 14 : Fix broken include #Patch Set 15 : Fixes #Messages
Total messages: 48 (16 generated)
ellyjones@chromium.org changed reviewers: + falken@chromium.org, rdsmith@chromium.org
falken, rdsmith: PTAL? sorry for how big this is :(
Thanks so much for working on this refactor! I'm sorry the current code is pretty convoluted. Overall I think the rigid state machine makes it a lot safer than the current code which keeps tweaking state/status everywhere. I gave some high-level comments. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:244: int result = state_machine_.Run(net::OK); When trying to trace through this code I found it's a bit inconvenient to figure out what would actually be called here. Maybe it'd be convenient to comment or DCHECK saying we're in STATE_WRITE_HEADERS_FOR_PASSTHROUGH or STATE_WRITE_HEADERS_FOR_COPY or that WriteHeadersForPassthrough or WriteHeadersForCopy would be called? Or is that overkill... just an idea. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:324: // Premature EOF while reading the AppCache data to compare. Fail the I wouldn't call it AppCache data. Service worker happens to reuse the AppCache implementation code for storing http responses but they are better thought as two separate things. Service worker scripts are stored in the "service worker script cache" (incidentally, AppCache uses blockfile disk cache backend for this and service worker uses SimpleCache). Maybe just saying "data from disk" is best. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer_unittest.cc (right): https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:7: #include <cstring> Is this include needed? can it just be <string>? https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:129: memcpy(pending_buffer_->data(), expected.data, to_read); Some lightweight high-level comments for these classes and maybe functions would be useful. Reading this the first time I'm a bit confused why the function's assigning and copying stuff from "expected", usually I'd think a test just compares stuff from "expected". In other words it looks like the expectations affect the result, which is surprising. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:389: TEST_F(ServiceWorkerCacheWriterTest, CompareHeadersSync) { It's a little magical to me how we seemed to switch from Passthrough mode to Compare mode between the previous test and this test. Is the creation of ExpectReader() what causes the switch? As I said some comments may be nice for reading this file. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:429: const std::string& net_data2 = "ghijkl"; Should there be cache_data1 or net_data1? https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:170: if (did_notify_finished_) After calling NotifyFinishedCaching, this will always be true. Is it correct to call NotifyFinishedCaching here? In the current code, the consumer_ may be PassThroughConsumer or Comparer. If it's the Comparer, it might need to start reading headers from disk asynchronously, in which case it sets is_io_pending(). https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:354: net::Error error = cache_writer_->WriteHeaders( Does this actually write headers? The current code avoids writing to disk until there's a change. Maybe the name can be tweaked if this doesn't write headers.
falken: PTAL? https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:244: int result = state_machine_.Run(net::OK); On 2015/08/28 05:45:24, falken wrote: > When trying to trace through this code I found it's a bit inconvenient to figure > out what would actually be called here. Maybe it'd be convenient to comment or > DCHECK saying we're in STATE_WRITE_HEADERS_FOR_PASSTHROUGH or > STATE_WRITE_HEADERS_FOR_COPY or that WriteHeadersForPassthrough or > WriteHeadersForCopy would be called? Or is that overkill... just an idea. We're actually in STATE_START here. I've added a DCHECK_EQ to that effect. From STATE_START we move to either WRITE_HEADERS_FOR_PASSTHROUGH or READ_HEADERS_FOR_COMPARE depending whether we have an existing reader to compare against. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:324: // Premature EOF while reading the AppCache data to compare. Fail the On 2015/08/28 05:45:24, falken wrote: > I wouldn't call it AppCache data. Service worker happens to reuse the AppCache > implementation code for storing http responses but they are better thought as > two separate things. Service worker scripts are stored in the "service worker > script cache" (incidentally, AppCache uses blockfile disk cache backend for this > and service worker uses SimpleCache). Maybe just saying "data from disk" is > best. Done. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer_unittest.cc (right): https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:7: #include <cstring> On 2015/08/28 05:45:24, falken wrote: > Is this include needed? can it just be <string>? Done. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:129: memcpy(pending_buffer_->data(), expected.data, to_read); On 2015/08/28 05:45:24, falken wrote: > Some lightweight high-level comments for these classes and maybe functions would > be useful. Reading this the first time I'm a bit confused why the function's > assigning and copying stuff from "expected", usually I'd think a test just > compares stuff from "expected". In other words it looks like the expectations > affect the result, which is surprising. Done. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:389: TEST_F(ServiceWorkerCacheWriterTest, CompareHeadersSync) { On 2015/08/28 05:45:24, falken wrote: > It's a little magical to me how we seemed to switch from Passthrough mode to > Compare mode between the previous test and this test. Is the creation of > ExpectReader() what causes the switch? As I said some comments may be nice for > reading this file. Done. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:429: const std::string& net_data2 = "ghijkl"; On 2015/08/28 05:45:24, falken wrote: > Should there be cache_data1 or net_data1? No; they both use data1 as a shared prefix. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:170: if (did_notify_finished_) On 2015/08/28 05:45:24, falken wrote: > After calling NotifyFinishedCaching, this will always be true. > > Is it correct to call NotifyFinishedCaching here? In the current code, the > consumer_ may be PassThroughConsumer or Comparer. If it's the Comparer, it might > need to start reading headers from disk asynchronously, in which case it sets > is_io_pending(). You are correct, this code is very wrong. It should only call NotifyFinishedCaching if !did_notify_finished_ and !is_io_pending(), since that means a synchronous EOF (errors are handled by the earlier !is_success() clause). I'll add a comment about this logic. Also, this is missing an EOF HandleNetData() call. https://codereview.chromium.org/1315443003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:354: net::Error error = cache_writer_->WriteHeaders( On 2015/08/28 05:45:24, falken wrote: > Does this actually write headers? The current code avoids writing to disk until > there's a change. Maybe the name can be tweaked if this doesn't write headers. I renamed these methods to MaybeWriteHeaders and MaybeWriteData.
After discussion on net-dev, I changed the StateMachine pattern back to the more conventional DoLoop pattern in ServiceWorkerCacheWriter.
This looks good! some more comments. I assume "TODO: Update_EmptyScript and Update_SameScript fail" has to be fixed in the final patchset. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:66: bool done_; done_ seems not used? It's set in WrappedCallback but seems done() is never called. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:175: size_t ServiceWorkerCacheWriter::BytesWritten() const { nit: better as bytes_written()? https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:247: // Compare the data from AppCache to the data from the network. AppCache -> the service worker script cache (or just disk) https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:25: // This class is responsible for possibly updating the ServiceWorker cache for I'd write this out as "service worker script cache". The "service worker cache" used to mean the "Cache Storage API", and there's still confusion when saying "cache" when talking about service workers. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer_unittest.cc (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:35: // there are any expected reads that have not yet happened. Thanks for these explanatory comments! https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:516: EXPECT_EQ(net::OK, error); should this also test AllExpectedDone() and BytesWritten? Same comment for all tests below. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:454: LOG(ERROR) << "Reader: " << incumbent_response_id_; debugging (and below)?
PTAL? https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:66: bool done_; On 2015/09/01 08:06:37, falken wrote: > done_ seems not used? It's set in WrappedCallback but seems done() is never > called. Good catch. Instead client code uses result() directly. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:175: size_t ServiceWorkerCacheWriter::BytesWritten() const { On 2015/09/01 08:06:37, falken wrote: > nit: better as bytes_written()? Done. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:247: // Compare the data from AppCache to the data from the network. On 2015/09/01 08:06:37, falken wrote: > AppCache -> the service worker script cache (or just disk) Done. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:25: // This class is responsible for possibly updating the ServiceWorker cache for On 2015/09/01 08:06:37, falken wrote: > I'd write this out as "service worker script cache". The "service worker cache" > used to mean the "Cache Storage API", and there's still confusion when saying > "cache" when talking about service workers. Done. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer_unittest.cc (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:35: // there are any expected reads that have not yet happened. On 2015/09/01 08:06:37, falken wrote: > Thanks for these explanatory comments! No problem. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_cache_writer_unittest.cc:516: EXPECT_EQ(net::OK, error); On 2015/09/01 08:06:37, falken wrote: > should this also test AllExpectedDone() and BytesWritten? Same comment for all > tests below. Done. https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:454: LOG(ERROR) << "Reader: " << incumbent_response_id_; On 2015/09/01 08:06:37, falken wrote: > debugging (and below)? Done.
Yep this looks good to me, I just have optional nits now. I think some changes will have to be made to get the unittests to pass, should I wait for that before the official lg-tm? Update_EmptyScript and Update_SameScript failing suggests there's some error where if the script didn't change the updated version still runs and tries to install itself (that's not good). FYI I had to add some CONTENT_EXPORT in ServiceWorkerCacheWriter to get the patch to build. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:6: need <algorithm> for std::min (according to git-cl lint) https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:43: AsyncOnlyCompletionCallbackAdaptor(const net::CompletionCallback& callback) nit: explicit https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:175: *next_state = STATE_WRITE_HEADERS_FOR_PASSTHROUGH; nit: I prefer braces for multi-line bodies https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:63: // ERR_IO_PENDING and does not invoke |callback|. nit: maybe nice for the comment to explain why it's "MaybeWrite" https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:70: // other than ERR_IO_PENDING and does not invoke |callback|. ditto https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:7: #include <algorithm> I think this can be removed now https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:24: #include "net/http/http_util.h" this one seems not used now either (not sure it ever was) https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:173: nit: extra newline? https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:368: } nit: i'd remove the braces for consistency with the rest of the file
First round of comments. I'm focussing on the high level async structure and DoLoop idiomaticity (matching what we have in the net stack). https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:17: enum { Include some comments to indicate what the state machine looks like? You can just break the enums out into blocks that are linear, then annotate with the branches between the linear aspects. (I have a GraphViz file for the state machine if you want it.) It'd also be really useful to document the general flow of the state machine, something like "Read headers and data from the cache to compare with incoming values. If this results in a miscompare, re-read the headers from the cache and write them back out [? Why is this necessary?], then copy out the network data that's been written to this class, then go into passthrough mode copying anything written to this class." https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:75: status = Start(&next_state, &pause, status); The standard DoLoop idiom is for the function corresponding to STATE_<STATE> to be named Do<State>. From a selfish POV, it makes it easier for a reviewer to figure out what the sub-functions of DoLoop are :-}. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:133: } while (status >= net::OK && state_ != STATE_DONE && !pause); Hmmm. |pause| is not the standard DoLoop() idiom (at least, I've never seen it before--if you can give me pointers to examples that would calm me down fairly quickly), and I'm a bit reluctant to see it added here. DoLoop() is already complicated enough without having variations on it in different places (yeah, I'm sure there already are, but we want to minimize that for maintainability). As I understand it, this idiomlet is here because you want to leave the state machine in a particular state on return and let it be driven from that state by a new consumer call. Most other places I've seen, specific consumer calls imply specific states (i.e. you set the state in the call and then call DoLoop()); the reason you can't do that here (maybe with some DCHECKs to make sure that appropriate things have already happened) is that you might be in either compare or passthrough mode; there aren't any other states that you'll want to pause in for consumer call driving. So that could be taken care of by a single boolean in the class, allowing us to keep the DoLoop idiom consistent. (Side note: The strong impression I get doing the DoLoop doc CL is that DoLoop is *not* intended as a general state machine implementation. Instead, it's intended to handle code which would ideally, from a maintainability perspective, be written in a single threaded blocking fashion, possibly with branches. Everywhere the code needs to pause for async IO return, it has a state transition and returns ERR_IO_PENDING. "Returning" (either synchronously or via callback) from consumer initiated operations implies that the operation is completed (there's no way to block for more consumer input). Presuming this interpretation of mine is accurate, I'm very wary of adding the "pause" flag, which isn't a very big code change, and *is* a very big philosophy change. Those changes are the worst kind.) https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:175: *next_state = STATE_WRITE_HEADERS_FOR_PASSTHROUGH; On 2015/09/01 14:43:52, falken wrote: > nit: I prefer braces for multi-line bodies I think this is required by the style guide. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:189: int ServiceWorkerCacheWriter::ReadHeadersForCompareDone(int* next_state, I like this idiom better than the normal DoLoop() idiom, but I still think consistency is better than proliferation. so I think you should change it to having a class internal next_state_ variable. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:194: return static_cast<int>(result); Why the static cast? It was and is an int. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:39: using OnWriteCompleteCallback = base::Callback<void(net::Error)>; Why using rather than typedef? https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:45: // writers to entries that won't be updated, and because this class may need What's the cost of creating writers to entries that won't be updated? (No need to update the comment, I'm just curious.) https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:89: // the requested operation failed synchronously. Based on my documentation CL, I think the usual state machine idiom is that when DoLoop() returns something other than ERR_IO_PENDING, the internal state is STATE_NONE (or STATE_DONE), and that consumer calls (MaybeWriteHeaders and MaybeWriteData in this case) set the state that they want to drive the machine from. That isn't the idiom here (and probably shouldn't be, given that writing data, at least, may hit a SM in compare mode or passthrough mode) but I think it's pretty important to document the states which the state machine may pause in waiting for a MaybeWriteHeaders/MaybeWriteData inputs. And I'd really like DCHECKs for that in MWH/MWD. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:93: // details on these. I think especially for state machine implementations, the ordering of the methods in the .cc file and the .h file should match. I'd also suggest that all the state machine methods are in a block together (as they are here, but I don't think they are in the .cc file). https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:88: base::Unretained(this)), I think it's a good idea for Unretained() wrappers to always have a comment next to them about why that's safe in terms of object lifetimes. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:172: // this update is complete if the cache writer finishes synchronously. I'd suggest you put in a note that HandleNetData() may result in asynchronous behavior, thus the second io_pending() check below (comment could be there if you wanted). https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:231: if (net_bytes_read != 0) { I didn't actually do the code transformation, so I might be wrong, but isn't this avoiding doing the call to HandleNetData() that is then done in the caller? Any reason not just to have the code in one place? https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.h (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.h:25: class ServiceWorkerResponseWriter; This doesn't look necessary to me, since it's declared in service_worker_disk_cache.h. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.h:86: void OnWriteHeadersComplete(net::Error error); I know it's in the private section, and hence part of the implementation, but I think it improves maintainability a bit if you have a little bit of comment documentation on the new added methods. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.h:115: scoped_ptr<ServiceWorkerResponseReader> CreateResponseReader(); nit, suggestion: Given that the response reader and writer are specifically for interaction with the disk cache, including "Cache" in their names?
PTAL? https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:6: On 2015/09/01 14:43:52, falken wrote: > need <algorithm> for std::min (according to git-cl lint) Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:17: enum { On 2015/09/01 20:35:42, rdsmith wrote: > Include some comments to indicate what the state machine looks like? You can > just break the enums out into blocks that are linear, then annotate with the > branches between the linear aspects. (I have a GraphViz file for the state > machine if you want it.) > > It'd also be really useful to document the general flow of the state machine, > something like "Read headers and data from the cache to compare with incoming > values. If this results in a miscompare, re-read the headers from the cache and > write them back out [? Why is this necessary?], then copy out the network data > that's been written to this class, then go into passthrough mode copying > anything written to this class." Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:43: AsyncOnlyCompletionCallbackAdaptor(const net::CompletionCallback& callback) On 2015/09/01 14:43:52, falken wrote: > nit: explicit Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:75: status = Start(&next_state, &pause, status); On 2015/09/01 20:35:42, rdsmith wrote: > The standard DoLoop idiom is for the function corresponding to STATE_<STATE> to > be named Do<State>. From a selfish POV, it makes it easier for a reviewer to > figure out what the sub-functions of DoLoop are :-}. Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:133: } while (status >= net::OK && state_ != STATE_DONE && !pause); On 2015/09/01 20:35:42, rdsmith wrote: > Hmmm. |pause| is not the standard DoLoop() idiom (at least, I've never seen it > before--if you can give me pointers to examples that would calm me down fairly > quickly), and I'm a bit reluctant to see it added here. DoLoop() is already > complicated enough without having variations on it in different places (yeah, > I'm sure there already are, but we want to minimize that for maintainability). > > As I understand it, this idiomlet is here because you want to leave the state > machine in a particular state on return and let it be driven from that state by > a new consumer call. Most other places I've seen, specific consumer calls imply > specific states (i.e. you set the state in the call and then call DoLoop()); the > reason you can't do that here (maybe with some DCHECKs to make sure that > appropriate things have already happened) is that you might be in either compare > or passthrough mode; there aren't any other states that you'll want to pause in > for consumer call driving. So that could be taken care of by a single boolean > in the class, allowing us to keep the DoLoop idiom consistent. Are you proposing deleting the pause variable and the concept of pause altogether? The reason it exists right now is that the state machine needs to "succeed" multiple times, and so there has to be a way to differentiate "success, and keep going" from "success, and return that result". The only other option I could think of was to store the knowledge of which states pause in DoLoop itself, but one of them (ReadDataForCompareDone) only pauses under somewhat complicated circumstances (basically, it only pauses if it has compared the entire block of input data already), so I don't think I can do that. I can move the pause variable to be a member of the class I guess, but I don't like that it makes this class's DoLoop *look* similar to other uses of DoLoop while having different semantics. I think that things with different behavior should look different. > (Side note: The strong impression I get doing the DoLoop doc CL is that DoLoop > is *not* intended as a general state machine implementation. Instead, it's > intended to handle code which would ideally, from a maintainability perspective, > be written in a single threaded blocking fashion, possibly with branches. > Everywhere the code needs to pause for async IO return, it has a state > transition and returns ERR_IO_PENDING. "Returning" (either synchronously or via > callback) from consumer initiated operations implies that the operation is > completed (there's no way to block for more consumer input). Presuming this > interpretation of mine is accurate, I'm very wary of adding the "pause" flag, > which isn't a very big code change, and *is* a very big philosophy change. > Those changes are the worst kind.) I agree; the usual way DoLoop is used is when there is a single "result", which is returned to the caller via a callback or something when the machine hits DONE. This use of it has multiple results that are generated in sequence, hence the pause idiom. I don't know how to escape this without moving away from using DoLoop. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:175: *next_state = STATE_WRITE_HEADERS_FOR_PASSTHROUGH; On 2015/09/01 14:43:52, falken wrote: > nit: I prefer braces for multi-line bodies Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:175: *next_state = STATE_WRITE_HEADERS_FOR_PASSTHROUGH; On 2015/09/01 20:35:42, rdsmith wrote: > On 2015/09/01 14:43:52, falken wrote: > > nit: I prefer braces for multi-line bodies > > I think this is required by the style guide. Acknowledged. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:189: int ServiceWorkerCacheWriter::ReadHeadersForCompareDone(int* next_state, On 2015/09/01 20:35:42, rdsmith wrote: > I like this idiom better than the normal DoLoop() idiom, but I still think > consistency is better than proliferation. so I think you should change it to > having a class internal next_state_ variable. Repeating my concern from above: since this DoLoop has different behavior from the ones elsewhere in the network stack, I think it should look different as well. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.cc:194: return static_cast<int>(result); On 2015/09/01 20:35:42, rdsmith wrote: > Why the static cast? It was and is an int. Leftover from earlier. Done and removed elsewhere. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:39: using OnWriteCompleteCallback = base::Callback<void(net::Error)>; On 2015/09/01 20:35:42, rdsmith wrote: > Why using rather than typedef? They are recommended instead of typedef here: http://chromium-cpp.appspot.com/ https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:45: // writers to entries that won't be updated, and because this class may need On 2015/09/01 20:35:42, rdsmith wrote: > What's the cost of creating writers to entries that won't be updated? (No need > to update the comment, I'm just curious.) This isn't done because of cost, it's done because I don't know (and the AppCache documentation doesn't guarantee) that it's safe to create a writer for an entry you don't actually intend to write. I am not positive that it doesn't evict an old version or something similar. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:63: // ERR_IO_PENDING and does not invoke |callback|. On 2015/09/01 14:43:52, falken wrote: > nit: maybe nice for the comment to explain why it's "MaybeWrite" Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:70: // other than ERR_IO_PENDING and does not invoke |callback|. On 2015/09/01 14:43:52, falken wrote: > ditto Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:89: // the requested operation failed synchronously. On 2015/09/01 20:35:42, rdsmith wrote: > Based on my documentation CL, I think the usual state machine idiom is that when > DoLoop() returns something other than ERR_IO_PENDING, the internal state is > STATE_NONE (or STATE_DONE), and that consumer calls (MaybeWriteHeaders and > MaybeWriteData in this case) set the state that they want to drive the machine > from. That isn't the idiom here (and probably shouldn't be, given that writing > data, at least, may hit a SM in compare mode or passthrough mode) but I think > it's pretty important to document the states which the state machine may pause > in waiting for a MaybeWriteHeaders/MaybeWriteData inputs. And I'd really like > DCHECKs for that in MWH/MWD. I've added these DCHECKs in MaybeWriteHeaders and MaybeWriteData, but I don't want to document the 6 allowed pause states for MaybeWriteHeaders and the 10 allowed pause states for MaybeWriteData in another place, especially a place that won't cause a crash when it's wrong. I've added a pointer to look at the DCHECKs in those functions instead. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_cache_writer.h:93: // details on these. On 2015/09/01 20:35:42, rdsmith wrote: > I think especially for state machine implementations, the ordering of the > methods in the .cc file and the .h file should match. I'd also suggest that all > the state machine methods are in a block together (as they are here, but I don't > think they are in the .cc file). Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:7: #include <algorithm> On 2015/09/01 14:43:52, falken wrote: > I think this can be removed now Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:24: #include "net/http/http_util.h" On 2015/09/01 14:43:53, falken wrote: > this one seems not used now either (not sure it ever was) Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:88: base::Unretained(this)), On 2015/09/01 20:35:42, rdsmith wrote: > I think it's a good idea for Unretained() wrappers to always have a comment next > to them about why that's safe in terms of object lifetimes. Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:172: // this update is complete if the cache writer finishes synchronously. On 2015/09/01 20:35:42, rdsmith wrote: > I'd suggest you put in a note that HandleNetData() may result in asynchronous > behavior, thus the second io_pending() check below (comment could be there if > you wanted). Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:173: On 2015/09/01 14:43:52, falken wrote: > nit: extra newline? Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:231: if (net_bytes_read != 0) { On 2015/09/01 20:35:42, rdsmith wrote: > I didn't actually do the code transformation, so I might be wrong, but isn't > this avoiding doing the call to HandleNetData() that is then done in the caller? > Any reason not just to have the code in one place? I've redone HandleNetData and ReadRawData completely. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.cc:368: } On 2015/09/01 14:43:52, falken wrote: > nit: i'd remove the braces for consistency with the rest of the file Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_write_to_cache_job.h (right): https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.h:25: class ServiceWorkerResponseWriter; On 2015/09/01 20:35:42, rdsmith wrote: > This doesn't look necessary to me, since it's declared in > service_worker_disk_cache.h. Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.h:86: void OnWriteHeadersComplete(net::Error error); On 2015/09/01 20:35:42, rdsmith wrote: > I know it's in the private section, and hence part of the implementation, but I > think it improves maintainability a bit if you have a little bit of comment > documentation on the new added methods. Done. https://codereview.chromium.org/1315443003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_write_to_cache_job.h:115: scoped_ptr<ServiceWorkerResponseReader> CreateResponseReader(); On 2015/09/01 20:35:42, rdsmith wrote: > nit, suggestion: Given that the response reader and writer are specifically for > interaction with the disk cache, including "Cache" in their names? Done.
lgtm https://codereview.chromium.org/1315443003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:163: I liked the DCHECK_EQ(0, *bytes_read) or the comment that here we are at EOF. It helps me understand what the state is at this point. https://codereview.chromium.org/1315443003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:416: int size = -1; nit: |size| could be moved to just about where it's used
https://codereview.chromium.org/1315443003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:163: On 2015/09/10 07:16:06, falken wrote: > I liked the DCHECK_EQ(0, *bytes_read) or the comment that here we are at EOF. It > helps me understand what the state is at this point. We aren't any more though, because ReadNetData no longer automatically calls HandleNetData for nonzero-length reads. https://codereview.chromium.org/1315443003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:416: int size = -1; On 2015/09/10 07:16:06, falken wrote: > nit: |size| could be moved to just about where it's used Done.
ellyjones@chromium.org changed reviewers: + michaeln@chromium.org
michaeln: ptal at appcache_response.h change?
lgtm (w the comments) https://codereview.chromium.org/1315443003/diff/120001/content/browser/appcac... File content/browser/appcache/appcache_response.h (right): https://codereview.chromium.org/1315443003/diff/120001/content/browser/appcac... content/browser/appcache/appcache_response.h:211: // Should only be called where there is no Write operation in progress. please put a comment here that says // (virtual for testing) similar to those on ReadInfo and ReadData
Elly: My apologies for not getting back to you more quickly on this CL; as you
can probably imagine, this week was a little frantic. I figure I shouldn't
compound my error by holding off responding to the high-level DoLoop() idiom
question until I've done a thorough review of the CL; I'll address that issue
now and keep going with what I can review of the CL while we figure out how to
resolve it.
My attitude is that we shouldn't use the name "DoLoop()" if we're not in basic
rough outline following the DoLoop() idiom, and if there's a way to follow the
DoLoop() idiom without *too* much in the way of contortions, we should do it
even if there's a better, but not commonly used, idiom. There are two value
judgements in that ("basic[ally] following" and "too much") that I want to be
careful of--I could easily imagine that I'm going all pedantic and
prescriptivist because of my documentation CL. For that reason, if you get Matt
or Sleevi to opine that this is fine, I'll yield, and if you want, I'm happy to
raise the issue with them. But at the moment I don't think this follows the
idiom closely enough (though it's subtle), and I don't think changing it would
be too hard.
1) Doesn't follow the idiom closely enough. The thing that David turned me on
to recently was how much the DoLoop() idiom held to the Chromium principle of
writing single threaded code; it just turns things that would conceptually look
like blocking into callouts and callback into DoLoop(). What this code is is an
honest state machine, where one of the "events" that can occur is the user
calling in to drive the state machine to the next state. You can't assume that
the state machine is in "neutral" while no callback is outstanding, so you can't
think of the DoLoop() as implementing (e.g.) MaybeWriteHeaders() or
MaybeWriteData() as if it was just blocking for subordinate I/O.
2) Wouldn't be too hard to change. I'm not saying that the *class* shouldn't
keep state between separate calls to MaybeWrite*(), I'm saying that it shouldn't
be kept in the state_ variable. The DoLoop() idiom, as I understand it, has the
consumer interface choosing the starting state of the state diagram when the
call comes in, and then calling DoLoop(); DoLoop() drives (possibly with async
callbacks) until the consumer interface call is complete, and then returns the
data (sync or async) through the consumer interface call, resetting the state
machine. So if we had a bool (or enum) for whether we were in passthrough,
copy, or compare mode, and had the data received so far (or read so far from the
cache--sorry, I'm not remembering which way the machine waits), we could choose
the correct entry state and keep the idiom.
This is definitely worse than what you have in terms of complexity of this
class--instead of the state being just in one state variable, you have a state
machine that's something of a subroutine that's driven in different ways by the
consumer interface depending on previous behavior. But I think overall it's
better because it keeps the core idiom more consistent, and hence more
understandable.
My apologies for pushing you to write what's arguably less tight code--let me
know if you want me to check in with the experts as to whether I'm being
appropriate pure or prescriptively pedantic.
https://codereview.chromium.org/1315443003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_cache_writer.h (right):
https://codereview.chromium.org/1315443003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_cache_writer.h:39: using
OnWriteCompleteCallback = base::Callback<void(net::Error)>;
On 2015/09/09 13:36:08, Elly Jones wrote:
> On 2015/09/01 20:35:42, rdsmith wrote:
> > Why using rather than typedef?
>
> They are recommended instead of typedef here: http://chromium-cpp.appspot.com/
Huh. Learn something new every day. Thanks for the pointer.
https://codereview.chromium.org/1315443003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_cache_writer.h:45: // writers to
entries that won't be updated, and because this class may need
On 2015/09/09 13:36:08, Elly Jones wrote:
> On 2015/09/01 20:35:42, rdsmith wrote:
> > What's the cost of creating writers to entries that won't be updated? (No
> need
> > to update the comment, I'm just curious.)
>
> This isn't done because of cost, it's done because I don't know (and the
> AppCache documentation doesn't guarantee) that it's safe to create a writer
for
> an entry you don't actually intend to write. I am not positive that it doesn't
> evict an old version or something similar.
Oy vais. Ok.
https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_cache_writer.cc (right):
https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_cache_writer.cc:30: // data.
This is an awesome explanatory comment, with a goldilocks level of detail (IMO).
Thank you.
https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_cache_writer.h (right):
https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_cache_writer.h:138: // Callback
used by the above helpers.
Suggestion: Include a sentence describing what this does/how it's different from
DoLoop (E.g. "Calls DoLoop(); if it returns synchronously, reports the result
back to the original caller via pending_callback_"); it's implementation
description, but I think the comment will help folks reading the header file
understand how the routines are used.
https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_cache_writer.h:141: int state_;
I'd vote in favor of leaving the enum in the header file so that this variable
could be of more specific type (and I think it's the common pattern, though feel
free to correct me if I'm wrong).
Some comments on ServiceWorkerWriteToCacheJob (not doing that as thoroughly, as I don't consider myself even a little bit of an expert). https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache.h (right): https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache.h:48: friend class MockServiceWorkerResponseWriter; I'm confused. What are the friend decls for? There doesn't seem to be anything private about this class that the friend decl gives "Mock*" access to. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:84: // |cache_writer_|, which in turn is the sole owner of these callbacks. nit: "owner"->"user" (or maybe "accessor"). The callbacks themselves aren't owned, or are owned by this class since they're part of this class and attached to an instance of this class. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:160: AsyncNotifyDoneHelper(status, kFetchScriptError); nit, suggestion: It's weird reading through the synchronous call and seeing a call to "Async..."; explanatory comment? (I assume that the only path we have to notify status is through SetStatus(), but it's still weird to see "Async...") https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:164: HandleNetData(*bytes_read); I think it would be valuable to somewhere (here, point of declaration, point of definition, reddit, 4chan?) to have a comment specifying the interface contract for this call, given that my memory is that it's somewhat complex. I'm happy to go with your preference for location, but I'll throw out the suggestion of a quick comment at the two points of call, and a longer and more pedantic one at function definition. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:165: return GetStatus().status() == net::URLRequestStatus::SUCCESS; nit, suggestion: Worthwhile having a comment as to why this isn't .is_success()? https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:346: void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(net::Error error) { nit, suggestion: Interchange the order of this function and the next function so that the order in source file matches the order of execution (also in header file).
This now uses the canonical form of the DoLoop pattern. rdsmith: PTAL? https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:30: // data. On 2015/09/13 20:56:36, rdsmith wrote: > This is an awesome explanatory comment, with a goldilocks level of detail (IMO). > Thank you. Acknowledged. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:138: // Callback used by the above helpers. On 2015/09/13 20:56:36, rdsmith wrote: > Suggestion: Include a sentence describing what this does/how it's different from > DoLoop (E.g. "Calls DoLoop(); if it returns synchronously, reports the result > back to the original caller via pending_callback_"); it's implementation > description, but I think the comment will help folks reading the header file > understand how the routines are used. Done. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:141: int state_; On 2015/09/13 20:56:36, rdsmith wrote: > I'd vote in favor of leaving the enum in the header file so that this variable > could be of more specific type (and I think it's the common pattern, though feel > free to correct me if I'm wrong). Done. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache.h (right): https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache.h:48: friend class MockServiceWorkerResponseWriter; On 2015/09/13 21:32:07, rdsmith wrote: > I'm confused. What are the friend decls for? There doesn't seem to be anything > private about this class that the friend decl gives "Mock*" access to. These are left over from earlier. Deleted. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:84: // |cache_writer_|, which in turn is the sole owner of these callbacks. On 2015/09/13 21:32:08, rdsmith wrote: > nit: "owner"->"user" (or maybe "accessor"). The callbacks themselves aren't > owned, or are owned by this class since they're part of this class and attached > to an instance of this class. Done. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:160: AsyncNotifyDoneHelper(status, kFetchScriptError); On 2015/09/13 21:32:08, rdsmith wrote: > nit, suggestion: It's weird reading through the synchronous call and seeing a > call to "Async..."; explanatory comment? (I assume that the only path we have > to notify status is through SetStatus(), but it's still weird to see "Async...") AsyncNotifyDoneHelper isn't actually Async in any sense now, so I'll rename it. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:164: HandleNetData(*bytes_read); On 2015/09/13 21:32:07, rdsmith wrote: > I think it would be valuable to somewhere (here, point of declaration, point of > definition, reddit, 4chan?) to have a comment specifying the interface contract > for this call, given that my memory is that it's somewhat complex. I'm happy to > go with your preference for location, but I'll throw out the suggestion of a > quick comment at the two points of call, and a longer and more pedantic one at > function definition. Done. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:165: return GetStatus().status() == net::URLRequestStatus::SUCCESS; On 2015/09/13 21:32:07, rdsmith wrote: > nit, suggestion: Worthwhile having a comment as to why this isn't .is_success()? Done. https://codereview.chromium.org/1315443003/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:346: void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(net::Error error) { On 2015/09/13 21:32:07, rdsmith wrote: > nit, suggestion: Interchange the order of this function and the next function so > that the order in source file matches the order of execution (also in header > file). Done.
Mostly nits; this is fairly close. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:107: default: Under what circumstances should this happen? Should we have a NOTREACHED()? (A la HttpStreamParser.) https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:111: } while (status >= net::OK && state_ != STATE_DONE); In other code I've read, I think of "STATE_NONE" as meaning "state machine ready to be re-driven if needed" and STATE_DONE as meaning "all tapped out, not good for anything ever again". This idiom doesn't strike me as key to the DoLoop(), and it's a bit contradictory to the earlier comment (which was basically saying that the DoLoop() mechanism shouldn't keep state after an external method invocation had completed), so I'm not committed to the standard pattern. But I would like a comment on the enum about STATE_DONE if you want to use it this way, just so people don't assume it means *done* done. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:139: // Synchronous errors always go to ERR_IO_PENDING. I don't understand this comment? https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:143: // ERR_IO_PENDING has to have one of the "done" states as the next state. nit, suggestion: Make clear you're not talking about STATE_DONE here? https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:144: if (result == net::ERR_IO_PENDING) { nit, random thought (i.e. not even a suggestion, feel free to ignore): If you include the ERR_IO_PENDING conditional in the DCHECK, any error messages/crashes from it will make it clear the failure (returned IO_PENDING without waiting on IO). Applies elsewhere as well (if you decide to take it). https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:98: // cached data. Worthwhile adding a comment about transitioning to done and back into the SM at end/beginning of each consumer call execution? https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:135: // b) One of the state functions pauses the machine Comment no longer valid, I don't think. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:143: int DoLoop(int result); Suggestion, up to you: My understanding from other comments in this file is that the only information passed back to the caller is an Error return (possibly net::OK). My understanding of the DoLoop() idiom is that the return value from DoLoop() is intended to be passed back to the caller. Would it be reasonable to make the return value more specifically an Error return and change the logic underneath DoLoop() to just pass around Errors? https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:190: bool io_pending_; If I read the code correctly, this variable is only used for assertions; the actually determination of io_pending_ is based on the state_ variable (as it should be based on my understanding of the code :-}). I wince a bit at seeing variables that are only used for assertions, just because that can change, resulting in an invisible jump in code complexity. Willing to put in a comment about this being only used for assertion and it's semi-equivalence to state_ != DONE && not in synchronous DoLoop() invocation?
PTAL? https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:107: default: On 2015/09/16 18:57:28, rdsmith wrote: > Under what circumstances should this happen? Should we have a NOTREACHED()? (A > la HttpStreamParser.) Done. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:111: } while (status >= net::OK && state_ != STATE_DONE); On 2015/09/16 18:57:28, rdsmith wrote: > In other code I've read, I think of "STATE_NONE" as meaning "state machine ready > to be re-driven if needed" and STATE_DONE as meaning "all tapped out, not good > for anything ever again". This idiom doesn't strike me as key to the DoLoop(), > and it's a bit contradictory to the earlier comment (which was basically saying > that the DoLoop() mechanism shouldn't keep state after an external method > invocation had completed), so I'm not committed to the standard pattern. But I > would like a comment on the enum about STATE_DONE if you want to use it this > way, just so people don't assume it means *done* done. Done. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:139: // Synchronous errors always go to ERR_IO_PENDING. On 2015/09/16 18:57:28, rdsmith wrote: > I don't understand this comment? That's because it makes absolutely no sense. Fixed. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:143: // ERR_IO_PENDING has to have one of the "done" states as the next state. On 2015/09/16 18:57:28, rdsmith wrote: > nit, suggestion: Make clear you're not talking about STATE_DONE here? Done. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:144: if (result == net::ERR_IO_PENDING) { On 2015/09/16 18:57:28, rdsmith wrote: > nit, random thought (i.e. not even a suggestion, feel free to ignore): If you > include the ERR_IO_PENDING conditional in the DCHECK, any error messages/crashes > from it will make it clear the failure (returned IO_PENDING without waiting on > IO). Applies elsewhere as well (if you decide to take it). I think it's already clear from getting this message in the first place? Maybe I have misunderstood. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:98: // cached data. On 2015/09/16 18:57:28, rdsmith wrote: > Worthwhile adding a comment about transitioning to done and back into the SM at > end/beginning of each consumer call execution? Done. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:135: // b) One of the state functions pauses the machine On 2015/09/16 18:57:28, rdsmith wrote: > Comment no longer valid, I don't think. Done. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:143: int DoLoop(int result); On 2015/09/16 18:57:28, rdsmith wrote: > Suggestion, up to you: My understanding from other comments in this file is that > the only information passed back to the caller is an Error return (possibly > net::OK). My understanding of the DoLoop() idiom is that the return value from > DoLoop() is intended to be passed back to the caller. Would it be reasonable to > make the return value more specifically an Error return and change the logic > underneath DoLoop() to just pass around Errors? I would rather not deviate from the idiom at all if I'm going to use the idiom, so I'll leave this as an int. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:190: bool io_pending_; On 2015/09/16 18:57:28, rdsmith wrote: > If I read the code correctly, this variable is only used for assertions; the > actually determination of io_pending_ is based on the state_ variable (as it > should be based on my understanding of the code :-}). I wince a bit at seeing > variables that are only used for assertions, just because that can change, > resulting in an invisible jump in code complexity. Willing to put in a comment > about this being only used for assertion and it's semi-equivalence to state_ != > DONE && not in synchronous DoLoop() invocation? Done. I sort of wish we had a naming convention for variables like this, like "bool debug_io_pending_" or "bool io_pending_assert_only_" or something, to indicate that it's being used exclusively as a sanity-check. I would #ifndef NDEBUG around it, but that looks gross in the code. Oh well.
LGTM; thanks for putting up with my nits. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.cc (right): https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.cc:144: if (result == net::ERR_IO_PENDING) { On 2015/09/16 19:25:57, Elly Jones wrote: > On 2015/09/16 18:57:28, rdsmith wrote: > > nit, random thought (i.e. not even a suggestion, feel free to ignore): If you > > include the ERR_IO_PENDING conditional in the DCHECK, any error > messages/crashes > > from it will make it clear the failure (returned IO_PENDING without waiting on > > IO). Applies elsewhere as well (if you decide to take it). > > I think it's already clear from getting this message in the first place? Maybe I > have misunderstood. It's targeting the folks who have a vague idea of how this code works before they actually look at the lines of code; the "Aha, I see what's going on" might happen a bit earlier. But it's really not important; that's a small subset of the potential audience. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_cache_writer.h (right): https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:143: int DoLoop(int result); On 2015/09/16 19:25:57, Elly Jones wrote: > On 2015/09/16 18:57:28, rdsmith wrote: > > Suggestion, up to you: My understanding from other comments in this file is > that > > the only information passed back to the caller is an Error return (possibly > > net::OK). My understanding of the DoLoop() idiom is that the return value > from > > DoLoop() is intended to be passed back to the caller. Would it be reasonable > to > > make the return value more specifically an Error return and change the logic > > underneath DoLoop() to just pass around Errors? > > I would rather not deviate from the idiom at all if I'm going to use the idiom, > so I'll leave this as an int. Acknowledged. https://codereview.chromium.org/1315443003/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_cache_writer.h:190: bool io_pending_; On 2015/09/16 19:25:57, Elly Jones wrote: > On 2015/09/16 18:57:28, rdsmith wrote: > > If I read the code correctly, this variable is only used for assertions; the > > actually determination of io_pending_ is based on the state_ variable (as it > > should be based on my understanding of the code :-}). I wince a bit at seeing > > variables that are only used for assertions, just because that can change, > > resulting in an invisible jump in code complexity. Willing to put in a > comment > > about this being only used for assertion and it's semi-equivalence to state_ > != > > DONE && not in synchronous DoLoop() invocation? > > Done. > > I sort of wish we had a naming convention for variables like this, like "bool > debug_io_pending_" or "bool io_pending_assert_only_" or something, to indicate > that it's being used exclusively as a sanity-check. I would #ifndef NDEBUG > around it, but that looks gross in the code. Oh well. Acknowledged.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/1315443003/#ps180001 (title: "Documentation fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315443003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315443003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, michaeln@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1315443003/#ps200001 (title: "Fix bogus DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315443003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315443003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, michaeln@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1315443003/#ps220001 (title: "CONTENT_EXPORT ServiceWorkerCacheWriter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315443003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315443003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, michaeln@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1315443003/#ps240001 (title: "Pull in CONTENT_EXPORT header")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315443003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315443003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/09/18 13:37:45, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Sorry for the spam, y'all. My workstation isn't feeling very workstation-y right now so I'm iterating using the trybots :\.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, michaeln@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1315443003/#ps260001 (title: "Fix broken include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315443003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315443003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/d778a5a82cdccab2b3cbf4940749753c98e59dcc Cr-Commit-Position: refs/heads/master@{#349669}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1351423002/ by japhet@chromium.org. The reason for reverting is: Caused a layout test to timeout on mac/win: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec... Test was added as part of fixing https://code.google.com/p/chromium/issues/detail?id=419999 and the bugfix looks to be in highly related code.. |
