Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1127963002:
Implement lossy pref behavior for JsonPrefStore. (Closed)
|
Created:
5 years, 7 months ago by raymes Modified:
5 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@prefs-fix-flags Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement lossy pref behavior for JsonPrefStore.
Writes to lossy prefs should not trigger writes to disk to be scheduled.
This behavior is implemented for JsonPrefStore. CommitPendingWrites will
ensure that all outstanding lossy writes are also written to disk.
BUG=476800
Committed: https://crrev.com/e9d8c9882ca272c803711e26ded4dd7d5203a59e
Cr-Commit-Position: refs/heads/master@{#329122}
Committed: https://crrev.com/4b6e14e878a0cea357943e248e99279a384256bb
Cr-Commit-Position: refs/heads/master@{#329287}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 8
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 4
Patch Set 13 : #Patch Set 14 : #
Messages
Total messages: 36 (11 generated)
raymes@chromium.org changed reviewers: + mnissler@chromium.org
mnissler: PTAL. This should be the last patch! I will add a test for this before landing, but I wanted to settle on the implementation first. Thanks!!
Approach looks good, happy to stamp this once you have added a test. https://codereview.chromium.org/1127963002/diff/1/base/prefs/json_pref_store.cc File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1127963002/diff/1/base/prefs/json_pref_store.... base/prefs/json_pref_store.cc:457: if (flags & LOSSY_PREF_WRITE_FLAG) { This file doesn't use braces for single-line conditionals, so please drop them here as well.
Thanks Mattias! I added a test. I created an ImportantFileWriter interface so that it could be delegated for testing. https://codereview.chromium.org/1127963002/diff/1/base/prefs/json_pref_store.cc File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1127963002/diff/1/base/prefs/json_pref_store.... base/prefs/json_pref_store.cc:457: if (flags & LOSSY_PREF_WRITE_FLAG) { On 2015/05/06 07:26:04, Mattias Nissler wrote: > This file doesn't use braces for single-line conditionals, so please drop them > here as well. Done.
Thanks for adding tests. As commented inline, I don't think mocking out ImportantFileWriter is really worth the trouble, and we really get more real-life test coverage if we just check whether changes appear on disk or not. Note that the remainder of the tests in json_pref_store_unittest.cc does the same. https://codereview.chromium.org/1127963002/diff/60001/base/prefs/json_pref_st... File base/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/1127963002/diff/60001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:860: struct PrefDetails { This is actually not providing any benefit that I can see? You could just as well inline the constants in the test code below. https://codereview.chromium.org/1127963002/diff/60001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:900: ASSERT_FALSE(file_writer->HasPendingWrite()); Instead of mocking the file writer, you could just spin the message loop using RunLoop.RunUntilIdle() and check whether the write gets flushed out to disk. https://codereview.chromium.org/1127963002/diff/60001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:907: ASSERT_FALSE(file_writer->HasPendingWrite()); Same here, could just spin the loop and check the result.
Thanks Mattias, I don't know what I was thinking. That turned out to be much simpler :)
https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... File base/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:815: struct PrefDetails { This is still here, but I don't see an answer to my previous comment. Can you comment? https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:888: // SetValue/RemoveValue Missing period? https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:934: // Cleanup. Instead of doing a cleanup and starting fresh, why not put the second test sequence in a separate TEST_F()?
Thanks! https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... File base/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:815: struct PrefDetails { I removed this. https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:888: // SetValue/RemoveValue On 2015/05/08 07:04:25, Mattias Nissler wrote: > Missing period? Done. https://codereview.chromium.org/1127963002/diff/80001/base/prefs/json_pref_st... base/prefs/json_pref_store_unittest.cc:934: // Cleanup. On 2015/05/08 07:04:25, Mattias Nissler wrote: > Instead of doing a cleanup and starting fresh, why not put the second test > sequence in a separate TEST_F()? Done.
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127963002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127963002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127963002/160001
LGTM https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... File base/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:897: ASSERT_FALSE(file_writer->HasPendingWrite()); suggestion: blank line before comment https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:901: ASSERT_TRUE(file_writer->HasPendingWrite()); ditto https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:917: ASSERT_TRUE(file_writer->HasPendingWrite()); ditto https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:921: ASSERT_TRUE(file_writer->HasPendingWrite()); ditto
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1127963002/#ps220001 (title: " ")
https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... File base/prefs/json_pref_store_unittest.cc (right): https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:897: ASSERT_FALSE(file_writer->HasPendingWrite()); On 2015/05/11 07:17:10, Mattias Nissler wrote: > suggestion: blank line before comment Done. https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:901: ASSERT_TRUE(file_writer->HasPendingWrite()); On 2015/05/11 07:17:10, Mattias Nissler wrote: > ditto Done. https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:917: ASSERT_TRUE(file_writer->HasPendingWrite()); On 2015/05/11 07:17:10, Mattias Nissler wrote: > ditto Done. https://codereview.chromium.org/1127963002/diff/140001/base/prefs/json_pref_s... base/prefs/json_pref_store_unittest.cc:921: ASSERT_TRUE(file_writer->HasPendingWrite()); On 2015/05/11 07:17:10, Mattias Nissler wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127963002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e9d8c9882ca272c803711e26ded4dd7d5203a59e Cr-Commit-Position: refs/heads/master@{#329122}
Message was sent while issue was closed.
https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... base/prefs/json_pref_store.cc:180: filtering_in_progress_(false), I think you're missing doing "pending_lossy_write_(false)" here. Which is causing the issues on the memory bots.
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1136983004/ by robert.bradford@intel.com. The reason for reverting is: Linux Valgrind & MSan bot went red due to use of an uninitialized value (newly added POD member missing from second constructor.) http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v....
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... base/prefs/json_pref_store.cc:180: filtering_in_progress_(false), On 2015/05/11 17:12:08, robert.bradford wrote: > I think you're missing doing "pending_lossy_write_(false)" here. Which is > causing the issues on the memory bots. https://codereview.chromium.org/1138783003/
Message was sent while issue was closed.
https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... base/prefs/json_pref_store.cc:180: filtering_in_progress_(false), Thanks guys (especially Dan for trying to fix it!!) sorry for the trouble. I've uploaded another CL which merges the two constructors. Hooray C++ 11.
Message was sent while issue was closed.
https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1127963002/diff/220001/base/prefs/json_pref_s... base/prefs/json_pref_store.cc:180: filtering_in_progress_(false), On 2015/05/11 22:59:08, raymes wrote: > Thanks guys (especially Dan for trying to fix it!!) sorry for the trouble. I've > uploaded another CL which merges the two constructors. Hooray C++ 11. til: ^ this is awesome
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1127963002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127963002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/4b6e14e878a0cea357943e248e99279a384256bb Cr-Commit-Position: refs/heads/master@{#329287} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
