Cleanup nits in ImportantFileWriter and JsonPrefStore.

Cleanup nits in ImportantFileWriter and JsonPrefStore.

Committed: https://crrev.com/1433edbcf474f61fdaca42fe46e4c8d4a08ed703
Cr-Commit-Position: refs/heads/master@{#342207}

[Lei Zhang, 2015-08-05 04:40:27]

Patchset #2 (id:20001) has been deleted

[Lei Zhang, 2015-08-05 04:40:33]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-08-05 20:03:12]

thestig@chromium.org changed reviewers:
+ cmumford@chromium.org

[Lei Zhang, 2015-08-05 20:03:13]

I was looking at ImportantFileWriter. Wanted to get the random cleanups out of
the way first.

[cmumford, 2015-08-05 20:45:05]

lgtm % my nits. I'm not a owner though...

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
File base/files/important_file_writer.cc (right):

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.cc:96:
static_cast<size_t>(std::numeric_limits<int32>::max()));
Need to #include <limits>

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.cc:130:
TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)) {
Instead of creating a new constructor why not just add a default parameter value
for interval? Sure, you need to move kDefaultCommitIntervalMs to the header, but
I think will simplify the code.

Your call - keep as is if you feel this is simpler.

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.cc:139: serializer_(nullptr),
If you're going to modify this line, why not just initialize serializer_ in the
header and delete this line?

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.h:68: // Save as above, but with a custom
commit interval.
Did you mean "Same as above..."

[tfarina, 2015-08-05 21:09:54]

tfarina@chromium.org changed reviewers:
+ tfarina@chromium.org

[tfarina, 2015-08-05 21:09:55]

bookmarks lgtm

https://codereview.chromium.org/1265363002/diff/40001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_storage.cc (right):

https://codereview.chromium.org/1265363002/diff/40001/components/bookmarks/br...
components/bookmarks/browser/bookmark_storage.cc:149:
sequenced_task_runner_(sequenced_task_runner),
yay. looks like we missed the opportunity to initialize it in the initializer
list. thanks for fixing this.

[cmumford, 2015-08-05 21:16:56]

https://codereview.chromium.org/1265363002/diff/60001/base/files/important_fi...
File base/files/important_file_writer.cc (right):

https://codereview.chromium.org/1265363002/diff/60001/base/files/important_fi...
base/files/important_file_writer.cc:96:
static_cast<size_t>(std::numeric_limits<int32>::max()));
Oh also - you might want to take a look at base::IsValueInRangeForNumericType()

[Lei Zhang, 2015-08-06 07:13:41]

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
File base/files/important_file_writer.cc (right):

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.cc:96:
static_cast<size_t>(std::numeric_limits<int32>::max()));
On 2015/08/05 20:45:04, cmumford wrote:
> Need to #include <limits>

Went with IsValueInRangeForNumericType() instead.

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.cc:130:
TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)) {
On 2015/08/05 20:45:05, cmumford wrote:
> Instead of creating a new constructor why not just add a default parameter
value
> for interval? Sure, you need to move kDefaultCommitIntervalMs to the header,
but
> I think will simplify the code.
> 
> Your call - keep as is if you feel this is simpler.

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.cc:139: serializer_(nullptr),
On 2015/08/05 20:45:05, cmumford wrote:
> If you're going to modify this line, why not just initialize serializer_ in
the
> header and delete this line?

Class member initializers haven't really caught on in base/. I don't see a
strong advantage to using it, because we'd then need to look at 2 different
places for init code. The ctor above delegates to this one, so we are not
suffering the pain of having to initialize |serializer_| in multiple places.

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/1265363002/diff/40001/base/files/important_fi...
base/files/important_file_writer.h:68: // Save as above, but with a custom
commit interval.
On 2015/08/05 20:45:05, cmumford wrote:
> Did you mean "Same as above..."

Done.

https://codereview.chromium.org/1265363002/diff/60001/base/files/important_fi...
File base/files/important_file_writer.cc (right):

https://codereview.chromium.org/1265363002/diff/60001/base/files/important_fi...
base/files/important_file_writer.cc:96:
static_cast<size_t>(std::numeric_limits<int32>::max()));
On 2015/08/05 21:16:56, cmumford wrote:
> Oh also - you might want to take a look at
base::IsValueInRangeForNumericType()

Good suggestion. That wasn't around when this was first written.

[Lei Zhang, 2015-08-06 18:20:02]

thestig@chromium.org changed reviewers:
+ sky@chromium.org

[Lei Zhang, 2015-08-06 18:20:02]

+sky for components/bookmarks

[sky, 2015-08-06 20:31:02]

LGTM

[Lei Zhang, 2015-08-06 20:39:32]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-08-06 20:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
tfarina@chromium.org, cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1265363002/#ps100001
(title: "self review")

[commit-bot: I haz the power, 2015-08-06 20:41:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265363002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265363002/100001

[commit-bot: I haz the power, 2015-08-06 21:45:35]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-08-06 21:46:09]

Patchset 4 (id:??) landed as
https://crrev.com/1433edbcf474f61fdaca42fe46e4c8d4a08ed703
Cr-Commit-Position: refs/heads/master@{#342207}