Implement a persistent storage aggregation counter class.

Implement a persistent storage aggregation counter class.

This class is currently used to aggregate the active daily use time
but can also be used to aggregate other data (e.g., active use time
between crashes) before sending to UMA. Abstracting this in a separate
class also simplifies the daemon unit tests.

An alternative design would store the data on shutdown (but may slow
down shutdown a little). This should do it for now.

BUG=none
TEST=gmerged on device,inspected logs,about:histograms,etc.

[petkov, 2010-06-10 16:08:19]

Once this is in, recording active use time between crashes would need to:

- Initialize a new counter in the daemon and Update it in SetUserActive.
- Send a dbus signal from crash reporter on a crash.
- Listen to the signal in the daemon and Flush the counter.

[sosa, 2010-06-10 20:41:37]

Mostly nits

http://codereview.chromium.org/2731008/diff/2001/3001
File Makefile (right):

http://codereview.chromium.org/2731008/diff/2001/3001#newcode40
Makefile:40: TESTCOUNTER_LIBS = -lgtest -lgtest -lbase -lrt -lpthread
Little bit of gtest redundancy ;)?

http://codereview.chromium.org/2731008/diff/2001/3002
File counter.cc (right):

http://codereview.chromium.org/2731008/diff/2001/3002#newcode30
counter.cc:30: 
Maybe add a comment line showing clear separation between classes.

http://codereview.chromium.org/2731008/diff/2001/3002#newcode51
counter.cc:51: UpdateInternal(/* tag */ 0, /* count */ 0, /* flush */ true);
I'm not sure it's good style to do inline comments like this.  If you do, it
might be more legible if you keep each argument on its own line.

http://codereview.chromium.org/2731008/diff/2001/3002#newcode119
counter.cc:119: if (count > 0) {
Combine to one check at the top of the function.

http://codereview.chromium.org/2731008/diff/2001/3003
File counter.h (right):

http://codereview.chromium.org/2731008/diff/2001/3003#newcode12
counter.h:12: // TaggedCounter maintains a persistent storage (i.e., file)
i.e., a file

http://codereview.chromium.org/2731008/diff/2001/3003#newcode45
counter.h:45: virtual void Update(int tag, int count) = 0;
You may want to add an interface to reset without flushing.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode76
counter.h:76: kRecordNullDirty,  // No current record, persistent storage not
synced.
More like persistent storage is invalid.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode120
counter.h:120: // Updates the cached record given the new tag and count. This
Probably put tag and count with ||'s here.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode126
counter.h:126: // storage and switches the record state to non-dirty.
What's fd?

http://codereview.chromium.org/2731008/diff/2001/3005
File counter_test.cc (right):

http://codereview.chromium.org/2731008/diff/2001/3005#newcode211
counter_test.cc:211: TEST_F(TaggedCounterTest, InitFromFile) {
Should re-init.

http://codereview.chromium.org/2731008/diff/2001/3005#newcode216
counter_test.cc:216: counter_.record_.Init(/* tag */ 0, /* count */ 0);
Why not use counter_init?

http://codereview.chromium.org/2731008/diff/2001/3005#newcode223
counter_test.cc:223: counter_.record_.Init(/* tag */ 0, /* count */ 0);
Shouldn't this be a separate test i.e., FlushOnNewTag

http://codereview.chromium.org/2731008/diff/2001/3005#newcode253
counter_test.cc:253: counter_.Update(/* tag */ 22, /* count */ 0);
Redundant with other new test.

[kmixter1, 2010-06-10 21:59:56]

LGTM

http://codereview.chromium.org/2731008/diff/2001/3002
File counter.cc (right):

http://codereview.chromium.org/2731008/diff/2001/3002#newcode51
counter.cc:51: UpdateInternal(/* tag */ 0, /* count */ 0, /* flush */ true);
On 2010/06/10 20:41:37, sosa wrote:
> I'm not sure it's good style to do inline comments like this.  If you do, it
> might be more legible if you keep each argument on its own line.

I've seen instead:

  UpdateInternal(0,  // tag
                 0,  // count
                 true);  // flush

http://codereview.chromium.org/2731008/diff/2001/3002#newcode63
counter.cc:63: int fd = HANDLE_EINTR(open(filename_, O_RDWR | O_CREAT, S_IRUSR |
S_IWUSR));
Is there any locking to consider - if not, document this?

http://codereview.chromium.org/2731008/diff/2001/3003
File counter.h (right):

http://codereview.chromium.org/2731008/diff/2001/3003#newcode12
counter.h:12: // TaggedCounter maintains a persistent storage (i.e., file)
I'd call it a TaggedAccumulator since it doesn't just increment, but up to you.

[petkov, 2010-06-10 22:03:32]

Thanks for the quick review. Addressed and responded to all comments, I think.
Also, fixed the Reporter callback expectations to use gmock.

Please take a look.

http://codereview.chromium.org/2731008/diff/2001/3001
File Makefile (right):

http://codereview.chromium.org/2731008/diff/2001/3001#newcode40
Makefile:40: TESTCOUNTER_LIBS = -lgtest -lgtest -lbase -lrt -lpthread
On 2010/06/10 20:41:37, sosa wrote:
> Little bit of gtest redundancy ;)?

I really, really wanted to link in gtest :-)
Done.

http://codereview.chromium.org/2731008/diff/2001/3002
File counter.cc (right):

http://codereview.chromium.org/2731008/diff/2001/3002#newcode30
counter.cc:30: 
On 2010/06/10 20:41:37, sosa wrote:
> Maybe add a comment line showing clear separation between classes.

Done. Not sure if this is what you meant.

http://codereview.chromium.org/2731008/diff/2001/3002#newcode51
counter.cc:51: UpdateInternal(/* tag */ 0, /* count */ 0, /* flush */ true);
On 2010/06/10 20:41:37, sosa wrote:
> I'm not sure it's good style to do inline comments like this.  If you do, it
> might be more legible if you keep each argument on its own line.

I'm not sure either :-) I had a couple of reviews so far that passed OK from
this perspective. And style suggests documenting constant literal arguments.
I'll leave it in as is -- maybe it will be addressed in a readability review (in
which case I'd have to change many instances).

http://codereview.chromium.org/2731008/diff/2001/3002#newcode119
counter.cc:119: if (count > 0) {
On 2010/06/10 20:41:37, sosa wrote:
> Combine to one check at the top of the function.

Done. Good point.

http://codereview.chromium.org/2731008/diff/2001/3003
File counter.h (right):

http://codereview.chromium.org/2731008/diff/2001/3003#newcode12
counter.h:12: // TaggedCounter maintains a persistent storage (i.e., file)
On 2010/06/10 20:41:37, sosa wrote:
> i.e., a file

Done.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode45
counter.h:45: virtual void Update(int tag, int count) = 0;
On 2010/06/10 20:41:37, sosa wrote:
> You may want to add an interface to reset without flushing.

Right -- there're other interfaces that may also be useful but I'd like to avoid
adding interfaces before having a client for them. The crash-to-crash time
metric will be a client for the Flush interface.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode76
counter.h:76: kRecordNullDirty,  // No current record, persistent storage not
synced.
On 2010/06/10 20:41:37, sosa wrote:
> More like persistent storage is invalid.

Done.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode120
counter.h:120: // Updates the cached record given the new tag and count. This
On 2010/06/10 20:41:37, sosa wrote:
> Probably put tag and count with ||'s here.

Done.

http://codereview.chromium.org/2731008/diff/2001/3003#newcode126
counter.h:126: // storage and switches the record state to non-dirty.
On 2010/06/10 20:41:37, sosa wrote:
> What's fd?

Done.

http://codereview.chromium.org/2731008/diff/2001/3005
File counter_test.cc (right):

http://codereview.chromium.org/2731008/diff/2001/3005#newcode211
counter_test.cc:211: TEST_F(TaggedCounterTest, InitFromFile) {
On 2010/06/10 20:41:37, sosa wrote:
> Should re-init.
Here? SetUp right before this code and Init happens there.

http://codereview.chromium.org/2731008/diff/2001/3005#newcode216
counter_test.cc:216: counter_.record_.Init(/* tag */ 0, /* count */ 0);
On 2010/06/10 20:41:37, sosa wrote:
> Why not use counter_init?

Done.

http://codereview.chromium.org/2731008/diff/2001/3005#newcode223
counter_test.cc:223: counter_.record_.Init(/* tag */ 0, /* count */ 0);
On 2010/06/10 20:41:37, sosa wrote:
> Shouldn't this be a separate test i.e., FlushOnNewTag

FlushOnNewTag is tested below by "counter_.Update(/* tag */ 21, /* count */
15);"

This whole test is intended to make sure the counter correctly goes through the
cached record states when it's initialized from an already existing file.

http://codereview.chromium.org/2731008/diff/2001/3005#newcode253
counter_test.cc:253: counter_.Update(/* tag */ 22, /* count */ 0);
On 2010/06/10 20:41:37, sosa wrote:
> Redundant with other new test.

Not really, because the cached record state before calling this Update is
different here -- it's Valid here vs. Invalid in the other test.

[petkov, 2010-06-10 22:15:18]

Thanks again for the quick review. Addressed new comments.

http://codereview.chromium.org/2731008/diff/2001/3002
File counter.cc (right):

http://codereview.chromium.org/2731008/diff/2001/3002#newcode51
counter.cc:51: UpdateInternal(/* tag */ 0, /* count */ 0, /* flush */ true);
On 2010/06/10 21:59:57, kmixter1 wrote:
> On 2010/06/10 20:41:37, sosa wrote:
> > I'm not sure it's good style to do inline comments like this.  If you do, it
> > might be more legible if you keep each argument on its own line.
> 
> I've seen instead:
> 
>   UpdateInternal(0,  // tag
>                  0,  // count
>                  true);  // flush
> 
> 

Ok, given that -1 became -2, fixed. I've left the unit tests as is though.

http://codereview.chromium.org/2731008/diff/2001/3002#newcode63
counter.cc:63: int fd = HANDLE_EINTR(open(filename_, O_RDWR | O_CREAT, S_IRUSR |
S_IWUSR));
On 2010/06/10 21:59:57, kmixter1 wrote:
> Is there any locking to consider - if not, document this?

Good point -- I've documented this both here and in the header file. There's no
need to do any locking now; maybe in the future.

http://codereview.chromium.org/2731008/diff/2001/3003
File counter.h (right):

http://codereview.chromium.org/2731008/diff/2001/3003#newcode12
counter.h:12: // TaggedCounter maintains a persistent storage (i.e., file)
On 2010/06/10 21:59:57, kmixter1 wrote:
> I'd call it a TaggedAccumulator since it doesn't just increment, but up to
you.

I called it TaggedAggregator first, then TaggedAccumulator (and my branch is
called accumulator) but at some point renamed it to TaggedCounter to keep the ID
shorter. Hopefully the documentation makes things clear.

[sosa, 2010-06-10 22:52:17]

lgtm