[Cleanup] Remove a param used to customize test behavior.

[Cleanup] Remove a param used to customize test behavior.

BUG=694884
TEST=none
R=tobiasjs@chromium.org

Review-Url: https://codereview.chromium.org/2721193002
Cr-Commit-Position: refs/heads/master@{#453958}
Committed: https://chromium.googlesource.com/chromium/src/+/1201b57d3a5b9d94ca34835b27c714bbc9b41fb6

[Tobias Sargeant, 2017-03-01 08:13:41]

https://codereview.chromium.org/2721193002/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2721193002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:187:
// our copying mechanism in the sense that it keeps less minidumps.
/less/fewer/

Also, this code seems not to be apropos anything in particular. Is it important?

https://codereview.chromium.org/2721193002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:188:
mFileManager.cleanOutAllNonFreshMinidumpFiles();
I can see the logic behind not having a for-testing flag passed to the
constructor of this code, but I think that this code is now harder to reason
about, because the name implies that the cleaning always happens. I would be
surprised to learn that there was ever a case where 'non fresh' minidumps
remained after this call - there's no indication that a serived class might
modify the behaviour.

[Ilya Sherman, 2017-03-01 08:31:12]

https://codereview.chromium.org/2721193002/diff/1/android_webview/java/src/or...
File
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java
(right):

https://codereview.chromium.org/2721193002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:187:
// our copying mechanism in the sense that it keeps less minidumps.
On 2017/03/01 08:13:41, Tobias Sargeant wrote:
> /less/fewer/

Done.

> Also, this code seems not to be apropos anything in particular. Is it
important?

Mmm, I think the idea is that we've uploaded everything we're going to upload,
so we might as well do some cleanup.  I'm not sure whether Webview has other
times that this cleanup might happen; Chrome runs it on startup, after some
delay.  Perhaps Gustav has some further comments?

https://codereview.chromium.org/2721193002/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/crash/MinidumpUploaderImpl.java:188:
mFileManager.cleanOutAllNonFreshMinidumpFiles();
On 2017/03/01 08:13:41, Tobias Sargeant wrote:
> I can see the logic behind not having a for-testing flag passed to the
> constructor of this code, but I think that this code is now harder to reason
> about, because the name implies that the cleaning always happens. I would be
> surprised to learn that there was ever a case where 'non fresh' minidumps
> remained after this call - there's no indication that a serived class might
> modify the behaviour.

A derived class could, in theory, override this whole run() method.  I don't
think it's really that surprising that test code might want to mock out side
effects -- is it?

[Ilya Sherman, 2017-03-01 08:36:12]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 08:36:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 08:38:39]

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

[commit-bot: I haz the power, 2017-03-01 08:38:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ilya Sherman, 2017-03-01 09:19:32]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 09:19:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-03-01 09:21:49]

The CQ bit was checked by isherman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 09:22:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 09:43:25]

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

[commit-bot: I haz the power, 2017-03-01 09:43:26]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2017-03-01 15:56:46]

On 2017/03/01 08:31:12, Ilya Sherman wrote:
> A derived class could, in theory, override this whole run() method.  I don't
> think it's really that surprising that test code might want to mock out side
> effects -- is it?

Yeah, I think I had the logic backwards earlier. It seems fine looking a second
time.

LGTM.

[gsennton, 2017-03-01 16:23:30]

> Mmm, I think the idea is that we've uploaded everything we're going to upload,
> so we might as well do some cleanup.  I'm not sure whether Webview has other
> times that this cleanup might happen; Chrome runs it on startup, after some
> delay.  Perhaps Gustav has some further comments?

It's for cleanup after uploads, IIRC this is the only cleanup we do in the
separate services.

[Ilya Sherman, 2017-03-01 17:17:08]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2017-03-01 17:17:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 17:21:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488388628120880,
"parent_rev": "d79662e1eae0b35104c93f7356b7077cfc0f3202", "commit_rev":
"1201b57d3a5b9d94ca34835b27c714bbc9b41fb6"}

[commit-bot: I haz the power, 2017-03-01 17:22:07]

Description was changed from

==========
[Cleanup] Remove a param used to customize test behavior.

BUG=694884
TEST=none
R=tobiasjs@chromium.org
==========

to

==========
[Cleanup] Remove a param used to customize test behavior.

BUG=694884
TEST=none
R=tobiasjs@chromium.org

Review-Url: https://codereview.chromium.org/2721193002
Cr-Commit-Position: refs/heads/master@{#453958}
Committed:
https://chromium.googlesource.com/chromium/src/+/1201b57d3a5b9d94ca34835b27c7...
==========

[commit-bot: I haz the power, 2017-03-01 17:22:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1201b57d3a5b9d94ca34835b27c7...