Add WARN_UNUSED_RESULT to base::Time methods that return bool.

Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

- PexeDownloader::didReceiveResponse() still ignores
  invalid HTTP response 'last-modified' header dates.
  As with RemoveExpiredEntriesForPref() this doesn't
  change the runtime behaviour, but it is hard to see
  whether this is desirable or dangerous.

- ConvertRequestValueToFileInfo() still ignores
  invalid date strings, as mentioned by the comment
  above the base::Time::FromString() line.

BUG=669625
Review-Url: https://codereview.chromium.org/2605293002
Cr-Commit-Position: refs/heads/master@{#443353}
Committed: https://chromium.googlesource.com/chromium/src/+/2c8eed34518aa37ebe2decc78af0694e1826c43b

[digit1, 2016-12-29 16:44:53]

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

[commit-bot: I haz the power, 2016-12-29 16:45:03]

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, 2016-12-29 17:20:37]

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

[commit-bot: I haz the power, 2016-12-29 17:20:38]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[digit1, 2016-12-29 19:34:50]

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

[commit-bot: I haz the power, 2016-12-29 19:34:59]

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, 2016-12-29 20:41:19]

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

[commit-bot: I haz the power, 2016-12-29 20:41:19]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[digit1, 2016-12-30 14:23:33]

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

[commit-bot: I haz the power, 2016-12-30 14:23:44]

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

[digit1, 2016-12-30 14:46:24]

Description was changed from

==========
Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

BUG=669625
==========

to

==========
Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

- PexeDownloader::didReceiveResponse() still ignores
  invalid HTTP response 'last-modified' header dates.
  As with RemoveExpiredEntriesForPref() this doesn't
  change the runtime behaviour, but it is hard to see
  whether this is desirable or dangerous.

BUG=669625
==========

[digit1, 2016-12-30 14:50:59]

digit@chromium.org changed reviewers:
+ bbudge@chromium.org, csharrison@chromium.org, dmurph@chromium.org,
gavinp@chromium.org, jwd@chromium.org, mmenke@chromium.org, raymes@chromium.org,
sclittle@chromium.org, tbansal@chromium.org, thakis@chromium.org,
thestig@chromium.org

[digit1, 2016-12-30 14:51:00]

gavinp@chromium.org: Please review changes in 

tbansal@chromium.org: Please review changes in 

sclittle@chromium.org: Please review changes in 

thestig@chromium.org: Please review changes in 

thakis@chromium.org: Please review changes in 

csharrison@chromium.org: Please review changes in 

dmurph@chromium.org: Please review changes in 

raymes@chromium.org: Please review changes in 

bbudge@chromium.org: Please review changes in 

jwd@chromium.org: Please review changes in 

mmenke@chromium.org: Please review changes in

[commit-bot: I haz the power, 2016-12-30 15:25:25]

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

[commit-bot: I haz the power, 2016-12-30 15:25:26]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Charlie Harrison, 2016-12-30 17:56:16]

content/browser/loader LGTM

[Avi (use Gerrit), 2016-12-30 18:08:24]

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

[Avi (use Gerrit), 2016-12-30 18:08:25]

content lgtm

[tbansal1, 2016-12-31 02:26:23]

components/d_r_p lgtm

[digit1, 2017-01-02 16:23:27]

Description was changed from

==========
Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

- PexeDownloader::didReceiveResponse() still ignores
  invalid HTTP response 'last-modified' header dates.
  As with RemoveExpiredEntriesForPref() this doesn't
  change the runtime behaviour, but it is hard to see
  whether this is desirable or dangerous.

BUG=669625
==========

to

==========
Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

- PexeDownloader::didReceiveResponse() still ignores
  invalid HTTP response 'last-modified' header dates.
  As with RemoveExpiredEntriesForPref() this doesn't
  change the runtime behaviour, but it is hard to see
  whether this is desirable or dangerous.

- ConvertRequestValueToFileInfo() still ignores
  invalid date strings, as mentioned by the comment
  above the base::Time::FromString() line.

BUG=669625
==========

[digit1, 2017-01-02 16:23:50]

digit@chromium.org changed reviewers:
+ asvitkine@chromium.org, sehr@chromium.org

[digit1, 2017-01-02 16:23:50]

sehr@chromium.org: Please review changes in 

asvitkine@chromium.org: Please review changes in

[digit1, 2017-01-02 16:24:04]

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

[commit-bot: I haz the power, 2017-01-02 16:24:09]

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

[dmurph, 2017-01-02 17:17:59]

Blob test lgtm

On Mon, Jan 2, 2017, 9:23 AM <digit@chromium.org> wrote:

> sehr@chromium.org: Please review changes in
>
> asvitkine@chromium.org: Please review changes in
>
>
>
> https://codereview.chromium.org/2605293002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-02 17:40:26]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-01-03 02:32:36]

On 2017/01/02 17:17:59, dmurph wrote:
> Blob test lgtm
> 
> On Mon, Jan 2, 2017, 9:23 AM <mailto:digit@chromium.org> wrote:
> 
> > mailto:sehr@chromium.org: Please review changes in
> >
> > mailto:asvitkine@chromium.org: Please review changes in
> >
> >
> >
> > https://codereview.chromium.org/2605293002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Could you tell people what they should be reviewing?  I overlap with csharrison
on some files, gavinp on others, so it's not at all clear what you want each of
us to review.

[digit1, 2017-01-03 10:20:29]

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

[commit-bot: I haz the power, 2017-01-03 10:20:44]

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-01-03 12:03:40]

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

[commit-bot: I haz the power, 2017-01-03 12:03:41]

Dry run: This issue passed the CQ dry run.

[digit1, 2017-01-03 14:32:25]

Thanks Matt,

Sorry, you're perfectly right. I'm coming back to the Chrome team after 3 years,
still learning the new tricks. Here's the
updated list for everyone:

gavinp@chromium.org: Please review changes in net/http/

thestig@chromium.org: Please review changes in base/time/

thakis@chromium.org: Please review changes in base/time/

raymes@chromium.org: Please review changes in components/nacl/renderer/

bbudge@chromium.org: Please review changes in components/nacl/renderer/

jwd@chromium.org: Please review changes in components/variations/

mmenke@chromium.org: Please review changes in content/browser/loader/

sehr@chromium.org: Please review changes in components/nacl/browser/

asvitkine@chromium.org: Please review changes in components/variations/ +
components/metrics/

[DONE] tbansal@chromium.org: Please review changes in
components/data_reduction_proxy/

[DONE] sclittle@chromium.org: Please review changes in 
components/data_reduction_proxy/

[DONE] csharrison@chromium.org: Please review changes in content/browser/loader/

[DONE] dmurph@chromium.org: Please review changes in
content/browser/blob_storage/

Sorry for the annoyance, and thanks for your patience :)

[Nico, 2017-01-03 14:36:03]

base/ lgtm, looks like a good change. thanks!

[Nico, 2017-01-03 14:38:36]

On 2017/01/03 14:32:25, digit1 wrote:
> Thanks Matt,
> 
> Sorry, you're perfectly right. I'm coming back to the Chrome team after 3
years,
> still learning the new tricks. Here's the
> updated list for everyone:
> 
> mailto:gavinp@chromium.org: Please review changes in net/http/
> 
> mailto:thestig@chromium.org: Please review changes in base/time/
> 
> mailto:thakis@chromium.org: Please review changes in base/time/
> 
> mailto:raymes@chromium.org: Please review changes in components/nacl/renderer/
> 
> mailto:bbudge@chromium.org: Please review changes in components/nacl/renderer/
> 
> mailto:jwd@chromium.org: Please review changes in components/variations/
> 
> mailto:mmenke@chromium.org: Please review changes in content/browser/loader/
> 
> mailto:sehr@chromium.org: Please review changes in components/nacl/browser/
> 
> mailto:asvitkine@chromium.org: Please review changes in components/variations/
+
> components/metrics/
> 
> [DONE] mailto:tbansal@chromium.org: Please review changes in
> components/data_reduction_proxy/
> 
> [DONE] mailto:sclittle@chromium.org: Please review changes in 
> components/data_reduction_proxy/
> 
> [DONE] mailto:csharrison@chromium.org: Please review changes in
content/browser/loader/
> 
> [DONE] mailto:dmurph@chromium.org: Please review changes in
> content/browser/blob_storage/
> 
> Sorry for the annoyance, and thanks for your patience :)

(you didn't list anyone for the changes in chrome/ – I didn't look at those but
my lgtm covers those. So now I did look at them and saw that there was an
ignore_result call there. Then I looked at the whole CL, and of the 28
FromString() calls, 6 (over 20%) have ignore_result calls. This suggests that
these functions probably should _not_ have WARN_UNUSED_RESULT, no?

[digit1, 2017-01-03 15:29:13]

> (you didn't list anyone for the changes in chrome/ – I didn't look at those
but
> my lgtm covers those. So now I did look at them and saw that there was an
> ignore_result call there. Then I looked at the whole CL, and of the 28
> FromString() calls, 6 (over 20%) have ignore_result calls. This suggests that
> these functions probably should _not_ have WARN_UNUSED_RESULT, no?

What would be a good criterion for WARN_UNUSED_RESULT? 3 of the uses you mention
come from unit-tests that obviously use hard-coded values that cannot fail,
but for the rest of the code, it's not always easy to tell whether a date comes
from user-provided data (instead of being handled purely internally).

I tried not to change the runtime behaviour, except in
VariationsSeedStore::ImportFirstRunJavaSeed() because it seems an invalid date
could easily slip by and unexpectedly skew results (I have no idea whether this
represents a real security issue or a potential UMA misdirection though). I also
added a comment regarding data ppb_nacl_private_impl.cc

https://codereview.chromium.org/2605293002/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2605293002/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1627:
base::Time::FromString(last_modified.c_str(), &last_modified_time));
note: I'm not sure this is the right thing to do here. What could be the impact
of an invalid modified-time string here? It looks like the extracted base::Time
value is stored in a cache node. So the worst that could happen would be to
force too many re-downloads of a resource. However that's my very superficial
view of the code. Would love to have a better opinion on this, and be able to
add a comment here explaining why it's ok.

At least the change doesn't change runtime behaviour.

[mmenke, 2017-01-03 15:39:24]

content/browser/loader LGTM

https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:789: base::Time::FromString("Wed, 28
Nov 2007 00:45:20 GMT", &current_time));
Should these all be ASSERT_TRUE?

[digit1, 2017-01-03 16:44:10]

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

[commit-bot: I haz the power, 2017-01-03 16:44:26]

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

[digit1, 2017-01-03 16:44:53]

https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:789: base::Time::FromString("Wed, 28
Nov 2007 00:45:20 GMT", &current_time));
Yes, that sounds more logical, I've uploaded a new patchset that does this.
Thanks.

[commit-bot: I haz the power, 2017-01-03 18:25:31]

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

[commit-bot: I haz the power, 2017-01-03 18:25:32]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-03 18:35:24]

On 2017/01/03 16:44:53, digit1 wrote:
>
https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_...
> File net/http/http_response_headers_unittest.cc (right):
> 
>
https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_...
> net/http/http_response_headers_unittest.cc:789: base::Time::FromString("Wed,
28
> Nov 2007 00:45:20 GMT", &current_time));
> Yes, that sounds more logical, I've uploaded a new patchset that does this.
> Thanks.

net/ also LGTM, so no need to wait for gavinp on that.

[Nico, 2017-01-03 19:00:38]

https://codereview.chromium.org/2605293002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc
(right):

https://codereview.chromium.org/2605293002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc:118:
ignore_result(base::Time::FromString(kLastUpdateTime, &now_));
this can probably be EXPECT_TRUE too?

[digit1, 2017-01-05 10:18:13]

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

[commit-bot: I haz the power, 2017-01-05 10:18:28]

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

[digit1, 2017-01-05 10:26:28]

https://codereview.chromium.org/2605293002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc
(right):

https://codereview.chromium.org/2605293002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc:118:
ignore_result(base::Time::FromString(kLastUpdateTime, &now_));
On 2017/01/03 19:00:38, Nico (ooo sick) wrote:
> this can probably be EXPECT_TRUE too?

Good point, I've changed that too. Thanks :)

[commit-bot: I haz the power, 2017-01-05 12:18:52]

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

[commit-bot: I haz the power, 2017-01-05 12:18:53]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-01-06 11:45:10]

components/nacl/renderer lgtm

[Alexei Svitkine (slow), 2017-01-06 16:08:52]

components/metrics lgtm

[miu, 2017-01-10 03:58:31]

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

[miu, 2017-01-10 03:58:32]

Adding myself as reviewer since, well, I'm the OWNER of base/time/...  ;-)

[miu, 2017-01-10 04:02:42]

PS7 lgtm % one small thing:

https://codereview.chromium.org/2605293002/diff/120001/components/nacl/render...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2605293002/diff/120001/components/nacl/render...
components/nacl/renderer/ppb_nacl_private_impl.cc:1626: ignore_result(
Did we check what downstream code will do if |last_modified_time| is not
parseable? If so, can you add a quick comment here explaining what we're
ignoring the result because ...?

[digit1, 2017-01-10 13:55:15]

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

[commit-bot: I haz the power, 2017-01-10 13:55:32]

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

[digit1, 2017-01-10 13:55:58]

https://codereview.chromium.org/2605293002/diff/120001/components/nacl/render...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2605293002/diff/120001/components/nacl/render...
components/nacl/renderer/ppb_nacl_private_impl.cc:1626: ignore_result(
I've checked downstream that the only user for this value handles null
base::Time values properly. I've added a comment explaining this in my latest
upload. Thanks.

[sehr, 2017-01-10 14:39:10]

sehr@chromium.org changed reviewers:
+ bradnelson@chromium.org

[sehr, 2017-01-10 14:39:13]

+bradnelson
-me

[commit-bot: I haz the power, 2017-01-10 16:52:48]

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

[commit-bot: I haz the power, 2017-01-10 16:52:49]

Dry run: This issue passed the CQ dry run.

[bradnelson, 2017-01-10 18:46:15]

lgtm for nacl portion

[gavinp, 2017-01-10 19:41:26]

net/http LGTM, thanks for catching that earlier problem mmenke.

[digit1, 2017-01-12 16:56:06]

The CQ bit was checked by digit@chromium.org

[digit1, 2017-01-12 16:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
csharrison@chromium.org, tbansal@chromium.org, thakis@chromium.org,
mmenke@chromium.org, bbudge@chromium.org, asvitkine@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2605293002/#ps140001
(title: "Add comment to PexeDownloader::didReceiveResponse()")

[commit-bot: I haz the power, 2017-01-12 16:56:31]

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-01-12 20:49:45]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484240166852600,
"parent_rev": "9020aee19a0a9cdb845f7cb2bc7f30c799741fdf", "commit_rev":
"2c8eed34518aa37ebe2decc78af0694e1826c43b"}

[commit-bot: I haz the power, 2017-01-12 20:50:18]

Description was changed from

==========
Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

- PexeDownloader::didReceiveResponse() still ignores
  invalid HTTP response 'last-modified' header dates.
  As with RemoveExpiredEntriesForPref() this doesn't
  change the runtime behaviour, but it is hard to see
  whether this is desirable or dangerous.

- ConvertRequestValueToFileInfo() still ignores
  invalid date strings, as mentioned by the comment
  above the base::Time::FromString() line.

BUG=669625
==========

to

==========
Add WARN_UNUSED_RESULT to base::Time methods that return bool.

base::Time::FromString() and base::Time::FromUTCString() might fail
at runtime on bad input. This adds a WARN_UNUSED_RESULT to their
declarations to ensure that all callers properly handle the result.

Apart from unit-tests, this changes two things:

- DataUseTracker::RemoveExpiredEntriesForPref() will also
  remove any entries with an invalid date string.

  This doesn't change the runtime behaviour though, because
  a failure in base::Time::FromUTCString() would keep the
  local |key_date| variable to 0, which would have been
  considered too old anyway.

- VariationsSeedStore::ImportFirstRunJavaSeed() has
  a new failure mode triggered by an invalid response
  date |FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE|

  It looks like the FirstRunResult enum values are only
  used on Android to report UMA metrics, so ensure that
  the new constant is added at the end of the list to
  avoid generating confusing histograms.

- PexeDownloader::didReceiveResponse() still ignores
  invalid HTTP response 'last-modified' header dates.
  As with RemoveExpiredEntriesForPref() this doesn't
  change the runtime behaviour, but it is hard to see
  whether this is desirable or dangerous.

- ConvertRequestValueToFileInfo() still ignores
  invalid date strings, as mentioned by the comment
  above the base::Time::FromString() line.

BUG=669625

Review-Url: https://codereview.chromium.org/2605293002
Cr-Commit-Position: refs/heads/master@{#443353}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c8eed34518aa37ebe2decc78af0...
==========

[commit-bot: I haz the power, 2017-01-12 20:50:22]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2c8eed34518aa37ebe2decc78af0...

[Alexei Svitkine (slow), 2017-01-17 21:38:07]

https://codereview.chromium.org/2605293002/diff/140001/components/variations/...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/2605293002/diff/140001/components/variations/...
components/variations/variations_seed_store.cc:142:
FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE,
Can you add this to histograms.xml? Seems like you forgot to do this as part of
the change.

[digit1, 2017-01-19 12:57:41]

https://codereview.chromium.org/2605293002/diff/140001/components/variations/...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/2605293002/diff/140001/components/variations/...
components/variations/variations_seed_store.cc:142:
FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE,
Thanks for spotting this. Please see the new patch at
https://codereview.chromium.org/2646743003