Compress and checksum pending logs that are going to be persisted. ...

Compress and checksum pending logs that are going to be persisted. Persisted logs now have the following format:
[list_size, log1, log2, ..., log_n, checksum].

where each log is bzipped before being written. Upon reading the logs from disk, we verify the data and register whether we faced corruptions or not.

r=jar




Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52885

[ziadh, 2010-07-15 01:53:45]

Hi jar,
Please let me know what you think. I'm going to add the histograms in the
meantime.

[jar (doing other things), 2010-07-15 06:05:54]

Thanks for providing the CL! I'm really looking forward to seeing this deployed
in the field. I think it might make it in this week.  

The big missing item is a set of test cases that exercises the new method for
persistence, and validates the error checking.

Additional comments below.

Thanks!

http://codereview.chromium.org/2936005/diff/45001/46002
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/45001/46002#newcode255
chrome/browser/metrics/metrics_service.cc:255: static const size_t
kMaxInitialLogsPersisted = 20 + 2;  // For size and checksum
I don't think you should redefine this value.  It only caused confusion when you
used it later.

I suspect that the number of logs is not changed, but rather the size of the
array (max) must be test for is a bit different.  Hence this additon of 2 should
probably be made closer to that check. If you're concerned about magic numbers,
you could define
static const size_t kChecksumEntryCount
or such.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode266
chrome/browser/metrics/metrics_service.cc:266: static const size_t
kMaxOngoingLogsPersisted = 8;
You need to also support this limitation on transmission of Ongoing logs.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1152
chrome/browser/metrics/metrics_service.cc:1152: return;
It appears that local_list is cleared in all other cases (or set to a resulting
value).  I'd suggest you just clear it before the if statement, and not bother
clearing it in the if statements when you haven't yet appended anything.

There is also a chance that you *might* consider structuring the code to more
easilly handle all the error cleanup at one point (perhaps via a "break" since
we don't have a "finally" construct).  I was just sad to see the cleanup recoded
time and again in so many blocks.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1156
chrome/browser/metrics/metrics_service.cc:1156: list->Clear();
I don't think you need to clear the list, since this is a once-only-call to
extract the values.  I also see this method as an extraction, or access method,
which shouldn't change the source (but only extract a value).  Avoiding the
clear would also allow you to make |list| be a const&.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1167
chrome/browser/metrics/metrics_service.cc:1167: list->Clear();
nit: indentation is too large.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1187
chrome/browser/metrics/metrics_service.cc:1187:
(*it)->GetAsString(&encoded_log);
You probably need to validate the success of GetAsString(), as that will be a
very plausible error case.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1237
chrome/browser/metrics/metrics_service.cc:1237: if (local_list.size() >
kMaxInitialLogsPersisted)
I think you're using kMaxInitialLogsPersisted as it's name suggests (and not
based on your current redefinition).  If you wanted to use the current
re-defintion, then you'd need to compare with kMaxInitialLogsPersisted - 2.

There are actually two distinct constants, one for Intial logs persisted, and
one for Ongoing logs persisted.  I think you need to add a parameter to this
helper function so that you can take the correct one in each case.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1239
chrome/browser/metrics/metrics_service.cc:1239: if (local_list.size() - start ==
0)
I think you meant:
 if (local_list.size() <= start)

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1251
chrome/browser/metrics/metrics_service.cc:1251: if (!Encode(*it, &encoded_log))
{
Do we need to base64 encode things, or does the prefs service do this for us?

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1253
chrome/browser/metrics/metrics_service.cc:1253: continue;
This would-be defensive code actually creates a corrupt persistent list (it will
have the wrong size).  Better might be to clear the output list, and exit.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1289
chrome/browser/metrics/metrics_service.cc:1289:
pending_log_->GetEncodedLog(WriteInto(&pending_log_text_, text_size + 1),
Suggest using a new member variable called compressed_log_, and no longer having
a member called pending_log_text_.
IMO, it is confusing to use this member variable as a temporary here (first you
write in the log text, then you put in the compressed log).

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1299
chrome/browser/metrics/metrics_service.cc:1299: DiscardPendingLog();
Can you really discard the log here?  You need to look at callsite for
PreparePendingLogText() to see if it could handle such semantics.

Perhaps better defensive code would be to just assign an empty string into
pending_log_text_.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1307
chrome/browser/metrics/metrics_service.cc:1307: PreparePendingLogText();
Even this callsite (there are others) can't handle the semantics of having
pendiing_log_text_ being empty.

http://codereview.chromium.org/2936005/diff/45001/46003
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/2936005/diff/45001/46003#newcode271
chrome/browser/metrics/metrics_service.h:271: void
RecallUnsentLogsHelper(ListValue* list,
Can we use a const ListValue&, or is that too different from common usage?  I
noticed you changed the list code, and I would expect it was intended to support
const here?

http://codereview.chromium.org/2936005/diff/45001/46003#newcode497
chrome/browser/metrics/metrics_service.h:497: int list_size_corruption_;
There should probably be comments for this group of members, or possibly for
each one.  I'm guessing they are counters, and wondering why we don't use a
histogram directly.  For example, 

UMA_HISTOGRAM_ENUMERATION(....)

Where the enum listed all possible error cases (including nothing to decode), as
well as a count of "trying" to decode.


...also... I see you gathering this data, but don't see it being written
anywhere (it should presumably go to some histogram anyway).

http://codereview.chromium.org/2936005/diff/45001/46005
File chrome/common/metrics_helpers.h (right):

http://codereview.chromium.org/2936005/diff/45001/46005#newcode214
chrome/common/metrics_helpers.h:214: // version into prefs if we can't transmit
it (after base64-encoding and
The comment is outdated: the contents of this member is already compressed, and
presumably encoded)

[jar (doing other things), 2010-07-16 01:21:46]

This is looking very good.  There are a few nits below, and we need some tests.

http://codereview.chromium.org/2936005/diff/70001/50004
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/70001/50004#newcode204
chrome/browser/metrics/metrics_service.cc:204: inline void
MakeHistogram(MetricsService::LogRecallStatus status) {
nit: undent the function definition, and you should remove the inline keyword. 
It mostly does nothing with modern compilers (that know how to select inline
better than you and I)... and I'm not clear how it would interact with the
static declared in the function.

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1158
chrome/browser/metrics/metrics_service.cc:1158: return;
You should probably also do a MakeHistogram(LIST_EMPTY).

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1170
chrome/browser/metrics/metrics_service.cc:1170: MakeHistogram(OTHER_CORRUPTION);
You may as well be specific.... LIST_SIZE_MISSING

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1192
chrome/browser/metrics/metrics_service.cc:1192: MakeHistogram(OTHER_CORRUPTION);
Perhaps you should use another enum for this, such as NONSTRING_CORRUPTION or
MISSING_LOGSTRING_CORRUPTION

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1250
chrome/browser/metrics/metrics_service.cc:1250: MakeHistogram(LIST_EMPTY);
I think you want the corruption stats when we are recovering the data from a
prefs, so  I think you shouldn't bother to MakeHistogram here.

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1378
chrome/browser/metrics/metrics_service.cc:1378: compressed_log_.length() >
nit: I'd suggest parenthesizing this comparison, which will then give you an
excuse to indent the next line by 4 more characters, and probably make this
easier to visually parse.

http://codereview.chromium.org/2936005/diff/70001/50005
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/2936005/diff/70001/50005#newcode157
chrome/browser/metrics/metrics_service.h:157: COMPRESS_FAIL,         // Failed
to compress log.
COMPRESS_FAIL and ENCODE_FAIL are currently log saving errors (rather than
Recall issues).  Perhaps you shouldn't bother to log them, and they probably
shouldn't be in this enum.

http://codereview.chromium.org/2936005/diff/70001/50005#newcode288
chrome/browser/metrics/metrics_service.h:288: // Convert pending_log_ to XML in
pending_log_text_, and compress it for
typo: |pending_log_text_|  --> |compressed_log_|

http://codereview.chromium.org/2936005/diff/70001/50007
File chrome/common/metrics_helpers.h (right):

http://codereview.chromium.org/2936005/diff/70001/50007#newcode194
chrome/common/metrics_helpers.h:194: // Discard |pending_log_|, and clear
|compressed_pending_log_text_|. Called
typo: we don't have compressed_pending_log_text_

[ziadh, 2010-07-16 20:50:01]

http://codereview.chromium.org/2936005/diff/45001/46002
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/45001/46002#newcode255
chrome/browser/metrics/metrics_service.cc:255: static const size_t
kMaxInitialLogsPersisted = 20 + 2;  // For size and checksum
On 2010/07/15 06:05:54, jar wrote:
> I don't think you should redefine this value.  It only caused confusion when
you
> used it later.
> 
> I suspect that the number of logs is not changed, but rather the size of the
> array (max) must be test for is a bit different.  Hence this additon of 2
should
> probably be made closer to that check. If you're concerned about magic
numbers,
> you could define
> static const size_t kChecksumEntryCount
> or such.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode266
chrome/browser/metrics/metrics_service.cc:266: static const size_t
kMaxOngoingLogsPersisted = 8;
On 2010/07/15 06:05:54, jar wrote:
> You need to also support this limitation on transmission of Ongoing logs.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1152
chrome/browser/metrics/metrics_service.cc:1152: return;
On 2010/07/15 06:05:54, jar wrote:
> It appears that local_list is cleared in all other cases (or set to a
resulting
> value).  I'd suggest you just clear it before the if statement, and not bother
> clearing it in the if statements when you haven't yet appended anything.
> 
> There is also a chance that you *might* consider structuring the code to more
> easilly handle all the error cleanup at one point (perhaps via a "break" since
> we don't have a "finally" construct).  I was just sad to see the cleanup
recoded
> time and again in so many blocks.

Cleanup code is now context-dependent. Lists are not being cleared all the time,
only after they've (possibly) been written to.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1156
chrome/browser/metrics/metrics_service.cc:1156: list->Clear();
On 2010/07/15 06:05:54, jar wrote:
> I don't think you need to clear the list, since this is a once-only-call to
> extract the values.  I also see this method as an extraction, or access
method,
> which shouldn't change the source (but only extract a value).  Avoiding the
> clear would also allow you to make |list| be a const&.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1167
chrome/browser/metrics/metrics_service.cc:1167: list->Clear();
On 2010/07/15 06:05:54, jar wrote:
> nit: indentation is too large.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1187
chrome/browser/metrics/metrics_service.cc:1187:
(*it)->GetAsString(&encoded_log);
On 2010/07/15 06:05:54, jar wrote:
> You probably need to validate the success of GetAsString(), as that will be a
> very plausible error case.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1237
chrome/browser/metrics/metrics_service.cc:1237: if (local_list.size() >
kMaxInitialLogsPersisted)
On 2010/07/15 06:05:54, jar wrote:
> I think you're using kMaxInitialLogsPersisted as it's name suggests (and not
> based on your current redefinition).  If you wanted to use the current
> re-defintion, then you'd need to compare with kMaxInitialLogsPersisted - 2.
> 
> There are actually two distinct constants, one for Intial logs persisted, and
> one for Ongoing logs persisted.  I think you need to add a parameter to this
> helper function so that you can take the correct one in each case.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1239
chrome/browser/metrics/metrics_service.cc:1239: if (local_list.size() - start ==
0)
On 2010/07/15 06:05:54, jar wrote:
> I think you meant:
>  if (local_list.size() <= start)

No longer applicable after introduction of kChecksumEntryCount. I also
introduced a DCHECK that asserts that start <= local_list.size().

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1251
chrome/browser/metrics/metrics_service.cc:1251: if (!Encode(*it, &encoded_log))
{
On 2010/07/15 06:05:54, jar wrote:
> Do we need to base64 encode things, or does the prefs service do this for us?

Turns out that CreateStringValue() expects a valid UTF8 string as an input.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1253
chrome/browser/metrics/metrics_service.cc:1253: continue;
On 2010/07/15 06:05:54, jar wrote:
> This would-be defensive code actually creates a corrupt persistent list (it
will
> have the wrong size).  Better might be to clear the output list, and exit.

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1289
chrome/browser/metrics/metrics_service.cc:1289:
pending_log_->GetEncodedLog(WriteInto(&pending_log_text_, text_size + 1),
On 2010/07/15 06:05:54, jar wrote:
> Suggest using a new member variable called compressed_log_, and no longer
having
> a member called pending_log_text_.
> IMO, it is confusing to use this member variable as a temporary here (first
you
> write in the log text, then you put in the compressed log).

Done.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1299
chrome/browser/metrics/metrics_service.cc:1299: DiscardPendingLog();
On 2010/07/15 06:05:54, jar wrote:
> Can you really discard the log here?  You need to look at callsite for
> PreparePendingLogText() to see if it could handle such semantics.
> 
> Perhaps better defensive code would be to just assign an empty string into
> pending_log_text_.

True. Some callers expect that the string not be empty. I assigned a string
("Unable to compress!") to it. We can then catch this at the server if needed.

http://codereview.chromium.org/2936005/diff/45001/46002#newcode1307
chrome/browser/metrics/metrics_service.cc:1307: PreparePendingLogText();
On 2010/07/15 06:05:54, jar wrote:
> Even this callsite (there are others) can't handle the semantics of having
> pendiing_log_text_ being empty.

Done.

http://codereview.chromium.org/2936005/diff/45001/46003
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/2936005/diff/45001/46003#newcode271
chrome/browser/metrics/metrics_service.h:271: void
RecallUnsentLogsHelper(ListValue* list,
On 2010/07/15 06:05:54, jar wrote:
> Can we use a const ListValue&, or is that too different from common usage?  I
> noticed you changed the list code, and I would expect it was intended to
support
> const here?

I changed RecallUnsentLogsHelper() to not modify the ListValue* parameter, and
converted it to a const ListValue&.

http://codereview.chromium.org/2936005/diff/45001/46003#newcode497
chrome/browser/metrics/metrics_service.h:497: int list_size_corruption_;
On 2010/07/15 06:05:54, jar wrote:
> There should probably be comments for this group of members, or possibly for
> each one.  I'm guessing they are counters, and wondering why we don't use a
> histogram directly.  For example, 
> 
> UMA_HISTOGRAM_ENUMERATION(....)
> 
> Where the enum listed all possible error cases (including nothing to decode),
as
> well as a count of "trying" to decode.
> 
> 
> ...also... I see you gathering this data, but don't see it being written
> anywhere (it should presumably go to some histogram anyway).

Done.

http://codereview.chromium.org/2936005/diff/45001/46005
File chrome/common/metrics_helpers.h (right):

http://codereview.chromium.org/2936005/diff/45001/46005#newcode214
chrome/common/metrics_helpers.h:214: // version into prefs if we can't transmit
it (after base64-encoding and
On 2010/07/15 06:05:54, jar wrote:
> The comment is outdated: the contents of this member is already compressed,
and
> presumably encoded)

Done.

http://codereview.chromium.org/2936005/diff/70001/50004
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/70001/50004#newcode204
chrome/browser/metrics/metrics_service.cc:204: inline void
MakeHistogram(MetricsService::LogRecallStatus status) {
On 2010/07/16 01:21:46, jar wrote:
> nit: undent the function definition, and you should remove the inline keyword.

> It mostly does nothing with modern compilers (that know how to select inline
> better than you and I)... and I'm not clear how it would interact with the
> static declared in the function.

Done.

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1158
chrome/browser/metrics/metrics_service.cc:1158: return;
On 2010/07/16 01:21:46, jar wrote:
> You should probably also do a MakeHistogram(LIST_EMPTY).

Done.

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1170
chrome/browser/metrics/metrics_service.cc:1170: MakeHistogram(OTHER_CORRUPTION);
On 2010/07/16 01:21:46, jar wrote:
> You may as well be specific.... LIST_SIZE_MISSING

Done.

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1192
chrome/browser/metrics/metrics_service.cc:1192: MakeHistogram(OTHER_CORRUPTION);
On 2010/07/16 01:21:46, jar wrote:
> Perhaps you should use another enum for this, such as NONSTRING_CORRUPTION or
> MISSING_LOGSTRING_CORRUPTION

Done.

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1250
chrome/browser/metrics/metrics_service.cc:1250: MakeHistogram(LIST_EMPTY);
On 2010/07/16 01:21:46, jar wrote:
> I think you want the corruption stats when we are recovering the data from a
> prefs, so  I think you shouldn't bother to MakeHistogram here.

Oops... I put it here instead of in RecallUnsentLogs() by mistake :-)

http://codereview.chromium.org/2936005/diff/70001/50004#newcode1378
chrome/browser/metrics/metrics_service.cc:1378: compressed_log_.length() >
On 2010/07/16 01:21:46, jar wrote:
> nit: I'd suggest parenthesizing this comparison, which will then give you an
> excuse to indent the next line by 4 more characters, and probably make this
> easier to visually parse.

Done.

http://codereview.chromium.org/2936005/diff/70001/50005
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/2936005/diff/70001/50005#newcode157
chrome/browser/metrics/metrics_service.h:157: COMPRESS_FAIL,         // Failed
to compress log.
On 2010/07/16 01:21:46, jar wrote:
> COMPRESS_FAIL and ENCODE_FAIL are currently log saving errors (rather than
> Recall issues).  Perhaps you shouldn't bother to log them, and they probably
> shouldn't be in this enum.

Done.

http://codereview.chromium.org/2936005/diff/70001/50005#newcode288
chrome/browser/metrics/metrics_service.h:288: // Convert pending_log_ to XML in
pending_log_text_, and compress it for
On 2010/07/16 01:21:46, jar wrote:
> typo: |pending_log_text_|  --> |compressed_log_|

Done.

http://codereview.chromium.org/2936005/diff/70001/50007
File chrome/common/metrics_helpers.h (right):

http://codereview.chromium.org/2936005/diff/70001/50007#newcode194
chrome/common/metrics_helpers.h:194: // Discard |pending_log_|, and clear
|compressed_pending_log_text_|. Called
On 2010/07/16 01:21:46, jar wrote:
> typo: we don't have compressed_pending_log_text_

Done.

[jar (doing other things), 2010-07-16 22:09:53]

You should probably break the tests into separate functions.  It was (for
example) hard to read teh test where the list was truncated, as I needed to
scroll up to the start of the mega-test to see what the current MaxListSize was.

http://codereview.chromium.org/2936005/diff/83001/62003
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/83001/62003#newcode1188
chrome/browser/metrics/metrics_service.cc:1188:
MakeRecallStatusHistogram(LIST_SIZE_CORRUPTION);
You might as well make this enum distinct from the one on line 1170.

http://codereview.chromium.org/2936005/diff/83001/62005
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/2936005/diff/83001/62005#newcode115
chrome/browser/metrics/metrics_service_unittest.cc:115: EXPECT_EQ(kMaxListSize,
local_list.size());
It would be good to check that the truncation is at the start of the list.  For
example, you could at least check that the last element of local_list was still
"four".

If you had a slightly larger exampele (5 elements? 6 elements?) you could check
that the last first element of the result was as expected as well.

http://codereview.chromium.org/2936005/diff/83001/62005#newcode133
chrome/browser/metrics/metrics_service_unittest.cc:133:
//MetricsService::RecallUnsentLogsHelper(list, &local_list);
You need your death test, or a way to calm the DCHECKS, or you should remove the
DCHECKs.

http://codereview.chromium.org/2936005/diff/83001/62005#newcode146
chrome/browser/metrics/metrics_service_unittest.cc:146: ASSERT_TRUE(list.Set(0,
Value::CreateIntegerValue(2)));  // XXX(ziadh): Memory leak?
If you had a test class, the preamble could create the list, and the destructor
could gracefully delete the list and its elements.

http://codereview.chromium.org/2936005/diff/83001/62005#newcode164
chrome/browser/metrics/metrics_service_unittest.cc:164: ASSERT_TRUE(list.Set(2,
Value::CreateStringValue("c228c1d424636e1b6d6e0030e08c8ad6")));  // XXX(ziadh):
Memory leak?
You seem to have "know" the string result.  You could also just pull out the
existing checksum, change one character, and write it back.

I think that these list *might* be lists of smart pointers, so you might not
have a leak.  I don't think the try-bots would spot the leak for you.

[jar (doing other things), 2010-07-17 01:10:52]

Please break the tests up into smaller blocks, and try to use EXPECT as much as
possible instead of ASSERT.

http://codereview.chromium.org/2936005/diff/70003/83005
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/2936005/diff/70003/83005#newcode10
chrome/browser/metrics/metrics_service_unittest.cc:10: #include "base/md5.h"
nit: alphabetize.

http://codereview.chromium.org/2936005/diff/70003/83005#newcode52
chrome/browser/metrics/metrics_service_unittest.cc:52: 
nit: remove line

http://codereview.chromium.org/2936005/diff/70003/83005#newcode78
chrome/browser/metrics/metrics_service_unittest.cc:78: // [1, B64En("Hello
world!"), MD5("Hello world!")].
nit: remove abbreviation Base64Encode(...

http://codereview.chromium.org/2936005/diff/70003/83005#newcode85
chrome/browser/metrics/metrics_service_unittest.cc:85: ASSERT_EQ(1, size);
Should be an EXPECT in most cases, unless you really need ASSERT.

http://codereview.chromium.org/2936005/diff/70003/83008
File chrome_frame/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/70003/83008#newcode448
chrome_frame/metrics_service.cc:448: std::string compressed_log;
nit: remove line.

[ziadh, 2010-07-17 01:21:24]

http://codereview.chromium.org/2936005/diff/83001/62003
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/83001/62003#newcode1188
chrome/browser/metrics/metrics_service.cc:1188:
MakeRecallStatusHistogram(LIST_SIZE_CORRUPTION);
On 2010/07/16 22:09:53, jar wrote:
> You might as well make this enum distinct from the one on line 1170.

Done.

http://codereview.chromium.org/2936005/diff/83001/62005
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/2936005/diff/83001/62005#newcode115
chrome/browser/metrics/metrics_service_unittest.cc:115: EXPECT_EQ(kMaxListSize,
local_list.size());
On 2010/07/16 22:09:53, jar wrote:
> It would be good to check that the truncation is at the start of the list. 
For
> example, you could at least check that the last element of local_list was
still
> "four".
> 
> If you had a slightly larger exampele (5 elements? 6 elements?) you could
check
> that the last first element of the result was as expected as well.

Done.

http://codereview.chromium.org/2936005/diff/83001/62005#newcode133
chrome/browser/metrics/metrics_service_unittest.cc:133:
//MetricsService::RecallUnsentLogsHelper(list, &local_list);
On 2010/07/16 22:09:53, jar wrote:
> You need your death test, or a way to calm the DCHECKS, or you should remove
the
> DCHECKs.

Done.

http://codereview.chromium.org/2936005/diff/83001/62005#newcode164
chrome/browser/metrics/metrics_service_unittest.cc:164: ASSERT_TRUE(list.Set(2,
Value::CreateStringValue("c228c1d424636e1b6d6e0030e08c8ad6")));  // XXX(ziadh):
Memory leak?
On 2010/07/16 22:09:53, jar wrote:
> You seem to have "know" the string result.  You could also just pull out the
> existing checksum, change one character, and write it back.
> 
> I think that these list *might* be lists of smart pointers, so you might not
> have a leak.  I don't think the try-bots would spot the leak for you.

Done.

[ziadh, 2010-07-18 01:53:45]

http://codereview.chromium.org/2936005/diff/70003/83005
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/2936005/diff/70003/83005#newcode10
chrome/browser/metrics/metrics_service_unittest.cc:10: #include "base/md5.h"
On 2010/07/17 01:10:52, jar wrote:
> nit: alphabetize.

Done.

http://codereview.chromium.org/2936005/diff/70003/83005#newcode52
chrome/browser/metrics/metrics_service_unittest.cc:52: 
On 2010/07/17 01:10:52, jar wrote:
> nit: remove line

Done.

http://codereview.chromium.org/2936005/diff/70003/83005#newcode78
chrome/browser/metrics/metrics_service_unittest.cc:78: // [1, B64En("Hello
world!"), MD5("Hello world!")].
On 2010/07/17 01:10:52, jar wrote:
> nit: remove abbreviation Base64Encode(...

Done.

http://codereview.chromium.org/2936005/diff/70003/83005#newcode85
chrome/browser/metrics/metrics_service_unittest.cc:85: ASSERT_EQ(1, size);
On 2010/07/17 01:10:52, jar wrote:
> Should be an EXPECT in most cases, unless you really need ASSERT.

Done.

http://codereview.chromium.org/2936005/diff/70003/83008
File chrome_frame/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/70003/83008#newcode448
chrome_frame/metrics_service.cc:448: std::string compressed_log;
On 2010/07/17 01:10:52, jar wrote:
> nit: remove line.

Done.

[jar (doing other things), 2010-07-19 07:08:03]

LGTM

...but please have a look at the nits below

http://codereview.chromium.org/2936005/diff/76004/59009
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/76004/59009#newcode1167
chrome/browser/metrics/metrics_service.cc:1167: return
MakeRecallStatusHistogram(LIST_EMPTY);
IMO, I like the chaining that allows you to put this on one line ;-).

nit: Since it falls on one line, you don't need the curlies.  This is generally
a consistency thing... but I checked much of the rest of the file, and I *think*
this file tends to leave out the curlies when there is a one line consequence in
an if statement.

http://codereview.chromium.org/2936005/diff/76004/59011
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/2936005/diff/76004/59011#newcode87
chrome/browser/metrics/metrics_service_unittest.cc:87: int size;
nit: initialize size so that IF the GetAsInteger failed, then the EXPECT that
follows will fail (even if size is not changed by the call).

http://codereview.chromium.org/2936005/diff/76004/59014
File chrome/common/metrics_helpers.h (right):

http://codereview.chromium.org/2936005/diff/76004/59014#newcode224
chrome/common/metrics_helpers.h:224: #endif  // CHROME_COMMON_METRICS_HELPERS_H_
Be sure you didn't really remove this ending line from the file.

[ziadh, 2010-07-19 07:39:39]

http://codereview.chromium.org/2936005/diff/76004/59009
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2936005/diff/76004/59009#newcode1167
chrome/browser/metrics/metrics_service.cc:1167: return
MakeRecallStatusHistogram(LIST_EMPTY);
On 2010/07/19 07:08:03, jar wrote:
> IMO, I like the chaining that allows you to put this on one line ;-).
> 
> nit: Since it falls on one line, you don't need the curlies.  This is
generally
> a consistency thing... but I checked much of the rest of the file, and I
*think*
> this file tends to leave out the curlies when there is a one line consequence
in
> an if statement.

Done.

http://codereview.chromium.org/2936005/diff/76004/59011
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/2936005/diff/76004/59011#newcode87
chrome/browser/metrics/metrics_service_unittest.cc:87: int size;
On 2010/07/19 07:08:03, jar wrote:
> nit: initialize size so that IF the GetAsInteger failed, then the EXPECT that
> follows will fail (even if size is not changed by the call).

Done.

http://codereview.chromium.org/2936005/diff/76004/59014
File chrome/common/metrics_helpers.h (right):

http://codereview.chromium.org/2936005/diff/76004/59014#newcode224
chrome/common/metrics_helpers.h:224: #endif  // CHROME_COMMON_METRICS_HELPERS_H_
On 2010/07/19 07:08:03, jar wrote:
> Be sure you didn't really remove this ending line from the file.

There were 2 extra blank lines. I removed one :)

[stuartmorgan, 2011-03-17 21:15:54]

http://codereview.chromium.org/2936005/diff/63005/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/2936005/diff/63005/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:100: // on.
Is this experiment still going on? 8 months is a long "temporarily"
(There's a lot of code in this class; if this isn't still being used it would be
nice to take it out and make things that much simpler.)