Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892553005:
Avoid base::Time::FromJavaTime() when not dealing with Java. (Closed)
DescriptionAvoid base::Time::FromJavaTime() when not dealing with Java.
DownloadSuggestionsProviderTest does not involve Java.
Review-Url: https://codereview.chromium.org/2892553005
Cr-Commit-Position: refs/heads/master@{#474115}
Committed: https://chromium.googlesource.com/chromium/src/+/a009c5fb061f47e6f6e4c7b486ab217c6ed6e5f9
Patch Set 1 #
Total comments: 1
Patch Set 2 : address comment #
Total comments: 2
Patch Set 3 : more constants #Messages
Total messages: 29 (13 generated)
The CQ bit was checked by thestig@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.
thestig@chromium.org changed reviewers: + miu@chromium.org, vitaliii@chromium.org
How's this? Making bits of base::Time constexpr turned out to be a bit more work than I liked.
https://codereview.chromium.org/2892553005/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2892553005/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:93: base::Time GetDummyNowTime() { How about == CODE STARTS == base::Time GetTimeFromUTCString(const std::string& time_string) { base::Time result; CHECK(base::Time::FromUTCString(time_string, &result)); return result; } const base::Time now = GetTimeFromUTCString("2017-01-31 13:00:00") == CODE ENDS == I do not like that GetDummy(Not)?OutdatedTime() adds a level of indirection.
How's patch set 2?
LGTM. I meant not to have GetDummyNotOutdatedTime() at all (our team internally agreed that such helpers in tests are not useful and increase time to understand the test). However, this is not a big deal, so feel free to land as it is.
On 2017/05/19 09:46:13, vitaliii wrote: > LGTM. > > I meant not to have GetDummyNotOutdatedTime() at all (our team internally agreed > that such helpers in tests are not useful and increase time to understand the > test). However, this is not a big deal, so feel free to land as it is. Is that agreement based on what's advocated in go/tott338 or something along those lines? Unlike the example given there, GetDummyNotOutdatedTime() actually better conveys what a complicated multi-line math equation is trying to do. At least what's my point of view.
On 2017/05/20 00:49:26, Lei Zhang wrote:
> On 2017/05/19 09:46:13, vitaliii wrote:
> > LGTM.
> >
> > I meant not to have GetDummyNotOutdatedTime() at all (our team internally
> agreed
> > that such helpers in tests are not useful and increase time to understand
the
> > test). However, this is not a big deal, so feel free to land as it is.
>
> Is that agreement based on what's advocated in go/tott338 or something along
> those lines?
Somewhat. We did not like additional time to understand the helpers.
> Unlike the example given there, GetDummyNotOutdatedTime() actually
> better conveys what a complicated multi-line math equation is trying to do. At
> least what's my point of view.
In my view |GetDummy{Not,}OutdatedTime()| conveys precisely the same information
as the variable declaration |base::Time {not_,}outdated|, but in the latter case
one can also look up precise meaning without going anywhere.
Again, I do not feel strongly about this, so feel free to decide.
lgtm % style concern (your call): https://codereview.chromium.org/2892553005/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2892553005/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:99: const base::Time now = CalculateDummyNowTime(); Global static initializer here. So, this goes against the style guide, unless we are willing to make an exception for unit test code?
On 2017/05/22 13:41:56, vitaliii wrote:
> In my view |GetDummy{Not,}OutdatedTime()| conveys precisely the same
information
> as the variable declaration |base::Time {not_,}outdated|, but in the latter
case
> one can also look up precise meaning without going anywhere.
>
> Again, I do not feel strongly about this, so feel free to decide.
So would you prefer I make a couple more constant variables?
https://codereview.chromium.org/2892553005/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2892553005/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:99: const base::Time now = CalculateDummyNowTime(); On 2017/05/22 21:45:31, miu wrote: > Global static initializer here. So, this goes against the style guide, unless we > are willing to make an exception for unit test code? Yes, which is what I was avoiding in patch set 1. However, we have tons of static initializers in tests, and we don't have any enforcement for unit tests, so I was meh about it.
> Yes, which is what I was avoiding in patch set 1. However, we have tons of > static initializers in tests, and we don't have any enforcement for unit tests, > so I was meh about it. Seems fine to me, since the main reason we don't want static initializers are things like: 1) start-up time; 2) process data segment bloat; 3) determinism. #1 and #2 aren't much of an issues for unit test binaries, and #3 is ensured because we are simply initializing a constant from constant inputs. BTW--Did you want to add a comment to base/time/time.h, as we discussed in the last code review? I just encountered another code review where ToJavaTime() is being used to compute UNIX epoch time in code that has nothing to do with Java. :(
On 2017/05/22 22:09:39, miu wrote: > > Yes, which is what I was avoiding in patch set 1. However, we have tons of > > static initializers in tests, and we don't have any enforcement for unit > tests, > > so I was meh about it. > > Seems fine to me, since the main reason we don't want static initializers are > things like: 1) start-up time; 2) process data segment bloat; 3) determinism. #1 > and #2 aren't much of an issues for unit test binaries, and #3 is ensured > because we are simply initializing a constant from constant inputs. Ack. Yes, if this was in code that links into the chrome binary, I would have stuck with patch set 1. > BTW--Did you want to add a comment to base/time/time.h, as we discussed in the > last code review? I just encountered another code review where ToJavaTime() is > being used to compute UNIX epoch time in code that has nothing to do with Java. > :( No, I was just going to fix this use case here. If you want to add the comment, please do. I can review it.
still lgtm
On 2017/05/22 22:02:19, Lei Zhang wrote:
> On 2017/05/22 13:41:56, vitaliii wrote:
> > In my view |GetDummy{Not,}OutdatedTime()| conveys precisely the same
> information
> > as the variable declaration |base::Time {not_,}outdated|, but in the latter
> case
> > one can also look up precise meaning without going anywhere.
> >
> > Again, I do not feel strongly about this, so feel free to decide.
>
> So would you prefer I make a couple more constant variables?
Yes, I would prefer constant variables.
The CQ bit was checked by thestig@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.
On 2017/05/23 09:36:45, vitaliii wrote: > Yes, I would prefer constant variables. Done.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vitaliii@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2892553005/#ps40001 (title: "more constants")
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": 40001, "attempt_start_ts": 1495583906266750,
"parent_rev": "3617b0eea7ec74b8e731a23fed2f4070cbc284c4", "commit_rev":
"a009c5fb061f47e6f6e4c7b486ab217c6ed6e5f9"}
Message was sent while issue was closed.
Description was changed from ========== Avoid base::Time::FromJavaTime() when not dealing with Java. DownloadSuggestionsProviderTest does not involve Java. ========== to ========== Avoid base::Time::FromJavaTime() when not dealing with Java. DownloadSuggestionsProviderTest does not involve Java. Review-Url: https://codereview.chromium.org/2892553005 Cr-Commit-Position: refs/heads/master@{#474115} Committed: https://chromium.googlesource.com/chromium/src/+/a009c5fb061f47e6f6e4c7b486ab... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a009c5fb061f47e6f6e4c7b486ab... |
