Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2299523003:
Add synchronous callback support to important_file_writer.cc (Closed)
DescriptionAdd synchronous callback support to important_file_writer.cc
This is required to support post-write callbacks which need to execute
even if Chrome is shutting down.
Carved out of https://codereview.chromium.org/2204943002/, new call will be used there.
BUG=624858
Committed: https://crrev.com/fc46ac15bce89405e0bfff3e246be0111f466318
Cr-Commit-Position: refs/heads/master@{#420102}
Patch Set 1 #Patch Set 2 : Fix unit test to work on non-Win OSes #Patch Set 3 : Update a parameter name #
Total comments: 10
Patch Set 4 : Struct -> Enum #
Total comments: 2
Patch Set 5 : Add logic to check that the synchronous callback is invoked before. Also rename successful_write_ob… #Patch Set 6 : update some comments #
Total comments: 7
Patch Set 7 : Address comments on #6 #Patch Set 8 : Re-add accidentaly removed test fixture ctor #
Total comments: 11
Patch Set 9 : Start() without options, ResetPolicy Manual #Patch Set 10 : Use base::Passed, on_write -> OnWrite #Patch Set 11 : on_successful_write -> OnNextSuccessfulWrite for consistency #
Total comments: 6
Patch Set 12 : Try rebase on 2337253003 #Patch Set 13 : Try move logic from ImportantFileWriter to JsonPrefStore #
Total comments: 43
Patch Set 14 : Address comments on #13 #
Total comments: 22
Patch Set 15 : Address comments on #14 #
Total comments: 4
Patch Set 16 : Fix last nits #Patch Set 17 : Move variable declaration down to fix 'initialized after field' warning #Patch Set 18 : more re-ordering #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 65 (17 generated)
Description was changed from ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. BUG=624858 ========== to ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. BUG=624858 ==========
proberge@chromium.org changed reviewers: + dcheng@chromium.org, gab@chromium.org
https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:95: }; Use an enum instead of a struct to keep it to a single member (easier verification below). i.e. enum WriteCallbackObservationState { NOT_CALLED, CALLED_WITH_ERROR, CALLED_WITH_SUCCESS, }; https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:195: cb_state = write_callback_observer_.GetAndResetObservationState(); The |write_callback_observer_| should have been called during WriteNow(), add a verification step between WriteNow() and RunUntilIdle() to confirm that the synchronous callback was invoked already but not the other one. https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:219: ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"), The ".." here merely makes this more likely to not be an invalid path doesn't it (i.e. only path/ needs to exist)
https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:95: }; On 2016/08/31 19:45:34, gab wrote: > Use an enum instead of a struct to keep it to a single member (easier > verification below). > > i.e. > > enum WriteCallbackObservationState { > NOT_CALLED, > CALLED_WITH_ERROR, > CALLED_WITH_SUCCESS, > }; Done. https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:195: cb_state = write_callback_observer_.GetAndResetObservationState(); On 2016/08/31 19:45:34, gab wrote: > The |write_callback_observer_| should have been called during WriteNow(), add a > verification step between WriteNow() and RunUntilIdle() to confirm that the > synchronous callback was invoked already but not the other one. Not quite. WriteNow() uses PostTaskAndReply to WriteFileAtomically , so we still need to step in the message loop for the |write_callback_observer_| to have been called. https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:219: ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"), On 2016/08/31 19:45:34, gab wrote: > The ".." here merely makes this more likely to not be an invalid path doesn't it > (i.e. only path/ needs to exist) This is actually a hack to make WriteFileAtomically fail. The File constructor fails if |path| contains path traversal ('..') components. See https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom.... WriteScopedStringToFileAtomically and WriteFileAtomically are both static, which makes mocking/stubbing difficult. We don't want to mock WriteNow, as we're testing its logic. File has a constructor which takes an error code, but then we would need WriteNow to take in a File instead of a FilePath and to pass that file to a different thread with PostTask, which seems sketchy. My initial attempt (patch set 1) created the file and locked it before calling WriteNow(), which works only on Windows since Lock is 'advisory only' on POSIX. See https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom... What do you think is less bad? 1. Keep "bad/../path" and hope no one tries to change the File ctor to accept path traversal components. 2. Use Lock() and enable the test only on Windows 3. Add a static variable which, when set to true, causes WriteFileAtomically to bail with a failure.
https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:195: cb_state = write_callback_observer_.GetAndResetObservationState(); On 2016/08/31 21:06:44, proberge wrote: > On 2016/08/31 19:45:34, gab wrote: > > The |write_callback_observer_| should have been called during WriteNow(), add > a > > verification step between WriteNow() and RunUntilIdle() to confirm that the > > synchronous callback was invoked already but not the other one. > > Not quite. WriteNow() uses PostTaskAndReply to WriteFileAtomically , so we still > need to step in the message loop for the |write_callback_observer_| to have been > called. I see, then the way to split this is to bind ImportantFileWriter to another base::Thread. i.e., at the top of this test, initialize a base::Thread and pass its task_runner() to ImportantFileWriter, then you can independently flush the message loops. Note that you can only RunUntilIdle() from a thread itself, one way to flush a message loop without having to manually RunUntilIdle() on its thread is to do this: writer.WriteNow(WrapUnique(new std::string("bar"))); // Make sure tasks posted by WriteNow() have ran before // continuing. WaitableEvent done(...MANUAL, ...NOT_SIGNALED); thread.task_runner().PostTask(&WaitableEvent::Signal, &done); done.Wait(); // The |successful_write_observer_| should not yet have been // notified yet but the |write_callback_observer_| should have been. EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); EXPECT_EQ(CALLED_WITH_SUCCESS, ... // There should be a pending task to notify |successful_write_observer_| // in this message loop. RunLoop().RunUntilIdle(); EXPECT_TRUE(... https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:219: ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"), On 2016/08/31 21:06:44, proberge wrote: > On 2016/08/31 19:45:34, gab wrote: > > The ".." here merely makes this more likely to not be an invalid path doesn't > it > > (i.e. only path/ needs to exist) > > This is actually a hack to make WriteFileAtomically fail. The File constructor > fails if |path| contains path traversal ('..') components. > See > https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom.... > > WriteScopedStringToFileAtomically and WriteFileAtomically are both static, which > makes mocking/stubbing difficult. We don't want to mock WriteNow, as we're > testing its logic. > > File has a constructor which takes an error code, but then we would need > WriteNow to take in a File instead of a FilePath and to pass that file to a > different thread with PostTask, which seems sketchy. > > My initial attempt (patch set 1) created the file and locked it before calling > WriteNow(), which works only on Windows since Lock is 'advisory only' on POSIX. > See > https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom... > > What do you think is less bad? > 1. Keep "bad/../path" and hope no one tries to change the File ctor to accept > path traversal components. > 2. Use Lock() and enable the test only on Windows > 3. Add a static variable which, when set to true, causes WriteFileAtomically to > bail with a failure. Ah ok I see, then (1.) is best IMO, but clarify the comment above to make it explicit that the relative path is what's inferring the error. https://codereview.chromium.org/2299523003/diff/60001/base/files/important_fi... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/60001/base/files/important_fi... base/files/important_file_writer_unittest.cc:110: bool write_callback_success_value_; Use WriteCallbackObservationState here instead of two bools
@dcheng can you approve the changes for base/files or suggest another reviewer? Thanks! https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:195: cb_state = write_callback_observer_.GetAndResetObservationState(); On 2016/09/01 19:59:53, gab wrote: > On 2016/08/31 21:06:44, proberge wrote: > > On 2016/08/31 19:45:34, gab wrote: > > > The |write_callback_observer_| should have been called during WriteNow(), > add > > a > > > verification step between WriteNow() and RunUntilIdle() to confirm that the > > > synchronous callback was invoked already but not the other one. > > > > Not quite. WriteNow() uses PostTaskAndReply to WriteFileAtomically , so we > still > > need to step in the message loop for the |write_callback_observer_| to have > been > > called. > > I see, then the way to split this is to bind ImportantFileWriter to another > base::Thread. i.e., at the top of this test, initialize a base::Thread and pass > its task_runner() to ImportantFileWriter, then you can independently flush the > message loops. > > Note that you can only RunUntilIdle() from a thread itself, one way to flush a > message loop without having to manually RunUntilIdle() on its thread is to do > this: > > writer.WriteNow(WrapUnique(new std::string("bar"))); > > // Make sure tasks posted by WriteNow() have ran before > // continuing. > WaitableEvent done(...MANUAL, ...NOT_SIGNALED); > thread.task_runner().PostTask(&WaitableEvent::Signal, &done); > done.Wait(); > > // The |successful_write_observer_| should not yet have been > // notified yet but the |write_callback_observer_| should have been. > EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState()); > EXPECT_EQ(CALLED_WITH_SUCCESS, ... > > // There should be a pending task to notify |successful_write_observer_| > // in this message loop. > RunLoop().RunUntilIdle(); > > EXPECT_TRUE(... Done. https://codereview.chromium.org/2299523003/diff/40001/base/files/important_fi... base/files/important_file_writer_unittest.cc:219: ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"), On 2016/09/01 19:59:53, gab wrote: > On 2016/08/31 21:06:44, proberge wrote: > > On 2016/08/31 19:45:34, gab wrote: > > > The ".." here merely makes this more likely to not be an invalid path > doesn't > > it > > > (i.e. only path/ needs to exist) > > > > This is actually a hack to make WriteFileAtomically fail. The File constructor > > fails if |path| contains path traversal ('..') components. > > See > > > https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom.... > > > > WriteScopedStringToFileAtomically and WriteFileAtomically are both static, > which > > makes mocking/stubbing difficult. We don't want to mock WriteNow, as we're > > testing its logic. > > > > File has a constructor which takes an error code, but then we would need > > WriteNow to take in a File instead of a FilePath and to pass that file to a > > different thread with PostTask, which seems sketchy. > > > > My initial attempt (patch set 1) created the file and locked it before calling > > WriteNow(), which works only on Windows since Lock is 'advisory only' on > POSIX. > > See > > > https://cs.chromium.org/chromium/src/base/files/file.h?dr=Ss&sq=package:chrom... > > > > What do you think is less bad? > > 1. Keep "bad/../path" and hope no one tries to change the File ctor to accept > > path traversal components. > > 2. Use Lock() and enable the test only on Windows > > 3. Add a static variable which, when set to true, causes WriteFileAtomically > to > > bail with a failure. > > Ah ok I see, then (1.) is best IMO, but clarify the comment above to make it > explicit that the relative path is what's inferring the error. Done. https://codereview.chromium.org/2299523003/diff/60001/base/files/important_fi... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/60001/base/files/important_fi... base/files/important_file_writer_unittest.cc:110: bool write_callback_success_value_; On 2016/09/01 19:59:53, gab wrote: > Use WriteCallbackObservationState here instead of two bools Done.
https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:134: base::Thread::Options(base::MessageLoop::TYPE_IO, 0)); I don't think you specifically need an IO thread ("TYPE_IO" is different than and not required for "file I/O") For the same reason, don't call it "io_thread_", something like "file_writer_thread_" will be more correct (same for its thread name). https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:137: ~ImportantFileWriterTest() { io_thread_.Stop(); } Stop() is implicit in base::Thread's destructor, don't need to invoke it here. https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:185: ImportantFileWriter writer(file_, GetTestThreadTaskRunner()); Since it's only for this test actually, bring the base::Thread as a local variable here instead of adding it to the test fixture.
https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:134: base::Thread::Options(base::MessageLoop::TYPE_IO, 0)); On 2016/09/07 19:40:52, gab wrote: > I don't think you specifically need an IO thread ("TYPE_IO" is different than > and not required for "file I/O") > > For the same reason, don't call it "io_thread_", something like > "file_writer_thread_" will be more correct (same for its thread name). Done. https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:137: ~ImportantFileWriterTest() { io_thread_.Stop(); } On 2016/09/07 19:40:52, gab wrote: > Stop() is implicit in base::Thread's destructor, don't need to invoke it here. Done. https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:185: ImportantFileWriter writer(file_, GetTestThreadTaskRunner()); On 2016/09/07 19:40:52, gab wrote: > Since it's only for this test actually, bring the base::Thread as a local > variable here instead of adding it to the test fixture. Done. I thought it would be useful to have it in the test fixture to avoid the duplicated logic.
lgtm % comments https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/100001/base/files/important_f... base/files/important_file_writer_unittest.cc:185: ImportantFileWriter writer(file_, GetTestThreadTaskRunner()); On 2016/09/08 17:19:31, proberge wrote: > On 2016/09/07 19:40:52, gab wrote: > > Since it's only for this test actually, bring the base::Thread as a local > > variable here instead of adding it to the test fixture. > > Done. I thought it would be useful to have it in the test fixture to avoid the > duplicated logic. Right, except that it then started an unnecessary thread in every other test using the fixture. Once Reset() is removed per other comment, it's not that much duplication really and the test is more readable because the logic is local. https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:161: file_writer_thread.StartWithOptions( Start() will do just that by default, use that instead. https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:164: base::WaitableEvent::ResetPolicy::MANUAL, Make this AUTOMATIC, then you don't have to ever manually Reset() below.
https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:161: file_writer_thread.StartWithOptions( On 2016/09/08 17:50:46, gab wrote: > Start() will do just that by default, use that instead. Done. https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:164: base::WaitableEvent::ResetPolicy::MANUAL, On 2016/09/08 17:50:46, gab wrote: > Make this AUTOMATIC, then you don't have to ever manually Reset() below. Done.
Also, am I missing something here? Nothing outside the test seems to be calling the new function. https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer.cc:178: on_next_write_callback_); Passed(&on_next_write_callback_) *should* work. https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:102: void on_write(bool success) { Nit: Chromium style dictates that this should be named OnWrite
On 2016/09/09 05:18:59, dcheng wrote: > Also, am I missing something here? Nothing outside the test seems to be calling > the new function. That's intended. This is pre-work for another CL.
Thanks for the review! https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer.cc:178: on_next_write_callback_); On 2016/09/09 05:18:59, dcheng wrote: > Passed(&on_next_write_callback_) *should* work. Done. https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:102: void on_write(bool success) { On 2016/09/09 05:18:59, dcheng wrote: > Nit: Chromium style dictates that this should be named OnWrite Done.
On 2016/09/09 17:23:46, proberge wrote: > On 2016/09/09 05:18:59, dcheng wrote: > > Also, am I missing something here? Nothing outside the test seems to be > calling > > the new function. > > That's intended. This is pre-work for another CL. How about adding something like: """ Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. """ to this CL's desc? On 2016/09/09 17:24:00, proberge wrote: > Thanks for the review! > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > File base/files/important_file_writer.cc (right): > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > base/files/important_file_writer.cc:178: on_next_write_callback_); > On 2016/09/09 05:18:59, dcheng wrote: > > Passed(&on_next_write_callback_) *should* work. > > Done. > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > File base/files/important_file_writer_unittest.cc (right): > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > base/files/important_file_writer_unittest.cc:102: void on_write(bool success) { > On 2016/09/09 05:18:59, dcheng wrote: > > Nit: Chromium style dictates that this should be named OnWrite > > Done. Forgot to upload latest PS?
Description was changed from ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. BUG=624858 ========== to ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. BUG=624858 ==========
On 2016/09/09 19:06:51, gab wrote: > On 2016/09/09 17:23:46, proberge wrote: > > On 2016/09/09 05:18:59, dcheng wrote: > > > Also, am I missing something here? Nothing outside the test seems to be > > calling > > > the new function. > > > > That's intended. This is pre-work for another CL. > > How about adding something like: > """ > Carved out of https://codereview.chromium.org/2204943002/, new call will be used > there. > """ > to this CL's desc? > > > On 2016/09/09 17:24:00, proberge wrote: > > Thanks for the review! > > > > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > > File base/files/important_file_writer.cc (right): > > > > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > > base/files/important_file_writer.cc:178: on_next_write_callback_); > > On 2016/09/09 05:18:59, dcheng wrote: > > > Passed(&on_next_write_callback_) *should* work. > > > > Done. > > > > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > > File base/files/important_file_writer_unittest.cc (right): > > > > > https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... > > base/files/important_file_writer_unittest.cc:102: void on_write(bool success) > { > > On 2016/09/09 05:18:59, dcheng wrote: > > > Nit: Chromium style dictates that this should be named OnWrite > > > > Done. > > Forgot to upload latest PS? Yep, sorry about that :(
(still lgtm % nit) https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer.cc:178: on_next_write_callback_); On 2016/09/09 17:24:00, proberge wrote: > On 2016/09/09 05:18:59, dcheng wrote: > > Passed(&on_next_write_callback_) *should* work. > > Done. Oh sweet, didn't know callbacks were moveable now :-)! https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:102: void on_write(bool success) { On 2016/09/09 17:24:00, proberge wrote: > On 2016/09/09 05:18:59, dcheng wrote: > > Nit: Chromium style dictates that this should be named OnWrite > > Done. I hadn't mentioned this mostly because the approach is copied from on_successful_write() above (so for consistency we should fix that one too). I thought this was "okay" even though it's not a one-liner as it's effectively a one-liner setter but it just happens to have an EXPECT first. I've definitely seen DCHECK+one-liner before written in snake_case(). But whatever, PascalCase is fine too, so long as on_successful_write() is fixed for consistency. Thanks!
https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/140001/base/files/important_f... base/files/important_file_writer_unittest.cc:102: void on_write(bool success) { On 2016/09/09 19:35:37, gab wrote: > On 2016/09/09 17:24:00, proberge wrote: > > On 2016/09/09 05:18:59, dcheng wrote: > > > Nit: Chromium style dictates that this should be named OnWrite > > > > Done. > > I hadn't mentioned this mostly because the approach is copied from > on_successful_write() above (so for consistency we should fix that one too). > > I thought this was "okay" even though it's not a one-liner as it's effectively a > one-liner setter but it just happens to have an EXPECT first. I've definitely > seen DCHECK+one-liner before written in snake_case(). > > But whatever, PascalCase is fine too, so long as on_successful_write() is fixed > for consistency. > > Thanks! Done.
https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... base/files/important_file_writer_unittest.cc:178: wait_helper.Wait(); Note: I just grew tired of having to do and suggest this pattern in multiple places and am introducing base::Thread::FlushForTesting() in https://codereview.chromium.org/2337253003/. You're welcome to rebase on top of it, say this CL is on branch "foo" with "master" as upstream then you can: git checkout -b flushfortesting master git cl patch 2337253003 git rebase flushfortesting foo git branch -u flushfortesting And the next time you git cl upload, Rietveld (and CQ) will know that you have a pending dependency (the diff will be as expected and CQ dry-run will be able to try the patch by also applying the dependent :-)).
So I think the main thing that feels weird to me is it seems like we're bolting on a rather special purpose callback (to ensure it provides an answer of some sort, even when shutting down) to what is otherwise a fairly general purpose API. Is it possible to serve this use case by just using ImportantFileWriter::WriteFileAtomically? https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... base/files/important_file_writer.cc:179: on_next_write_callback_.Reset(); No need to call Reset() here, Passed() will transfer ownership of it. https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... base/files/important_file_writer.cc:250: on_next_successful_write_reply_.Reset(); This can use RunAndReset() in callback_helpers.h
On 2016/09/14 06:54:49, dcheng wrote: > So I think the main thing that feels weird to me is it seems like we're bolting > on a rather special purpose callback (to ensure it provides an answer of some > sort, even when shutting down) to what is otherwise a fairly general purpose > API. > > Is it possible to serve this use case by just using > ImportantFileWriter::WriteFileAtomically? Our use case (JsonPrefStore) is pretty tightly coupled with ImportantFileWriter as a DataSerializer to do scheduled writes. I don't think WriteFileAtomically is an option for us; @gab wdyt? https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... base/files/important_file_writer.cc:179: on_next_write_callback_.Reset(); On 2016/09/14 06:54:49, dcheng wrote: > No need to call Reset() here, Passed() will transfer ownership of it. Done. https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... base/files/important_file_writer.cc:250: on_next_successful_write_reply_.Reset(); On 2016/09/14 06:54:49, dcheng wrote: > This can use RunAndReset() in callback_helpers.h RunAndReset is available to ScopedClosureRunner, not to normal base::Closure. https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/200001/base/files/important_f... base/files/important_file_writer_unittest.cc:178: wait_helper.Wait(); On 2016/09/13 20:47:37, gab wrote: > Note: I just grew tired of having to do and suggest this pattern in multiple > places and am introducing base::Thread::FlushForTesting() in > https://codereview.chromium.org/2337253003/. > > You're welcome to rebase on top of it, say this CL is on branch "foo" with > "master" as upstream then you can: > > git checkout -b flushfortesting master > git cl patch 2337253003 > git rebase flushfortesting foo > git branch -u flushfortesting > > And the next time you git cl upload, Rietveld (and CQ) will know that you have a > pending dependency (the diff will be as expected and CQ dry-run will be able to > try the patch by also applying the dependent :-)). Done. Thanks!
On 2016/09/14 14:33:34, proberge wrote: > On 2016/09/14 06:54:49, dcheng wrote: > > So I think the main thing that feels weird to me is it seems like we're > bolting > > on a rather special purpose callback (to ensure it provides an answer of some > > sort, even when shutting down) to what is otherwise a fairly general purpose > > API. > > > > Is it possible to serve this use case by just using > > ImportantFileWriter::WriteFileAtomically? > > Our use case (JsonPrefStore) is pretty tightly coupled with ImportantFileWriter > as a DataSerializer to do scheduled writes. I don't think WriteFileAtomically is > an option for us; @gab wdyt? Agreed, @dcheng: what's happening is that we need to make sure something follows every write (to complete an operation we kick off pre-write, during serialization). On shutdown the last write occurs on the ImportantFileWriter's I/O thread but the reply never makes it as the owning thread is done processing messages. So if we rely on the reply to complete our operation, we reliably end up in a broken state where the operation is left in an in-between state. While we support this race say it crashed in the middle, having it be the default state on shutdown isn't acceptable. And indeed, since this is hooked for every write, so using WriteFileAtomically isn't an option. Our use case is indeed fairly special purposed but I think the proposed API change is generic enough that it could be re-used by another feature requiring similar functionality for whatever reason.
On 2016/09/14 14:48:46, gab wrote: > On 2016/09/14 14:33:34, proberge wrote: > > On 2016/09/14 06:54:49, dcheng wrote: > > > So I think the main thing that feels weird to me is it seems like we're > > bolting > > > on a rather special purpose callback (to ensure it provides an answer of > some > > > sort, even when shutting down) to what is otherwise a fairly general purpose > > > API. > > > > > > Is it possible to serve this use case by just using > > > ImportantFileWriter::WriteFileAtomically? > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > ImportantFileWriter > > as a DataSerializer to do scheduled writes. I don't think WriteFileAtomically > is > > an option for us; @gab wdyt? > > Agreed, @dcheng: what's happening is that we need to make sure something follows > every write (to complete an operation we kick off pre-write, during > serialization). On shutdown the last write occurs on the ImportantFileWriter's > I/O thread but the reply never makes it as the owning thread is done processing > messages. So if we rely on the reply to complete our operation, we reliably end > up in a broken state where the operation is left in an in-between state. While > we support this race say it crashed in the middle, having it be the default > state on shutdown isn't acceptable. > > And indeed, since this is hooked for every write, so using WriteFileAtomically > isn't an option. > > Our use case is indeed fairly special purposed but I think the proposed API > change is generic enough that it could be re-used by another feature requiring > similar functionality for whatever reason. Sorry for the delayed reply. Can we just get rid of the old RegisterOnNextSuccessfulWriteCallback() then? It looks like the next CL touches the only two users of this callback: it feels a bit weird to have both callbacks, so if we could just get rid of the old one and only use the new one, I'd be OK with that.
On 2016/09/15 09:10:51, dcheng wrote: > On 2016/09/14 14:48:46, gab wrote: > > On 2016/09/14 14:33:34, proberge wrote: > > > On 2016/09/14 06:54:49, dcheng wrote: > > > > So I think the main thing that feels weird to me is it seems like we're > > > bolting > > > > on a rather special purpose callback (to ensure it provides an answer of > > some > > > > sort, even when shutting down) to what is otherwise a fairly general > purpose > > > > API. > > > > > > > > Is it possible to serve this use case by just using > > > > ImportantFileWriter::WriteFileAtomically? > > > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > > ImportantFileWriter > > > as a DataSerializer to do scheduled writes. I don't think > WriteFileAtomically > > is > > > an option for us; @gab wdyt? > > > > Agreed, @dcheng: what's happening is that we need to make sure something > follows > > every write (to complete an operation we kick off pre-write, during > > serialization). On shutdown the last write occurs on the ImportantFileWriter's > > I/O thread but the reply never makes it as the owning thread is done > processing > > messages. So if we rely on the reply to complete our operation, we reliably > end > > up in a broken state where the operation is left in an in-between state. While > > we support this race say it crashed in the middle, having it be the default > > state on shutdown isn't acceptable. > > > > And indeed, since this is hooked for every write, so using WriteFileAtomically > > isn't an option. > > > > Our use case is indeed fairly special purposed but I think the proposed API > > change is generic enough that it could be re-used by another feature requiring > > similar functionality for whatever reason. > > Sorry for the delayed reply. Can we just get rid of the old > RegisterOnNextSuccessfulWriteCallback() then? It looks like the next CL touches > the only two users of this callback: it feels a bit weird to have both > callbacks, so if we could just get rid of the old one and only use the new one, > I'd be OK with that. Sadly, no, because the other one does need to run as a reply as it calls into JsonPrefStore::pref_filter_ which is not thread-safe (must be used on the owning sequence -- in practice this is the UI thread for Preferences).
On 2016/09/15 13:38:33, gab wrote: > On 2016/09/15 09:10:51, dcheng wrote: > > On 2016/09/14 14:48:46, gab wrote: > > > On 2016/09/14 14:33:34, proberge wrote: > > > > On 2016/09/14 06:54:49, dcheng wrote: > > > > > So I think the main thing that feels weird to me is it seems like we're > > > > bolting > > > > > on a rather special purpose callback (to ensure it provides an answer of > > > some > > > > > sort, even when shutting down) to what is otherwise a fairly general > > purpose > > > > > API. > > > > > > > > > > Is it possible to serve this use case by just using > > > > > ImportantFileWriter::WriteFileAtomically? > > > > > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > > > ImportantFileWriter > > > > as a DataSerializer to do scheduled writes. I don't think > > WriteFileAtomically > > > is > > > > an option for us; @gab wdyt? > > > > > > Agreed, @dcheng: what's happening is that we need to make sure something > > follows > > > every write (to complete an operation we kick off pre-write, during > > > serialization). On shutdown the last write occurs on the > ImportantFileWriter's > > > I/O thread but the reply never makes it as the owning thread is done > > processing > > > messages. So if we rely on the reply to complete our operation, we reliably > > end > > > up in a broken state where the operation is left in an in-between state. > While > > > we support this race say it crashed in the middle, having it be the default > > > state on shutdown isn't acceptable. > > > > > > And indeed, since this is hooked for every write, so using > WriteFileAtomically > > > isn't an option. > > > > > > Our use case is indeed fairly special purposed but I think the proposed API > > > change is generic enough that it could be re-used by another feature > requiring > > > similar functionality for whatever reason. > > > > Sorry for the delayed reply. Can we just get rid of the old > > RegisterOnNextSuccessfulWriteCallback() then? It looks like the next CL > touches > > the only two users of this callback: it feels a bit weird to have both > > callbacks, so if we could just get rid of the old one and only use the new > one, > > I'd be OK with that. > > Sadly, no, because the other one does need to run as a reply as it calls into > JsonPrefStore::pref_filter_ which is not thread-safe (must be used on the owning > sequence -- in practice this is the UI thread for Preferences). Can't JsonPrefStore bounce back to the correct thread itself?
On 2016/09/15 18:54:00, dcheng wrote: > On 2016/09/15 13:38:33, gab wrote: > > On 2016/09/15 09:10:51, dcheng wrote: > > > On 2016/09/14 14:48:46, gab wrote: > > > > On 2016/09/14 14:33:34, proberge wrote: > > > > > On 2016/09/14 06:54:49, dcheng wrote: > > > > > > So I think the main thing that feels weird to me is it seems like > we're > > > > > bolting > > > > > > on a rather special purpose callback (to ensure it provides an answer > of > > > > some > > > > > > sort, even when shutting down) to what is otherwise a fairly general > > > purpose > > > > > > API. > > > > > > > > > > > > Is it possible to serve this use case by just using > > > > > > ImportantFileWriter::WriteFileAtomically? > > > > > > > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > > > > ImportantFileWriter > > > > > as a DataSerializer to do scheduled writes. I don't think > > > WriteFileAtomically > > > > is > > > > > an option for us; @gab wdyt? > > > > > > > > Agreed, @dcheng: what's happening is that we need to make sure something > > > follows > > > > every write (to complete an operation we kick off pre-write, during > > > > serialization). On shutdown the last write occurs on the > > ImportantFileWriter's > > > > I/O thread but the reply never makes it as the owning thread is done > > > processing > > > > messages. So if we rely on the reply to complete our operation, we > reliably > > > end > > > > up in a broken state where the operation is left in an in-between state. > > While > > > > we support this race say it crashed in the middle, having it be the > default > > > > state on shutdown isn't acceptable. > > > > > > > > And indeed, since this is hooked for every write, so using > > WriteFileAtomically > > > > isn't an option. > > > > > > > > Our use case is indeed fairly special purposed but I think the proposed > API > > > > change is generic enough that it could be re-used by another feature > > requiring > > > > similar functionality for whatever reason. > > > > > > Sorry for the delayed reply. Can we just get rid of the old > > > RegisterOnNextSuccessfulWriteCallback() then? It looks like the next CL > > touches > > > the only two users of this callback: it feels a bit weird to have both > > > callbacks, so if we could just get rid of the old one and only use the new > > one, > > > I'd be OK with that. > > > > Sadly, no, because the other one does need to run as a reply as it calls into > > JsonPrefStore::pref_filter_ which is not thread-safe (must be used on the > owning > > sequence -- in practice this is the UI thread for Preferences). > > Can't JsonPrefStore bounce back to the correct thread itself? The semantics of the two calls are a bit different too. The old one wants to be invoked on origin sequence after the next *successful* write. Whereas the new one wants to be invoked on the write sequence after the *next write* (and be told about the result). I guess JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(), could handle the thread hop to either report the success or re-register from the origin sequence on failure, but that feels weird.
On 2016/09/15 21:00:35, gab wrote: > On 2016/09/15 18:54:00, dcheng wrote: > > On 2016/09/15 13:38:33, gab wrote: > > > On 2016/09/15 09:10:51, dcheng wrote: > > > > On 2016/09/14 14:48:46, gab wrote: > > > > > On 2016/09/14 14:33:34, proberge wrote: > > > > > > On 2016/09/14 06:54:49, dcheng wrote: > > > > > > > So I think the main thing that feels weird to me is it seems like > > we're > > > > > > bolting > > > > > > > on a rather special purpose callback (to ensure it provides an > answer > > of > > > > > some > > > > > > > sort, even when shutting down) to what is otherwise a fairly general > > > > purpose > > > > > > > API. > > > > > > > > > > > > > > Is it possible to serve this use case by just using > > > > > > > ImportantFileWriter::WriteFileAtomically? > > > > > > > > > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > > > > > ImportantFileWriter > > > > > > as a DataSerializer to do scheduled writes. I don't think > > > > WriteFileAtomically > > > > > is > > > > > > an option for us; @gab wdyt? > > > > > > > > > > Agreed, @dcheng: what's happening is that we need to make sure something > > > > follows > > > > > every write (to complete an operation we kick off pre-write, during > > > > > serialization). On shutdown the last write occurs on the > > > ImportantFileWriter's > > > > > I/O thread but the reply never makes it as the owning thread is done > > > > processing > > > > > messages. So if we rely on the reply to complete our operation, we > > reliably > > > > end > > > > > up in a broken state where the operation is left in an in-between state. > > > While > > > > > we support this race say it crashed in the middle, having it be the > > default > > > > > state on shutdown isn't acceptable. > > > > > > > > > > And indeed, since this is hooked for every write, so using > > > WriteFileAtomically > > > > > isn't an option. > > > > > > > > > > Our use case is indeed fairly special purposed but I think the proposed > > API > > > > > change is generic enough that it could be re-used by another feature > > > requiring > > > > > similar functionality for whatever reason. > > > > > > > > Sorry for the delayed reply. Can we just get rid of the old > > > > RegisterOnNextSuccessfulWriteCallback() then? It looks like the next CL > > > touches > > > > the only two users of this callback: it feels a bit weird to have both > > > > callbacks, so if we could just get rid of the old one and only use the new > > > one, > > > > I'd be OK with that. > > > > > > Sadly, no, because the other one does need to run as a reply as it calls > into > > > JsonPrefStore::pref_filter_ which is not thread-safe (must be used on the > > owning > > > sequence -- in practice this is the UI thread for Preferences). > > > > Can't JsonPrefStore bounce back to the correct thread itself? > > The semantics of the two calls are a bit different too. > > The old one wants to be invoked on origin sequence after the next *successful* > write. > > Whereas the new one wants to be invoked on the write sequence after the *next > write* (and be told about the result). > > I guess JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(), could handle the > thread hop to either report the success or re-register from the origin sequence > on failure, but that feels weird. It feels weirder to keep both callbacks in //base. There's only one (or two?) consumers of the old callback; given that it's not used by anything else, it seems to make more sense to make the consumers just implement that extra logic (on success, also hop to ui thread and do something)
On 2016/09/15 21:09:21, dcheng wrote: > On 2016/09/15 21:00:35, gab wrote: > > On 2016/09/15 18:54:00, dcheng wrote: > > > On 2016/09/15 13:38:33, gab wrote: > > > > On 2016/09/15 09:10:51, dcheng wrote: > > > > > On 2016/09/14 14:48:46, gab wrote: > > > > > > On 2016/09/14 14:33:34, proberge wrote: > > > > > > > On 2016/09/14 06:54:49, dcheng wrote: > > > > > > > > So I think the main thing that feels weird to me is it seems like > > > we're > > > > > > > bolting > > > > > > > > on a rather special purpose callback (to ensure it provides an > > answer > > > of > > > > > > some > > > > > > > > sort, even when shutting down) to what is otherwise a fairly > general > > > > > purpose > > > > > > > > API. > > > > > > > > > > > > > > > > Is it possible to serve this use case by just using > > > > > > > > ImportantFileWriter::WriteFileAtomically? > > > > > > > > > > > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > > > > > > ImportantFileWriter > > > > > > > as a DataSerializer to do scheduled writes. I don't think > > > > > WriteFileAtomically > > > > > > is > > > > > > > an option for us; @gab wdyt? > > > > > > > > > > > > Agreed, @dcheng: what's happening is that we need to make sure > something > > > > > follows > > > > > > every write (to complete an operation we kick off pre-write, during > > > > > > serialization). On shutdown the last write occurs on the > > > > ImportantFileWriter's > > > > > > I/O thread but the reply never makes it as the owning thread is done > > > > > processing > > > > > > messages. So if we rely on the reply to complete our operation, we > > > reliably > > > > > end > > > > > > up in a broken state where the operation is left in an in-between > state. > > > > While > > > > > > we support this race say it crashed in the middle, having it be the > > > default > > > > > > state on shutdown isn't acceptable. > > > > > > > > > > > > And indeed, since this is hooked for every write, so using > > > > WriteFileAtomically > > > > > > isn't an option. > > > > > > > > > > > > Our use case is indeed fairly special purposed but I think the > proposed > > > API > > > > > > change is generic enough that it could be re-used by another feature > > > > requiring > > > > > > similar functionality for whatever reason. > > > > > > > > > > Sorry for the delayed reply. Can we just get rid of the old > > > > > RegisterOnNextSuccessfulWriteCallback() then? It looks like the next CL > > > > touches > > > > > the only two users of this callback: it feels a bit weird to have both > > > > > callbacks, so if we could just get rid of the old one and only use the > new > > > > one, > > > > > I'd be OK with that. > > > > > > > > Sadly, no, because the other one does need to run as a reply as it calls > > into > > > > JsonPrefStore::pref_filter_ which is not thread-safe (must be used on the > > > owning > > > > sequence -- in practice this is the UI thread for Preferences). > > > > > > Can't JsonPrefStore bounce back to the correct thread itself? > > > > The semantics of the two calls are a bit different too. > > > > The old one wants to be invoked on origin sequence after the next *successful* > > write. > > > > Whereas the new one wants to be invoked on the write sequence after the *next > > write* (and be told about the result). > > > > I guess JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(), could handle > the > > thread hop to either report the success or re-register from the origin > sequence > > on failure, but that feels weird. > > It feels weirder to keep both callbacks in //base. There's only one (or two?) > consumers of the old callback; given that it's not used by anything else, it > seems to make more sense to make the consumers just implement that extra logic > (on success, also hop to ui thread and do something) Ok, I think I buy that, basically since it's an "important" write it makes sense for the callback to be synchronous with the write as they may also be "important" and if a user needs to hop back to owning sequence they can do so themselves. Alright, let's do that. @proberge, back to you :-)
On 2016/09/15 21:15:05, gab wrote: > On 2016/09/15 21:09:21, dcheng wrote: > > On 2016/09/15 21:00:35, gab wrote: > > > On 2016/09/15 18:54:00, dcheng wrote: > > > > On 2016/09/15 13:38:33, gab wrote: > > > > > On 2016/09/15 09:10:51, dcheng wrote: > > > > > > On 2016/09/14 14:48:46, gab wrote: > > > > > > > On 2016/09/14 14:33:34, proberge wrote: > > > > > > > > On 2016/09/14 06:54:49, dcheng wrote: > > > > > > > > > So I think the main thing that feels weird to me is it seems > like > > > > we're > > > > > > > > bolting > > > > > > > > > on a rather special purpose callback (to ensure it provides an > > > answer > > > > of > > > > > > > some > > > > > > > > > sort, even when shutting down) to what is otherwise a fairly > > general > > > > > > purpose > > > > > > > > > API. > > > > > > > > > > > > > > > > > > Is it possible to serve this use case by just using > > > > > > > > > ImportantFileWriter::WriteFileAtomically? > > > > > > > > > > > > > > > > Our use case (JsonPrefStore) is pretty tightly coupled with > > > > > > > ImportantFileWriter > > > > > > > > as a DataSerializer to do scheduled writes. I don't think > > > > > > WriteFileAtomically > > > > > > > is > > > > > > > > an option for us; @gab wdyt? > > > > > > > > > > > > > > Agreed, @dcheng: what's happening is that we need to make sure > > something > > > > > > follows > > > > > > > every write (to complete an operation we kick off pre-write, during > > > > > > > serialization). On shutdown the last write occurs on the > > > > > ImportantFileWriter's > > > > > > > I/O thread but the reply never makes it as the owning thread is done > > > > > > processing > > > > > > > messages. So if we rely on the reply to complete our operation, we > > > > reliably > > > > > > end > > > > > > > up in a broken state where the operation is left in an in-between > > state. > > > > > While > > > > > > > we support this race say it crashed in the middle, having it be the > > > > default > > > > > > > state on shutdown isn't acceptable. > > > > > > > > > > > > > > And indeed, since this is hooked for every write, so using > > > > > WriteFileAtomically > > > > > > > isn't an option. > > > > > > > > > > > > > > Our use case is indeed fairly special purposed but I think the > > proposed > > > > API > > > > > > > change is generic enough that it could be re-used by another feature > > > > > requiring > > > > > > > similar functionality for whatever reason. > > > > > > > > > > > > Sorry for the delayed reply. Can we just get rid of the old > > > > > > RegisterOnNextSuccessfulWriteCallback() then? It looks like the next > CL > > > > > touches > > > > > > the only two users of this callback: it feels a bit weird to have both > > > > > > callbacks, so if we could just get rid of the old one and only use the > > new > > > > > one, > > > > > > I'd be OK with that. > > > > > > > > > > Sadly, no, because the other one does need to run as a reply as it calls > > > into > > > > > JsonPrefStore::pref_filter_ which is not thread-safe (must be used on > the > > > > owning > > > > > sequence -- in practice this is the UI thread for Preferences). > > > > > > > > Can't JsonPrefStore bounce back to the correct thread itself? > > > > > > The semantics of the two calls are a bit different too. > > > > > > The old one wants to be invoked on origin sequence after the next > *successful* > > > write. > > > > > > Whereas the new one wants to be invoked on the write sequence after the > *next > > > write* (and be told about the result). > > > > > > I guess JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(), could handle > > the > > > thread hop to either report the success or re-register from the origin > > sequence > > > on failure, but that feels weird. > > > > It feels weirder to keep both callbacks in //base. There's only one (or two?) > > consumers of the old callback; given that it's not used by anything else, it > > seems to make more sense to make the consumers just implement that extra logic > > (on success, also hop to ui thread and do something) > > Ok, I think I buy that, basically since it's an "important" write it makes sense > for the callback to be synchronous with the write as they may also be > "important" and if a user needs to hop back to owning sequence they can do so > themselves. > > Alright, let's do that. @proberge, back to you :-) Tried using a single callback in important_file_writer and having logic in JsonPrefStore to handle synchronization of its callback needs. PTAL.
base LGTM https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer_unittest.cc:129: writer.WriteNow(WrapUnique(new std::string("foo"))); Nit: MakeUnique<std::string> here and elsewhere.
This looks great but "not lgtm" yet (to remove my previous lgtm) as there is a race in JsonPrefStore as it stands. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer.cc:175: auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data), In the mindset of the comment below, I think s/auto/Closure/ makes this code easier to read without introducing many more characters. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer.cc:178: if (!task_runner_->PostTask(FROM_HERE, MakeCriticalClosure(Bind(task)))) { |task| is already a Closure, why do we need the extra Bind() wrapping it? https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer.h (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer.h:102: // If called more than once before a write is done, the latest callback s/before a write is done/before a write is scheduled on |task_runner|/ (i.e. the callback is passed when the write is scheduled and the next one can be registered before the write is truly "done" -- note your new approach actually fixes a race in the previous implementation whereby when the reply arrived it could in theory run a callback registered later... this never occurred in practice the way it was used today, but this CL is still addressing a to-this-date-unknown API bug :-)) https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer_unittest.cc:123: rm this empty line to be consistent with surrounding tests https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer_unittest.cc:176: Re-add the test with the independent thread and confirm that the callback is invoked on that thread, not on the main thread (by confirming that running the current loop doesn't result in the notification but flushing the independent thread does -- you'll need make this deterministic by posting a WaitableEvent::Wait task to the thread before calling WriteNow() so that the callback is queued behind it and you control when it should be released, otherwise the callback could still racily run while you're verifying that it doesn't run as part of the main loop). https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:329: // Static "static" (lower-case) https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:342: calling_store->has_pending_write_callback_ = false; Member state can only be modified on the owning sequence. This callback runs off-sequence and as such touching member state without synchronization (e.g. locks) is racy -- and I don't think we want to add explicit synchronization. I'm trying to think of the best way to achieve what we want here but will post these comments before that to give you some head way (or let you find the best way before I get to it :-)). https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:358: const base::Closure& on_next_successful_write_reply_reply) { s/on_next_successful_write_reply_reply/on_next_successful_write_reply/ https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.h:117: // Registers |on_next_write_callback| to be called once, on the ... to be called once, synchronously, on the ... https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.h:120: void RegisterOnNextWriteCallback( RegisterOnNextWriteSynchronousCallback https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:983: bool successful_write_reply_observed_; Use C++11 member initialization (i.e. = false here) and define default constructor instead of using initializer lists. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1018: WriteCallbackObservationState observation_state_; C++11 member init here too https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1042: // Creates a JsonPrefStore with the given |file_writer|. |file_writer| refers to nothing? https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1050: scoped_refptr<SequencedTaskRunner> task_runner) { Unused? Should there be a test with an independent thread using this though? https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1057: scoped_refptr<JsonPrefStore> pref_store) { scoped_refptr<JsonPrefStore> as a parameter implicitly indicates that the method may take ownership. When it definitely won't, a raw pointer should be used. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1061: void TriggerFakeWriteForCallback(scoped_refptr<JsonPrefStore> pref_store, Same here https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1075: }; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer.cc:175: auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data), On 2016/09/19 18:01:30, gab wrote: > In the mindset of the comment below, I think s/auto/Closure/ makes this code > easier to read without introducing many more characters. Done. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer.cc:178: if (!task_runner_->PostTask(FROM_HERE, MakeCriticalClosure(Bind(task)))) { On 2016/09/19 18:01:30, gab wrote: > |task| is already a Closure, why do we need the extra Bind() wrapping it? Done. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer.h (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer.h:102: // If called more than once before a write is done, the latest callback On 2016/09/19 18:01:30, gab wrote: > s/before a write is done/before a write is scheduled on |task_runner|/ > > (i.e. the callback is passed when the write is scheduled and the next one can be > registered before the write is truly "done" -- note your new approach actually > fixes a race in the previous implementation whereby when the reply arrived it > could in theory run a callback registered later... this never occurred in > practice the way it was used today, but this CL is still addressing a > to-this-date-unknown API bug :-)) Done. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer_unittest.cc:123: On 2016/09/19 18:01:30, gab wrote: > rm this empty line to be consistent with surrounding tests Done. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer_unittest.cc:129: writer.WriteNow(WrapUnique(new std::string("foo"))); On 2016/09/16 22:42:05, dcheng wrote: > Nit: MakeUnique<std::string> here and elsewhere. Done. https://codereview.chromium.org/2299523003/diff/240001/base/files/important_f... base/files/important_file_writer_unittest.cc:176: On 2016/09/19 18:01:30, gab wrote: > Re-add the test with the independent thread and confirm that the callback is > invoked on that thread, not on the main thread (by confirming that running the > current loop doesn't result in the notification but flushing the independent > thread does -- you'll need make this deterministic by posting a > WaitableEvent::Wait task to the thread before calling WriteNow() so that the > callback is queued behind it and you control when it should be released, > otherwise the callback could still racily run while you're verifying that it > doesn't run as part of the main loop). Done. Did not add a WaitableEvent::Wait task as I don't think this is non-deterministic as implemented. Please take a look. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:329: // Static On 2016/09/19 18:01:30, gab wrote: > "static" (lower-case) Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:342: calling_store->has_pending_write_callback_ = false; On 2016/09/19 18:01:30, gab wrote: > Member state can only be modified on the owning sequence. This callback runs > off-sequence and as such touching member state without synchronization (e.g. > locks) is racy -- and I don't think we want to add explicit synchronization. > > I'm trying to think of the best way to achieve what we want here but will post > these comments before that to give you some head way (or let you find the best > way before I get to it :-)). Hmm. Not sure if this is better than locks. PTAL. (static instead of member method as it sounds like trouble if the JsonPrefStore dies after the FileWriter thread PostTasks. https://cs.chromium.org/chromium/src/docs/callback.md?dr=Ss&q=callback%5C.md&...) https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:358: const base::Closure& on_next_successful_write_reply_reply) { On 2016/09/19 18:01:30, gab wrote: > s/on_next_successful_write_reply_reply/on_next_successful_write_reply/ Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.h:120: void RegisterOnNextWriteCallback( On 2016/09/19 18:01:30, gab wrote: > RegisterOnNextWriteSynchronousCallback I don't think RegisterOnNextWriteSynchronousCallback is the right name. 1. SynchronousCallback doesn't seem to be the standard way of naming it: there's no codesearch result for a method signature involving Register, Synchronous and Callback. 2. The callback isn't synchronous from the JsonPrefStore's point of view, but rather from the file writer. 3. Does it matter to the caller whether it's synchronous or not? As long as |on_next_write_callback| is thread safe, does it matter on which thread it's run? https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:983: bool successful_write_reply_observed_; On 2016/09/19 18:01:30, gab wrote: > Use C++11 member initialization (i.e. = false here) and define default > constructor instead of using initializer lists. Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1018: WriteCallbackObservationState observation_state_; On 2016/09/19 18:01:31, gab wrote: > C++11 member init here too Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1042: // Creates a JsonPrefStore with the given |file_writer|. On 2016/09/19 18:01:31, gab wrote: > |file_writer| refers to nothing? Copied the comment from line 827. Removed the comment here and there. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1050: scoped_refptr<SequencedTaskRunner> task_runner) { On 2016/09/19 18:01:30, gab wrote: > Unused? Should there be a test with an independent thread using this though? I thought I needed it for TestPostWriteCallbackDuringProfileDeath, but ended up not using it. Any suggestion on what a test with an independent thread would do? https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1057: scoped_refptr<JsonPrefStore> pref_store) { On 2016/09/19 18:01:30, gab wrote: > scoped_refptr<JsonPrefStore> as a parameter implicitly indicates that the method > may take ownership. When it definitely won't, a raw pointer should be used. Was copied from line 834. Updated there as well. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1061: void TriggerFakeWriteForCallback(scoped_refptr<JsonPrefStore> pref_store, On 2016/09/19 18:01:30, gab wrote: > Same here Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1075: }; On 2016/09/19 18:01:30, gab wrote: > DISALLOW_COPY_AND_ASSIGN Looks like DISALLOW_COPY_AND_ASSIGN isn't happy with using TEST_F(JsonPrefStoreCallbackTest, foo): c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1067): error C2512: 'base::JsonPrefStoreCallbackTest': no appropriate default constructor available c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1033): note: see declaration of 'base::JsonPrefStoreCallbackTest'
https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1075: }; On 2016/09/19 21:24:26, proberge wrote: > On 2016/09/19 18:01:30, gab wrote: > > DISALLOW_COPY_AND_ASSIGN > > Looks like DISALLOW_COPY_AND_ASSIGN isn't happy with using > TEST_F(JsonPrefStoreCallbackTest, foo): > > c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1067): error > C2512: 'base::JsonPrefStoreCallbackTest': no appropriate default constructor > available > c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1033): note: > see declaration of 'base::JsonPrefStoreCallbackTest' You'll have to give the test an explicit default constructor in that case, e.g. JsonPrefStoreCallbackTest() { }
Latest approach in JsonPrefStore looks great :-) https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.cc:342: calling_store->has_pending_write_callback_ = false; On 2016/09/19 21:24:25, proberge wrote: > On 2016/09/19 18:01:30, gab wrote: > > Member state can only be modified on the owning sequence. This callback runs > > off-sequence and as such touching member state without synchronization (e.g. > > locks) is racy -- and I don't think we want to add explicit synchronization. > > > > I'm trying to think of the best way to achieve what we want here but will post > > these comments before that to give you some head way (or let you find the best > > way before I get to it :-)). > > Hmm. Not sure if this is better than locks. PTAL. Yes, much better than locks :-), well done! > (static instead of member method as it sounds like trouble if the JsonPrefStore > dies after the FileWriter thread PostTasks. > https://cs.chromium.org/chromium/src/docs/callback.md?dr=Ss&q=callback%5C.md&...) https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.h:120: void RegisterOnNextWriteCallback( On 2016/09/19 21:24:25, proberge wrote: > On 2016/09/19 18:01:30, gab wrote: > > RegisterOnNextWriteSynchronousCallback > > I don't think RegisterOnNextWriteSynchronousCallback is the right name. > > 1. SynchronousCallback doesn't seem to be the standard way of naming it: there's > no codesearch result for a method signature involving Register, Synchronous and > Callback. Well this use case is very much an edge case, I wouldn't expect this to be done elsewhere really. > 2. The callback isn't synchronous from the JsonPrefStore's point of view, but > rather from the file writer. > 3. Does it matter to the caller whether it's synchronous or not? As long as > |on_next_write_callback| is thread safe, does it matter on which thread it's > run? It does matter that it's synchronous, i.e. the very user you're about to introduce cares because if it's posted anywhere instead of being executed synchronously after the write then it's not guaranteed to run on shutdown. Happy to field other naming suggestions, but I think it's core to this API that it be synchronous (and therefore adding it in the name sounds best to me). https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1050: scoped_refptr<SequencedTaskRunner> task_runner) { On 2016/09/19 21:24:26, proberge wrote: > On 2016/09/19 18:01:30, gab wrote: > > Unused? Should there be a test with an independent thread using this though? > > I thought I needed it for TestPostWriteCallbackDuringProfileDeath, but ended up > not using it. > > Any suggestion on what a test with an independent thread would do? I guess it wouldn't do much more than re-test ImportantFileWriter. The important part is that the shutdown test works so what you have here is fine :-) https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1075: }; On 2016/09/19 21:24:26, proberge wrote: > On 2016/09/19 18:01:30, gab wrote: > > DISALLOW_COPY_AND_ASSIGN > > Looks like DISALLOW_COPY_AND_ASSIGN isn't happy with using > TEST_F(JsonPrefStoreCallbackTest, foo): > > c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1067): error > C2512: 'base::JsonPrefStoreCallbackTest': no appropriate default constructor > available > c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1033): note: > see declaration of 'base::JsonPrefStoreCallbackTest' This is because DISALLOW_COPY_AND_ASSIGN is implemented by making the copy constructor private. This has the side-effect of deleting the implicit default constructor. As such you need to explicitly add JsonPrefStoreCallbackTest() = default; at the top of this class' public section. Also note: in the old Chromium days, it was common for people not to put DISALLOW_COPY_AND_ASSIGN in test fixtures, the rules have since been clarified to enforce it everywhere (that's why you'll see old code that doesn't do this still today but be told to do it in new code that merely mimics it) https://codereview.chromium.org/2299523003/diff/260001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/260001/base/files/important_f... base/files/important_file_writer_unittest.cc:187: EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); This is racy and you will need a WaitableEvent::Wait task queued before calling WriteNow(). Otherwise tasks posted by WriteNow() can execute on the other thread at any point between WriteNow() and the end of FlushForTesting(). https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:352: base::WeakPtr<JsonPrefStore> calling_store, Since this method should be using member state, it feels scary to take this as a parameter (even though it's only being forward in practice which is okay). Instead take a const base::Callback<void(bool success)>& on_next_write_reply, then the WeakPtr can be bound into it ahead of time and hidden from this static method entirely (Bind supports partial binding). (and probably s/on_next_write/on_next_write_callback/ ? https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:368: const base::Closure& on_next_successful_write_reply) { DCHECK(CalledOnValidThread()); https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:374: if (!has_pending_write_callback_) { Add a meta comment here (or elsewhere but this seems like a good place) on how this whole dance is performed (it does work, but it took me some time to understand why it does). https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:383: const base::Callback<void(bool success)>& on_next_write_callback) { DCHECK(CalledOnValidThread()); https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:389: base::Bind(&PostWriteCallback, this->AsWeakPtr(), on_next_write_callback, AsWeakPtr() (no need to explicitly add "this->") https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.h:187: // If |write_success| is true, runs |on_next_successful_write_| on the thread s/thread/sequence/ https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.h:191: base::WeakPtr<JsonPrefStore> calling_store, Make this a member method, essentially what you are doing is the same thing as a member method with the |this| pointer bound as a WeakPtr which Bind() allows (i.e. the first parameter of a member method is always the class instance which Bind allows to be a WeakPtr). https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.h:195: // |on_next_write| callback on the current thread and calls |PostWriteReply| s/calls/posts/ Also, what's |PostWriteReply|? https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:962: SuccessfulWriteReplyObserver() {} s/{}/= default;/ https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1001: WriteCallbackObserver() {} = default
https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.h:117: // Registers |on_next_write_callback| to be called once, on the On 2016/09/19 18:01:30, gab wrote: > ... to be called once, synchronously, on the ... Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store.h:120: void RegisterOnNextWriteCallback( On 2016/09/20 01:27:41, gab wrote: > On 2016/09/19 21:24:25, proberge wrote: > > On 2016/09/19 18:01:30, gab wrote: > > > RegisterOnNextWriteSynchronousCallback > > > > I don't think RegisterOnNextWriteSynchronousCallback is the right name. > > > > 1. SynchronousCallback doesn't seem to be the standard way of naming it: > there's > > no codesearch result for a method signature involving Register, Synchronous > and > > Callback. > > Well this use case is very much an edge case, I wouldn't expect this to be done > elsewhere really. > > > 2. The callback isn't synchronous from the JsonPrefStore's point of view, but > > rather from the file writer. > > 3. Does it matter to the caller whether it's synchronous or not? As long as > > |on_next_write_callback| is thread safe, does it matter on which thread it's > > run? > > It does matter that it's synchronous, i.e. the very user you're about to > introduce cares because if it's posted anywhere instead of being executed > synchronously after the write then it's not guaranteed to run on shutdown. > > Happy to field other naming suggestions, but I think it's core to this API that > it be synchronous (and therefore adding it in the name sounds best to me). Done. https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/240001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1075: }; On 2016/09/20 01:27:41, gab wrote: > On 2016/09/19 21:24:26, proberge wrote: > > On 2016/09/19 18:01:30, gab wrote: > > > DISALLOW_COPY_AND_ASSIGN > > > > Looks like DISALLOW_COPY_AND_ASSIGN isn't happy with using > > TEST_F(JsonPrefStoreCallbackTest, foo): > > > > c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1067): error > > C2512: 'base::JsonPrefStoreCallbackTest': no appropriate default constructor > > available > > c:\src\chromium\src\components\prefs\json_pref_store_unittest.cc(1033): note: > > see declaration of 'base::JsonPrefStoreCallbackTest' > > This is because DISALLOW_COPY_AND_ASSIGN is implemented by making the copy > constructor private. This has the side-effect of deleting the implicit default > constructor. As such you need to explicitly add > > JsonPrefStoreCallbackTest() = default; > > at the top of this class' public section. > > > Also note: in the old Chromium days, it was common for people not to put > DISALLOW_COPY_AND_ASSIGN in test fixtures, the rules have since been clarified > to enforce it everywhere (that's why you'll see old code that doesn't do this > still today but be told to do it in new code that merely mimics it) Done. https://codereview.chromium.org/2299523003/diff/260001/base/files/important_f... File base/files/important_file_writer_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/260001/base/files/important_f... base/files/important_file_writer_unittest.cc:187: EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState()); On 2016/09/20 01:27:41, gab wrote: > This is racy and you will need a WaitableEvent::Wait task queued before calling > WriteNow(). Otherwise tasks posted by WriteNow() can execute on the other thread > at any point between WriteNow() and the end of FlushForTesting(). I don't see it: WriteNow posts a task on the file_writer_thread. If we don't run FlushForTesting, the callback hasn't been posted to any task runners. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:352: base::WeakPtr<JsonPrefStore> calling_store, On 2016/09/20 01:27:42, gab wrote: > Since this method should be using member state, it feels scary to take this as a > parameter (even though it's only being forward in practice which is okay). > > Instead take a > const base::Callback<void(bool success)>& on_next_write_reply, > then the WeakPtr can be bound into it ahead of time and hidden from this static > method entirely (Bind supports partial binding). > > (and probably s/on_next_write/on_next_write_callback/ ? > Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:368: const base::Closure& on_next_successful_write_reply) { On 2016/09/20 01:27:41, gab wrote: > DCHECK(CalledOnValidThread()); Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:374: if (!has_pending_write_callback_) { On 2016/09/20 01:27:42, gab wrote: > Add a meta comment here (or elsewhere but this seems like a good place) on how > this whole dance is performed (it does work, but it took me some time to > understand why it does). Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:383: const base::Callback<void(bool success)>& on_next_write_callback) { On 2016/09/20 01:27:42, gab wrote: > DCHECK(CalledOnValidThread()); Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.cc:389: base::Bind(&PostWriteCallback, this->AsWeakPtr(), on_next_write_callback, On 2016/09/20 01:27:41, gab wrote: > AsWeakPtr() > > (no need to explicitly add "this->") Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.h:187: // If |write_success| is true, runs |on_next_successful_write_| on the thread On 2016/09/20 01:27:42, gab wrote: > s/thread/sequence/ Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.h:191: base::WeakPtr<JsonPrefStore> calling_store, On 2016/09/20 01:27:42, gab wrote: > Make this a member method, essentially what you are doing is the same thing as a > member method with the |this| pointer bound as a WeakPtr which Bind() allows > (i.e. the first parameter of a member method is always the class instance which > Bind allows to be a WeakPtr). Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store.h:195: // |on_next_write| callback on the current thread and calls |PostWriteReply| On 2016/09/20 01:27:42, gab wrote: > s/calls/posts/ > > Also, what's |PostWriteReply|? My bad, had renamed PostWriteReply -> RunOrScheduleNextSuccessfulWriteCallback and forgot to update this comment. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... File components/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:962: SuccessfulWriteReplyObserver() {} On 2016/09/20 01:27:42, gab wrote: > s/{}/= default;/ Done. https://codereview.chromium.org/2299523003/diff/260001/components/prefs/json_... components/prefs/json_pref_store_unittest.cc:1001: WriteCallbackObserver() {} On 2016/09/20 01:27:42, gab wrote: > = default Done.
lgtm w/ nits, this is great thanks :-D! https://codereview.chromium.org/2299523003/diff/280001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/280001/components/prefs/json_... components/prefs/json_pref_store.cc:350: if (!on_next_write_callback.is_null()) { nit: no {} for single-line conditional + single-line body https://codereview.chromium.org/2299523003/diff/280001/components/prefs/json_... components/prefs/json_pref_store.cc:373: // it is run even if Chrome is shutting down. I meant more something like: // If there already is a pending callback, there will also already be a matching pending reply. Otherwise, setup a reply with an empty callback. (i.e. why this works is based on how things a bit further work and this is what the comment should explain, not textually mention what's happening right here)
https://codereview.chromium.org/2299523003/diff/280001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2299523003/diff/280001/components/prefs/json_... components/prefs/json_pref_store.cc:350: if (!on_next_write_callback.is_null()) { On 2016/09/20 17:30:12, gab wrote: > nit: no {} for single-line conditional + single-line body Done. https://codereview.chromium.org/2299523003/diff/280001/components/prefs/json_... components/prefs/json_pref_store.cc:373: // it is run even if Chrome is shutting down. On 2016/09/20 17:30:12, gab wrote: > I meant more something like: > > // If there already is a pending callback, there will also already be a matching > pending reply. Otherwise, setup a reply with an empty callback. > > (i.e. why this works is based on how things a bit further work and this is what > the comment should explain, not textually mention what's happening right here) Done.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2299523003/#ps300001 (title: "Fix last nits")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2299523003/#ps320001 (title: "Move variable declaration down to fix 'initialized after field' warning")
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
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...)
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2299523003/#ps340001 (title: "more re-ordering")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by proberge@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by proberge@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2299523003/diff/340001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2299523003/diff/340001/components/prefs/json_... components/prefs/json_pref_store.h:120: void RegisterOnNextWriteSynchronousCallback( Just realized from https://codereview.chromium.org/2204943002/diff/200001/components/prefs/json_..., this should probably be private, right? The API change required is not that this is a public API but that the Callback returned from FilterSerializeData will be invoked synchronously after the write. I'm happy to see this in a CL between this one and https://codereview.chromium.org/2204943002/ if you'd prefer to land this one first. (i.e. a CL that introduces the change to FilterSerializeData's return value and tests this through it instead of directly).
On 2016/09/21 17:49:59, gab wrote: > https://codereview.chromium.org/2299523003/diff/340001/components/prefs/json_... > File components/prefs/json_pref_store.h (right): > > https://codereview.chromium.org/2299523003/diff/340001/components/prefs/json_... > components/prefs/json_pref_store.h:120: void > RegisterOnNextWriteSynchronousCallback( > Just realized from > https://codereview.chromium.org/2204943002/diff/200001/components/prefs/json_..., > this should probably be private, right? > > The API change required is not that this is a public API but that the Callback > returned from FilterSerializeData will be invoked synchronously after the write. > > I'm happy to see this in a CL between this one and > https://codereview.chromium.org/2204943002/ if you'd prefer to land this one > first. (i.e. a CL that introduces the change to FilterSerializeData's return > value and tests this through it instead of directly). Good point. If the commit queue fails again I'll change it in this CL, otherwise I'll do it in a follow-up CL.
Message was sent while issue was closed.
Description was changed from ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. BUG=624858 ========== to ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. BUG=624858 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. BUG=624858 ========== to ========== Add synchronous callback support to important_file_writer.cc This is required to support post-write callbacks which need to execute even if Chrome is shutting down. Carved out of https://codereview.chromium.org/2204943002/, new call will be used there. BUG=624858 Committed: https://crrev.com/fc46ac15bce89405e0bfff3e246be0111f466318 Cr-Commit-Position: refs/heads/master@{#420102} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/fc46ac15bce89405e0bfff3e246be0111f466318 Cr-Commit-Position: refs/heads/master@{#420102} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
