Avoid base::Time::FromJavaTime() when not dealing with Java.

Avoid base::Time::FromJavaTime() when not dealing with Java.

Add comments to base/time/time.h to point readers to
FromInternalValue()/ToInternalValue().

[Lei Zhang, 2017-05-09 22:11:44]

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

[commit-bot: I haz the power, 2017-05-09 22:13:13]

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-05-10 00:10:55]

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

[commit-bot: I haz the power, 2017-05-10 00:10:56]

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

[Lei Zhang, 2017-05-10 08:17:13]

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

[commit-bot: I haz the power, 2017-05-10 08:17:37]

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-05-10 09:55:12]

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

[commit-bot: I haz the power, 2017-05-10 09:55:13]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-05-10 17:10:20]

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

[Lei Zhang, 2017-05-10 17:10:20]

I think folks are not seeing FromInternalValue() because it's 150 lines up in
TimeBase, so they are using the next closest thing.

[miu, 2017-05-10 21:43:02]

https://codereview.chromium.org/2870223003/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/2870223003/diff/1/base/time/time.h#newcode495
base/time/time.h:495: // Consider only using this when dealing with Javascript,
and using
Actually, I'm trying to eliminate all uses of From/ToInternalValue()
 (interesting discussion in http://crbug.com/634507). People use them to cheat
the typing system more often than for wire-format serialization. This has and
continues to cause lots of hard-to-catch bugs, exactly what strong typing of our
time data and math is meant to prevent.

FWIW, this isn't the first time we've had issues with these "java time
conversion" functions. ToJsTime() is inconsistent with FromJsTime() because
folks decided to make the perfectly valid "0" be a representation of null only
in ToJsTime(). So, it's possible for a time math expression to correctly
evaluate to zero, but then after conversion to JS time, it'll be erroneously
interpreted as null. :-/

There was an earlier attempt to fix this that had to be rolled-back due to too
much interweaved dependence on this behavior. Nevertheless, I really want to
kill these and force the platform glue (e.g. between Blink and Chromium base) to
deal with any special conversions. Also, now that we have base::Optional, it
probably makes sense to get rid of is_null() in these time classes; which would
allow "0" to be a valid non-null result.

I'm curious: Can you point me to examples of where you found these were being
misused for serialization? I'd like to better understand the use cases (outside
of Blink<-->Chromium time type conversion).

https://codereview.chromium.org/2870223003/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2870223003/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc:93: const
base::Time kDummyNow = base::Time::FromInternalValue(13130341200000000);
nit: Rather than FromInternalValue, how about something like:

  constexpr base::Time kDummyNow =
      Time::UnixEpoch() + TimeDelta::FromDays(47 * 365.2425);

Feel free to make Time::UnixEpoch() a constexpr function (we've been gradually
enhancing the Time classes with C++11 constructs). :)

[Lei Zhang, 2017-05-10 23:43:00]

What prompted my interest was https://codereview.chromium.org/2858353004/ which
uses FromJavaTime() even though there's no Java involved. There was a CL from
months ago that wrote the ToJavaTime() value into a protobuf, so now it's too
late to change it / doing a migration is just not worth it.

So I started looking at FromJavaTime() usage elsewhere, and this was the only
other spot where it was using FromJavaTime() when it does not involve Java.

[miu, 2017-05-11 21:12:44]

On 2017/05/10 23:43:00, Lei Zhang (OOO) wrote:
> What prompted my interest was https://codereview.chromium.org/2858353004/
which

Unfortunately, that change added a perfect example of why I'm trying to
eliminate FromInternalValue(). This code (in the unit test):

  const base::Time kTimestamp = base::Time::FromInternalValue(445566);

Will cause all the tests to use a "last updated printer configuration time" of
~8 minutes after January 1, 1601. I'm pretty sure that's not what was intended.
;-)

Instead, there's what I proposed in the last round of comments in this change.
Or, for even more readability (but more verbose):

  base::Time GetPrinterLastUpdatedTime() {
    base::Time result;
    CHECK(base::Time::FromUTCExploded(
        base::Time::Exploded {.year = 2017, .month = 1, .day_of_month = 1, 0},
        &result));
    return result;
  }

> uses FromJavaTime() even though there's no Java involved. There was a CL from
> months ago that wrote the ToJavaTime() value into a protobuf, so now it's too
> late to change it / doing a migration is just not worth it.

I agree they shouldn't have been using FromJavaTime() when interfacing with Java
wasn't involved. The thing I'm uncomfortable with in this change is even
mentioning that To/FromInternalValue() exist. You could be right, however. I'm
just not there yet, myself. ;-)

Thinking more about this, what if we had usage comments like the following for
these converters instead:

  // These are only to be used for interfacing purposes (e.g., Java JNI, or
third-party
  // libraries). Chromium C++ code must otherwise consistently use base::Time.

Sound good? (I suppose a next logical step *could* be to propose a change the
Chromium style guide, w.r.t. time data type usage.)

> So I started looking at FromJavaTime() usage elsewhere, and this was the only
> other spot where it was using FromJavaTime() when it does not involve Java.

That's a relief! :)

[vitaliii, 2017-05-12 10:15:18]

On 2017/05/11 21:12:44, miu wrote:
> On 2017/05/10 23:43:00, Lei Zhang (OOO) wrote:
> > What prompted my interest was https://codereview.chromium.org/2858353004/
> which
> 
> Unfortunately, that change added a perfect example of why I'm trying to
> eliminate FromInternalValue(). This code (in the unit test):
> 
>   const base::Time kTimestamp = base::Time::FromInternalValue(445566);
> 
> Will cause all the tests to use a "last updated printer configuration time" of
> ~8 minutes after January 1, 1601. I'm pretty sure that's not what was
intended.
> ;-)
> 
> Instead, there's what I proposed in the last round of comments in this change.
> Or, for even more readability (but more verbose):
> 
>   base::Time GetPrinterLastUpdatedTime() {
>     base::Time result;
>     CHECK(base::Time::FromUTCExploded(
>         base::Time::Exploded {.year = 2017, .month = 1, .day_of_month = 1, 0},
>         &result));
>     return result;
>   }
> 
> > uses FromJavaTime() even though there's no Java involved. There was a CL
from
> > months ago that wrote the ToJavaTime() value into a protobuf, so now it's
too
> > late to change it / doing a migration is just not worth it.
> 
> I agree they shouldn't have been using FromJavaTime() when interfacing with
Java
> wasn't involved. The thing I'm uncomfortable with in this change is even
> mentioning that To/FromInternalValue() exist. You could be right, however. I'm
> just not there yet, myself. ;-)
> 
> Thinking more about this, what if we had usage comments like the following for
> these converters instead:
> 
>   // These are only to be used for interfacing purposes (e.g., Java JNI, or
> third-party
>   // libraries). Chromium C++ code must otherwise consistently use base::Time.
> 
> Sound good? (I suppose a next logical step *could* be to propose a change the
> Chromium style guide, w.r.t. time data type usage.)
> 
> > So I started looking at FromJavaTime() usage elsewhere, and this was the
only
> > other spot where it was using FromJavaTime() when it does not involve Java.
> 
> That's a relief! :)

Hey,

I used FromJavaTime() in download_suggestions_provider_unittest.cc.

I did so, because it is not clear what "Internal Value" means and how to convert
a given date to "Internal Value".
Even now I cannot understand how I could get a given date using
FromInternalValue just by looking at that function and around it.
At the same time JavaTime is a more known concept and there are converters
online. Moreover, FromInternalValue says that is meant as a constructor and only
for serializing.

If you do not like people using FromJavaTime, please provide some other way to
get a given date
or elaborate in FromInternalValue comment.

[Bernhard Bauer, 2017-05-12 10:53:31]

On 2017/05/12 10:15:18, vitaliii wrote:
> On 2017/05/11 21:12:44, miu wrote:
> > On 2017/05/10 23:43:00, Lei Zhang (OOO) wrote:
> > > What prompted my interest was https://codereview.chromium.org/2858353004/
> > which
> > 
> > Unfortunately, that change added a perfect example of why I'm trying to
> > eliminate FromInternalValue(). This code (in the unit test):
> > 
> >   const base::Time kTimestamp = base::Time::FromInternalValue(445566);
> > 
> > Will cause all the tests to use a "last updated printer configuration time"
of
> > ~8 minutes after January 1, 1601. I'm pretty sure that's not what was
> intended.
> > ;-)
> > 
> > Instead, there's what I proposed in the last round of comments in this
change.
> > Or, for even more readability (but more verbose):
> > 
> >   base::Time GetPrinterLastUpdatedTime() {
> >     base::Time result;
> >     CHECK(base::Time::FromUTCExploded(
> >         base::Time::Exploded {.year = 2017, .month = 1, .day_of_month = 1,
0},
> >         &result));
> >     return result;
> >   }
> > 
> > > uses FromJavaTime() even though there's no Java involved. There was a CL
> from
> > > months ago that wrote the ToJavaTime() value into a protobuf, so now it's
> too
> > > late to change it / doing a migration is just not worth it.
> > 
> > I agree they shouldn't have been using FromJavaTime() when interfacing with
> Java
> > wasn't involved. The thing I'm uncomfortable with in this change is even
> > mentioning that To/FromInternalValue() exist. You could be right, however.
I'm
> > just not there yet, myself. ;-)
> > 
> > Thinking more about this, what if we had usage comments like the following
for
> > these converters instead:
> > 
> >   // These are only to be used for interfacing purposes (e.g., Java JNI, or
> > third-party
> >   // libraries). Chromium C++ code must otherwise consistently use
base::Time.
> > 
> > Sound good? (I suppose a next logical step *could* be to propose a change
the
> > Chromium style guide, w.r.t. time data type usage.)
> > 
> > > So I started looking at FromJavaTime() usage elsewhere, and this was the
> only
> > > other spot where it was using FromJavaTime() when it does not involve
Java.
> > 
> > That's a relief! :)
> 
> Hey,
> 
> I used FromJavaTime() in download_suggestions_provider_unittest.cc.
> 
> I did so, because it is not clear what "Internal Value" means and how to
convert
> a given date to "Internal Value".
> Even now I cannot understand how I could get a given date using
> FromInternalValue just by looking at that function and around it.
> At the same time JavaTime is a more known concept and there are converters
> online. Moreover, FromInternalValue says that is meant as a constructor and
only
> for serializing.
> 
> If you do not like people using FromJavaTime, please provide some other way to
> get a given date
> or elaborate in FromInternalValue comment.

See the comment you just replied to:

>     base::Time result;
>     CHECK(base::Time::FromUTCExploded(
>         base::Time::Exploded {.year = 2017, .month = 1, .day_of_month = 1, 0},
>         &result));
>     return result;

[vitaliii, 2017-05-12 11:50:00]

On 2017/05/12 10:53:31, Bernhard Bauer wrote:
> On 2017/05/12 10:15:18, vitaliii wrote:
> > On 2017/05/11 21:12:44, miu wrote:
> > > On 2017/05/10 23:43:00, Lei Zhang (OOO) wrote:
> > > > What prompted my interest was
https://codereview.chromium.org/2858353004/
> > > which
> > > 
> > > Unfortunately, that change added a perfect example of why I'm trying to
> > > eliminate FromInternalValue(). This code (in the unit test):
> > > 
> > >   const base::Time kTimestamp = base::Time::FromInternalValue(445566);
> > > 
> > > Will cause all the tests to use a "last updated printer configuration
time"
> of
> > > ~8 minutes after January 1, 1601. I'm pretty sure that's not what was
> > intended.
> > > ;-)
> > > 
> > > Instead, there's what I proposed in the last round of comments in this
> change.
> > > Or, for even more readability (but more verbose):
> > > 
> > >   base::Time GetPrinterLastUpdatedTime() {
> > >     base::Time result;
> > >     CHECK(base::Time::FromUTCExploded(
> > >         base::Time::Exploded {.year = 2017, .month = 1, .day_of_month = 1,
> 0},
> > >         &result));
> > >     return result;
> > >   }
> > > 
> > > > uses FromJavaTime() even though there's no Java involved. There was a CL
> > from
> > > > months ago that wrote the ToJavaTime() value into a protobuf, so now
it's
> > too
> > > > late to change it / doing a migration is just not worth it.
> > > 
> > > I agree they shouldn't have been using FromJavaTime() when interfacing
with
> > Java
> > > wasn't involved. The thing I'm uncomfortable with in this change is even
> > > mentioning that To/FromInternalValue() exist. You could be right, however.
> I'm
> > > just not there yet, myself. ;-)
> > > 
> > > Thinking more about this, what if we had usage comments like the following
> for
> > > these converters instead:
> > > 
> > >   // These are only to be used for interfacing purposes (e.g., Java JNI,
or
> > > third-party
> > >   // libraries). Chromium C++ code must otherwise consistently use
> base::Time.
> > > 
> > > Sound good? (I suppose a next logical step *could* be to propose a change
> the
> > > Chromium style guide, w.r.t. time data type usage.)
> > > 
> > > > So I started looking at FromJavaTime() usage elsewhere, and this was the
> > only
> > > > other spot where it was using FromJavaTime() when it does not involve
> Java.
> > > 
> > > That's a relief! :)
> > 
> > Hey,
> > 
> > I used FromJavaTime() in download_suggestions_provider_unittest.cc.
> > 
> > I did so, because it is not clear what "Internal Value" means and how to
> convert
> > a given date to "Internal Value".
> > Even now I cannot understand how I could get a given date using
> > FromInternalValue just by looking at that function and around it.
> > At the same time JavaTime is a more known concept and there are converters
> > online. Moreover, FromInternalValue says that is meant as a constructor and
> only
> > for serializing.
> > 
> > If you do not like people using FromJavaTime, please provide some other way
to
> > get a given date
> > or elaborate in FromInternalValue comment.
> 
> See the comment you just replied to:
> 
> >     base::Time result;
> >     CHECK(base::Time::FromUTCExploded(
> >         base::Time::Exploded {.year = 2017, .month = 1, .day_of_month = 1,
0},
> >         &result));
> >     return result;

That's very nice, but still it is nearly impossible to figure out that you need
FromUTCExploded, but not FromInternalValue. 
My message was more about FromInternalValue comment.

[miu, 2017-05-12 23:05:02]

On 2017/05/12 11:50:00, vitaliii wrote:
> That's very nice, but still it is nearly impossible to figure out that you
need
> FromUTCExploded, but not FromInternalValue. 
> My message was more about FromInternalValue comment.

Wait a sec, folks. I've actually been saying I'd suggest *not* using
FromInternalValue for any use case. ;-)

It sounds like we're trying to write time.h header comments to describe what
"standard practice" should be for all possible use cases. Instead, perhaps we
should put together a chromium.org sites page that goes through many scenarios
in detail, describing the pros/cons of different approaches, much like a style
guide? Then, the time.h header can reference that document.

To expand on what I said, in #msg13, regarding what I feel the header comments
should look like for To/FromJSTime: "Serialization" is too-general a
consideration. In Chromium, there are three specific problems developers face:
1) "How should I persist time values?"; 2) "How should I send time values
between processes in Chromium?"; 3) "How should I translate values to/from some
platform (JS/Java), service (API demands something specific) or library at
runtime?"

For #1, you want to be storing calendar time. Some typical ways to do this: a) a
standard epoch time, such as "seconds since UNIX epoch."; b) strings (e.g., ISO
formats like "2017-01-01 01:02:03Z"); c) year/month/day as separate fields. If
you are writing Chromium code persisting values in Chromium-owned storage, then
it's probably best not to use the JS/Java converters; because you don't want to
be surprised by special-cases (like "0" means null...SOMETIMES).

For #2, don't convert away from the base::TimeXXX types. The Mojo/IPC layer has
you covered.

For #3, it makes sense to use the JS/Java/other converters. They are basically
adapter code; ideally, these should always be wrapping arguments or return
values for non-Chromium function calls.

[Lei Zhang, 2017-05-19 00:51:53]

If we want to put together some documentation in docs/ and point to that in
time.h, that SGTM.

I'll fix up this CL to not use FromInternalValue() either and ping you when it's
ready.

[Lei Zhang, 2017-05-19 01:00:34]

Actually, it'll be cleaner to start with a new CL. Closing.

[Lei Zhang, 2017-05-19 01:00:40]

Description was changed from

==========
Avoid base::Time::FromJavaTime() when not dealing with Java.

Add comments to base/time/time.h to point readers to
FromInternalValue()/ToInternalValue().
==========

to

==========
Avoid base::Time::FromJavaTime() when not dealing with Java.

Add comments to base/time/time.h to point readers to
FromInternalValue()/ToInternalValue().
==========