|
|
Created:
3 years, 11 months ago by digit1 Modified:
3 years, 11 months ago Reviewers:
sclittle, bbudge, dmurph, Lei Zhang, bradnelson, gavinp, Avi (use Gerrit), Charlie Harrison, sehr, raymes, jwd, Alexei Svitkine (slow), tbansal1, Nico, mmenke, miu 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. |
DescriptionAdd 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
Messages
Total messages: 71 (43 generated)
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
content/browser/loader LGTM
avi@chromium.org changed reviewers: + avi@chromium.org
content lgtm
components/d_r_p lgtm
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 ==========
digit@chromium.org changed reviewers: + asvitkine@chromium.org, sehr@chromium.org
sehr@chromium.org: Please review changes in asvitkine@chromium.org: Please review changes in
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 :)
base/ lgtm, looks like a good change. thanks!
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?
> (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.
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", ¤t_time)); Should these all be ASSERT_TRUE?
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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", ¤t_time)); Yes, that sounds more logical, I've uploaded a new patchset that does this. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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", ¤t_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.
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?
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
components/nacl/renderer lgtm
components/metrics lgtm
miu@chromium.org changed reviewers: + miu@chromium.org
Adding myself as reviewer since, well, I'm the OWNER of base/time/... ;-)
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 ...?
The CQ bit was checked by digit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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@chromium.org changed reviewers: + bradnelson@chromium.org
+bradnelson -me
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for nacl portion
net/http LGTM, thanks for catching that earlier problem mmenke.
The CQ bit was checked by digit@chromium.org
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()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484240166852600, "parent_rev": "9020aee19a0a9cdb845f7cb2bc7f30c799741fdf", "commit_rev": "2c8eed34518aa37ebe2decc78af0694e1826c43b"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2c8eed34518aa37ebe2decc78af0...
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, Can you add this to histograms.xml? Seems like you forgot to do this as part of the change.
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 |