[Chromecast] Move SynchronizedMinidumpManager ratelimit logic to child classes.

[Chromecast] Move SynchronizedMinidumpManager ratelimit logic to child classes.

Previously rate limit logic was in SynchronizedMinidumpManager under the
assumption that this was the only producer for the lockfile.

This is not the case, so it makes more sense for ratelimit logic to be
contained in consumers of the lockfile.

TEST=modified tests pass
BUG=internal b/19210655

Committed: https://crrev.com/e2b5d24e3b338f552e27b1fd37c3eade95d436e1
Cr-Commit-Position: refs/heads/master@{#348681}

[bcf_google, 2015-09-08 22:20:08]

bcf@google.com changed reviewers:
+ gunsch@chromium.org, slan@chromium.org

[bcf_google, 2015-09-08 22:22:02]

The max number of dumps allowed in lockfile restriction was removed because this
isn't enforced by external producers. The associated tests have been removed.

[slan, 2015-09-09 19:08:11]

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
File chromecast/crash/linux/minidump_writer_unittest.cc (left):

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/minidump_writer_unittest.cc:124:
TEST_F(MinidumpWriterTest, Write_FailsWhenTooManyDumpsPresent) {
Would you say that all of the test coverage removed here is replaced in
synchronized_minidump_manager_unittests?

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.h (right):

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.h:102: int
IncrementRatelimitPeriodDumps();
This name is confusing to me. Perhaps IncrementNumDumpsInCurrentPeriod()?

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager_unittest.cc (right):

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:82: class
SynchronizedMinidumpManagerUploader : public SynchronizedMinidumpManager {
Please rename to SynchronizedMinidumpManagerConsumer, Fake*Uploader, or
something similar.

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:106: const
std::string& dump_path() { return dump_path_.value(); }
I don't see these methods called?

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:421:
TEST_F(SynchronizedMinidumpManagerTest,
I know that this test was like this before this change, but it seems a little
too complex. Currently it tests 3 bits of functionality:
 * Upload is successful when limit not exceeded.
 * Upload is not successful when limit exceeded.
 * Upload is successful when period rolls over. 

Can this be split into 3 separate unit tests (without being painfully redundant
when testing the third bullet :-) )?

[bcf_google, 2015-09-09 21:20:29]

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
File chromecast/crash/linux/minidump_writer_unittest.cc (left):

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/minidump_writer_unittest.cc:124:
TEST_F(MinidumpWriterTest, Write_FailsWhenTooManyDumpsPresent) {
On 2015/09/09 19:08:10, slan wrote:
> Would you say that all of the test coverage removed here is replaced in
> synchronized_minidump_manager_unittests?

Yes. These tests were actually testing SynchronizedMinidumpManager ratelimit
logic anyways.

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager.h (right):

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager.h:102: int
IncrementRatelimitPeriodDumps();
On 2015/09/09 19:08:11, slan wrote:
> This name is confusing to me. Perhaps IncrementNumDumpsInCurrentPeriod()?

Done.

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
File chromecast/crash/linux/synchronized_minidump_manager_unittest.cc (right):

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:82: class
SynchronizedMinidumpManagerUploader : public SynchronizedMinidumpManager {
On 2015/09/09 19:08:11, slan wrote:
> Please rename to SynchronizedMinidumpManagerConsumer, Fake*Uploader, or
> something similar.

Done.

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:106: const
std::string& dump_path() { return dump_path_.value(); }
On 2015/09/09 19:08:11, slan wrote:
> I don't see these methods called?

Okay they were removed.

https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:421:
TEST_F(SynchronizedMinidumpManagerTest,
On 2015/09/09 19:08:11, slan wrote:
> I know that this test was like this before this change, but it seems a little
> too complex. Currently it tests 3 bits of functionality:
>  * Upload is successful when limit not exceeded.
>  * Upload is not successful when limit exceeded.
>  * Upload is successful when period rolls over. 
> 
> Can this be split into 3 separate unit tests (without being painfully
redundant
> when testing the third bullet :-) )?

How does this look?

[bcf_google, 2015-09-09 21:22:08]

>
https://codereview.chromium.org/1310313004/diff/20001/chromecast/crash/linux/...
> chromecast/crash/linux/synchronized_minidump_manager_unittest.cc:421:
> TEST_F(SynchronizedMinidumpManagerTest,
> On 2015/09/09 19:08:11, slan wrote:
> > I know that this test was like this before this change, but it seems a
little
> > too complex. Currently it tests 3 bits of functionality:
> >  * Upload is successful when limit not exceeded.
> >  * Upload is not successful when limit exceeded.
> >  * Upload is successful when period rolls over. 
> > 
> > Can this be split into 3 separate unit tests (without being painfully
> redundant
> > when testing the third bullet :-) )?
> 
> How does this look?

What I mean is do you think my PS4 tests look okay?

[slan, 2015-09-10 22:41:20]

lgtm, thanks for the fixes.

[gunsch, 2015-09-10 22:48:35]

committer-stamp lgtm

[bcf_google, 2015-09-10 23:02:12]

The CQ bit was checked by bcf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-10 23:02:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310313004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310313004/60001

[commit-bot: I haz the power, 2015-09-10 23:44:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-10 23:44:27]

Dry run: This issue passed the CQ dry run.

[bcf_google, 2015-09-14 19:04:33]

The CQ bit was checked by bcf@google.com

[commit-bot: I haz the power, 2015-09-14 19:05:57]

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

[commit-bot: I haz the power, 2015-09-14 19:26:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-14 19:26:59]

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

[commit-bot: I haz the power, 2015-09-23 12:35:39]

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