Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(125)

Issue 2605293002: Add WARN_UNUSED_RESULT to base::Time methods that return bool. (Closed)

Created:
2 years, 9 months ago by digit1
Modified:
2 years, 8 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, vmpstr+watch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, asvitkine+watch_chromium.org, loading-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Add WARN_UNUSED_RESULT to base::Time methods that return bool. #

Patch Set 3 : simple rebase #

Patch Set 4 : fix ChromeOS build issue. #

Patch Set 5 : Fix ChromeOS unit-test #

Total comments: 3

Patch Set 6 : Use ASSERT_TRUE in unit test #

Total comments: 2

Patch Set 7 : Better use of EXPECT_TRUE in unit-tests #

Total comments: 2

Patch Set 8 : Add comment to PexeDownloader::didReceiveResponse() #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -26 lines) Patch
M base/time/time.h View 1 2 5 chunks +11 lines, -8 lines 0 comments Download
M chrome/browser/chromeos/file_system_provider/operations/get_metadata.cc View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/mobile_config_unittest.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/metrics/data_use_tracker.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M components/metrics/data_use_tracker_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/nacl/browser/pnacl_translation_cache_unittest.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M components/nacl/renderer/ppb_nacl_private_impl.cc View 1 2 3 4 5 6 7 1 chunk +7 lines, -1 line 0 comments Download
M components/variations/variations_seed_store.cc View 2 chunks +6 lines, -1 line 2 comments Download
M content/browser/blob_storage/blob_storage_context_unittest.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/loader/upload_data_stream_builder_unittest.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/http/http_response_headers_unittest.cc View 1 2 3 4 5 1 chunk +6 lines, -3 lines 0 comments Download

Messages

Total messages: 71 (43 generated)
digit1
gavinp@chromium.org: Please review changes in tbansal@chromium.org: Please review changes in sclittle@chromium.org: Please review changes in ...
2 years, 9 months ago (2016-12-30 14:51:00 UTC) #13
Charlie Harrison
content/browser/loader LGTM
2 years, 9 months ago (2016-12-30 17:56:16 UTC) #16
Avi (use Gerrit)
content lgtm
2 years, 9 months ago (2016-12-30 18:08:25 UTC) #18
tbansal1
components/d_r_p lgtm
2 years, 9 months ago (2016-12-31 02:26:23 UTC) #19
digit1
sehr@chromium.org: Please review changes in asvitkine@chromium.org: Please review changes in
2 years, 9 months ago (2017-01-02 16:23:50 UTC) #22
dmurph
Blob test lgtm On Mon, Jan 2, 2017, 9:23 AM <digit@chromium.org> wrote: > sehr@chromium.org: Please ...
2 years, 9 months ago (2017-01-02 17:17:59 UTC) #25
mmenke
On 2017/01/02 17:17:59, dmurph wrote: > Blob test lgtm > > On Mon, Jan 2, ...
2 years, 9 months ago (2017-01-03 02:32:36 UTC) #28
digit1
Thanks Matt, Sorry, you're perfectly right. I'm coming back to the Chrome team after 3 ...
2 years, 9 months ago (2017-01-03 14:32:25 UTC) #33
Nico
base/ lgtm, looks like a good change. thanks!
2 years, 9 months ago (2017-01-03 14:36:03 UTC) #34
Nico
On 2017/01/03 14:32:25, digit1 wrote: > Thanks Matt, > > Sorry, you're perfectly right. I'm ...
2 years, 9 months ago (2017-01-03 14:38:36 UTC) #35
digit1
> (you didn't list anyone for the changes in chrome/ – I didn't look at ...
2 years, 9 months ago (2017-01-03 15:29:13 UTC) #36
mmenke
content/browser/loader LGTM https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_headers_unittest.cc#newcode789 net/http/http_response_headers_unittest.cc:789: base::Time::FromString("Wed, 28 Nov 2007 00:45:20 GMT", &current_time)); ...
2 years, 9 months ago (2017-01-03 15:39:24 UTC) #37
digit1
https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_headers_unittest.cc File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_headers_unittest.cc#newcode789 net/http/http_response_headers_unittest.cc:789: base::Time::FromString("Wed, 28 Nov 2007 00:45:20 GMT", &current_time)); Yes, that ...
2 years, 9 months ago (2017-01-03 16:44:53 UTC) #40
mmenke
On 2017/01/03 16:44:53, digit1 wrote: > https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_headers_unittest.cc > File net/http/http_response_headers_unittest.cc (right): > > https://codereview.chromium.org/2605293002/diff/80001/net/http/http_response_headers_unittest.cc#newcode789 > ...
2 years, 9 months ago (2017-01-03 18:35:24 UTC) #43
Nico
https://codereview.chromium.org/2605293002/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc (right): https://codereview.chromium.org/2605293002/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc#newcode118 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?
2 years, 9 months ago (2017-01-03 19:00:38 UTC) #44
digit1
https://codereview.chromium.org/2605293002/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc (right): https://codereview.chromium.org/2605293002/diff/100001/components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats_unittest.cc#newcode118 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: ...
2 years, 9 months ago (2017-01-05 10:26:28 UTC) #47
bbudge
components/nacl/renderer lgtm
2 years, 9 months ago (2017-01-06 11:45:10 UTC) #50
Alexei Svitkine (slow)
components/metrics lgtm
2 years, 9 months ago (2017-01-06 16:08:52 UTC) #51
miu
Adding myself as reviewer since, well, I'm the OWNER of base/time/... ;-)
2 years, 9 months ago (2017-01-10 03:58:32 UTC) #53
miu
PS7 lgtm % one small thing: https://codereview.chromium.org/2605293002/diff/120001/components/nacl/renderer/ppb_nacl_private_impl.cc File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/2605293002/diff/120001/components/nacl/renderer/ppb_nacl_private_impl.cc#newcode1626 components/nacl/renderer/ppb_nacl_private_impl.cc:1626: ignore_result( Did we ...
2 years, 9 months ago (2017-01-10 04:02:42 UTC) #54
digit1
https://codereview.chromium.org/2605293002/diff/120001/components/nacl/renderer/ppb_nacl_private_impl.cc File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/2605293002/diff/120001/components/nacl/renderer/ppb_nacl_private_impl.cc#newcode1626 components/nacl/renderer/ppb_nacl_private_impl.cc:1626: ignore_result( I've checked downstream that the only user for ...
2 years, 9 months ago (2017-01-10 13:55:58 UTC) #57
sehr
+bradnelson -me
2 years, 9 months ago (2017-01-10 14:39:13 UTC) #59
bradnelson
lgtm for nacl portion
2 years, 9 months ago (2017-01-10 18:46:15 UTC) #62
gavinp
net/http LGTM, thanks for catching that earlier problem mmenke.
2 years, 9 months ago (2017-01-10 19:41:26 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2605293002/140001
2 years, 9 months ago (2017-01-12 16:56:31 UTC) #66
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2c8eed34518aa37ebe2decc78af0694e1826c43b
2 years, 9 months ago (2017-01-12 20:50:22 UTC) #69
Alexei Svitkine (slow)
https://codereview.chromium.org/2605293002/diff/140001/components/variations/variations_seed_store.cc File components/variations/variations_seed_store.cc (right): https://codereview.chromium.org/2605293002/diff/140001/components/variations/variations_seed_store.cc#newcode142 components/variations/variations_seed_store.cc:142: FIRST_RUN_SEED_IMPORT_FAIL_INVALID_RESPONSE_DATE, Can you add this to histograms.xml? Seems like ...
2 years, 9 months ago (2017-01-17 21:38:07 UTC) #70
digit1
2 years, 8 months ago (2017-01-19 12:57:41 UTC) #71
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld 408576698