Make callers of FromUTC(Local)Exploded in components/ use new time API.

Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG=601900
Committed: https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011
Cr-Commit-Position: refs/heads/master@{#404095}

[maksims (do not use this acc), 2016-06-23 07:53:02]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 07:53:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095553002/1

[commit-bot: I haz the power, 2016-06-23 08:07:28]

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

[commit-bot: I haz the power, 2016-06-23 08:07:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[maksims (do not use this acc), 2016-06-28 10:02:51]

Description was changed from

==========
components time callers

BUG=
==========

to

==========
components time callers

BUG=
==========

[maksims (do not use this acc), 2016-06-28 10:02:51]

maksim.sisov@intel.com changed reviewers:
+ blundell@chromium.org

[maksims (do not use this acc), 2016-06-28 10:03:10]

Description was changed from

==========
components time callers

BUG=
==========

to

==========
Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with 
https://codereview.chromium.org/1988663002/ 

BUG=601900
==========

[maksims (do not use this acc), 2016-06-28 12:00:42]

Patchset #2 (id:20001) has been deleted

[maksims (do not use this acc), 2016-06-28 12:00:55]

please review

[blundell, 2016-06-28 13:50:55]

Please send this to OWNERS of the individual components in question, who will
also be best able to advise on error handling.

[sfiera, 2016-06-28 14:13:28]

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

[sfiera, 2016-06-28 14:13:28]

(looking at this for ntp_snippets, but I'm not an OWNER)

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
Here, we are combining YMD of a valid date (from LocalExplode()) with HMSm of a
valid time (06:00:00.000 or 22:00:00.000).

Can this fail?

I suppose if a country were to enter DST at 6AM, then for example
2017/03/01-06:00:00 might not exist locally [1]. In this case, we probably want
7AM, but any approximation might be good enough. Can we approximate somehow? I
imagine the old behavior on posix/mac might have been what we wanted.

[1] Or maybe I have it backwards and this kind of thing happens when leaving
DST. I hate DST.

[vabr (Chromium), 2016-06-28 14:34:12]

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

[vabr (Chromium), 2016-06-28 14:34:13]

I reviewed password_manager and autofill code. Please have a look at the
comments.

Thanks!
Vaclav

https://codereview.chromium.org/2095553002/diff/40001/components/autofill/con...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/autofill/con...
components/autofill/content/browser/risk/fingerprint.cc:70: if
(!base::Time::FromUTCExploded(local, &out_time)) {
Given that the exploded value comes from the result of base::Time::Now(), how
could FromUTCExploded fail on it? It seems to me that a DCHECK is in order here:

bool conversion_success = base::Time::FromUTCExploded(local, &out_time);
DCHECK(conversion_success);

Please note that for reasons explained in my other comment, the conversion must
not be placed inside the DCHECK, and the DCHECK failure must not be handled.

https://codereview.chromium.org/2095553002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/password_man...
components/password_manager/core/browser/password_store.cc:181: if
(base::Time::FromUTCExploded(exploded_cutoff, &out_time)) {
This is called for a constant |exploded_cutoff|. Is that not a guarantee that
this won't fail? If you agree, I sugest the following:

bool time_conversion_succeeded = base::Time::FromUTCExploded(exploded_cutoff,
&ignore_logins_cutoff);
DCHECK(time_conversion_succeeded);

Note 2 things:
(1) The conversion itself must not be inside the DCHECK, or it won't happen in
Release builds.
(2) No attempt is made to handle the failing DCHECK-ed condition in Release
builds, because the style guide forbids it
(http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).

[Alexei Svitkine (slow), 2016-06-28 15:27:39]

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

[Alexei Svitkine (slow), 2016-06-28 15:27:40]

https://codereview.chromium.org/2095553002/diff/40001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/variations/v...
components/variations/variations_seed_store.cc:119: NOTIMPLEMENTED();
Similar comment as vabr's - how can this fail? Should this be a NOTREACHED()
instead?

[maksims (do not use this acc), 2016-06-29 05:48:25]

maksim.sisov@intel.com changed reviewers:
+ fukino@chromium.org, hashimoto@chromium.org, jwd@chromium.org,
kundaji@chromium.org, megjablon@chromium.org, noyau@chromium.org

[maksims (do not use this acc), 2016-06-29 05:48:59]

Added reviewers per components. Please take a look

https://codereview.chromium.org/2095553002/diff/40001/components/autofill/con...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/autofill/con...
components/autofill/content/browser/risk/fingerprint.cc:70: if
(!base::Time::FromUTCExploded(local, &out_time)) {
On 2016/06/28 14:34:12, vabr (Chromium) wrote:
> Given that the exploded value comes from the result of base::Time::Now(), how
> could FromUTCExploded fail on it? It seems to me that a DCHECK is in order
here:
> 
> bool conversion_success = base::Time::FromUTCExploded(local, &out_time);
> DCHECK(conversion_success);
> 
> Please note that for reasons explained in my other comment, the conversion
must
> not be placed inside the DCHECK, and the DCHECK failure must not be handled.

Correct, it will never fail in this case.

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
On 2016/06/28 14:13:28, sfiera wrote:
> Here, we are combining YMD of a valid date (from LocalExplode()) with HMSm of
a
> valid time (06:00:00.000 or 22:00:00.000).
> 
> Can this fail?
> 
> I suppose if a country were to enter DST at 6AM, then for example
> 2017/03/01-06:00:00 might not exist locally [1]. In this case, we probably
want
> 7AM, but any approximation might be good enough. Can we approximate somehow? I
> imagine the old behavior on posix/mac might have been what we wanted.
> 
> [1] Or maybe I have it backwards and this kind of thing happens when leaving
> DST. I hate DST.

[1] Me neither.

Well, it seems like it shouldn't fail in this case. If you read the comments in
time.h, it says the conversion is treated as failed when a day of month is set
to 31 on a 28-30 day month.

In other words, what is done there is after time has been converted, it is
converted back to the exploded structure and checked if both the original and
converted times are same. That is treated as correct converted time.

https://codereview.chromium.org/2095553002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/password_man...
components/password_manager/core/browser/password_store.cc:181: if
(base::Time::FromUTCExploded(exploded_cutoff, &out_time)) {
On 2016/06/28 14:34:12, vabr (Chromium) wrote:
> This is called for a constant |exploded_cutoff|. Is that not a guarantee that
> this won't fail? If you agree, I sugest the following:
> 
> bool time_conversion_succeeded = base::Time::FromUTCExploded(exploded_cutoff,
> &ignore_logins_cutoff);
> DCHECK(time_conversion_succeeded);
> 
> Note 2 things:
> (1) The conversion itself must not be inside the DCHECK, or it won't happen in
> Release builds.
> (2) No attempt is made to handle the failing DCHECK-ed condition in Release
> builds, because the style guide forbids it
>
(http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).

Oh, correct. It will never ever fail. I haven't checked that. I have a bunch of
callers to be modified and I didn't get that deep into the code to check what
time was used.

https://codereview.chromium.org/2095553002/diff/40001/components/variations/v...
File components/variations/variations_seed_store.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/variations/v...
components/variations/variations_seed_store.cc:119: NOTIMPLEMENTED();
On 2016/06/28 15:27:40, Alexei Svitkine (very slow) wrote:
> Similar comment as vabr's - how can this fail? Should this be a NOTREACHED()
> instead?

In this case, DCHECK should be enough here as vabr@ says if you sure it doesn't
use some fancy times like 30th of February.

[vabr (Chromium), 2016-06-29 06:35:39]

Autofill and password_manager LGTM.
Thanks!
Vaclav

[Marc Treib, 2016-06-29 08:49:02]

ntp_snippets LGTM (thanks Chris for stepping in!)

[sfiera, 2016-06-29 09:15:11]

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
On 2016/06/29 05:48:59, maksims wrote:
> On 2016/06/28 14:13:28, sfiera wrote:
> > Here, we are combining YMD of a valid date (from LocalExplode()) with HMSm
of
> a
> > valid time (06:00:00.000 or 22:00:00.000).
> > 
> > Can this fail?
> > 
> > I suppose if a country were to enter DST at 6AM, then for example
> > 2017/03/01-06:00:00 might not exist locally [1]. In this case, we probably
> want
> > 7AM, but any approximation might be good enough. Can we approximate somehow?
I
> > imagine the old behavior on posix/mac might have been what we wanted.
> > 
> > [1] Or maybe I have it backwards and this kind of thing happens when leaving
> > DST. I hate DST.
> 
> [1] Me neither.
> 
> Well, it seems like it shouldn't fail in this case. If you read the comments
in
> time.h, it says the conversion is treated as failed when a day of month is set
> to 31 on a 28-30 day month.
> 
> In other words, what is done there is after time has been converted, it is
> converted back to the exploded structure and checked if both the original and
> converted times are same. That is treated as correct converted time.

TL;DR this code can fail in Western Greenland.

See http://crrev.com/2108823003. If we call FromLocalExploded() on a time that
we're skipping over due to DST, then it'll now fail. In these cases, we can
un-explode the time just fine (even on Windows, I think?), but when we explode
it again, we get a corresponding valid time, which doesn't match.

Most places don't enter DST at 10PM or 6AM (our times here), but Western
Greenland does. Previously, we would've gotten back 11PM. That's good enough for
us. Skipping the time is also probably good enough for us.

I guess the best thing to do here would be "return GetRescheduleTime(now +
base::TimeDelta::FromDays(1))". We might skip an update or two, but that's
better than rescheduling at Time(0).

[maksims (do not use this acc), 2016-06-30 07:33:14]

gentle reminder

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
On 2016/06/29 09:15:11, sfiera wrote:
> On 2016/06/29 05:48:59, maksims wrote:
> > On 2016/06/28 14:13:28, sfiera wrote:
> > > Here, we are combining YMD of a valid date (from LocalExplode()) with HMSm
> of
> > a
> > > valid time (06:00:00.000 or 22:00:00.000).
> > > 
> > > Can this fail?
> > > 
> > > I suppose if a country were to enter DST at 6AM, then for example
> > > 2017/03/01-06:00:00 might not exist locally [1]. In this case, we probably
> > want
> > > 7AM, but any approximation might be good enough. Can we approximate
somehow?
> I
> > > imagine the old behavior on posix/mac might have been what we wanted.
> > > 
> > > [1] Or maybe I have it backwards and this kind of thing happens when
leaving
> > > DST. I hate DST.
> > 
> > [1] Me neither.
> > 
> > Well, it seems like it shouldn't fail in this case. If you read the comments
> in
> > time.h, it says the conversion is treated as failed when a day of month is
set
> > to 31 on a 28-30 day month.
> > 
> > In other words, what is done there is after time has been converted, it is
> > converted back to the exploded structure and checked if both the original
and
> > converted times are same. That is treated as correct converted time.
> 
> TL;DR this code can fail in Western Greenland.
> 
> See http://crrev.com/2108823003. If we call FromLocalExploded() on a time that
> we're skipping over due to DST, then it'll now fail. In these cases, we can
> un-explode the time just fine (even on Windows, I think?), but when we explode
> it again, we get a corresponding valid time, which doesn't match.
> 
> Most places don't enter DST at 10PM or 6AM (our times here), but Western
> Greenland does. Previously, we would've gotten back 11PM. That's good enough
for
> us. Skipping the time is also probably good enough for us.
> 
> I guess the best thing to do here would be "return GetRescheduleTime(now +
> base::TimeDelta::FromDays(1))". We might skip an update or two, but that's
> better than rescheduling at Time(0).

Check the latests patch. Is it fine now? Did I understand you right?

[blundell, 2016-06-30 07:39:52]

On 2016/06/30 07:33:14, maksims wrote:
> gentle reminder

(If you're sending a ping on a patch with multiple reviewers, please specify who
the ping is for)

> 
>
https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
> File components/ntp_snippets/ntp_snippets_service.cc (right):
> 
>
https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_service.cc:144: if
> (!base::Time::FromLocalExploded(exploded, &reschedule)) {
> On 2016/06/29 09:15:11, sfiera wrote:
> > On 2016/06/29 05:48:59, maksims wrote:
> > > On 2016/06/28 14:13:28, sfiera wrote:
> > > > Here, we are combining YMD of a valid date (from LocalExplode()) with
HMSm
> > of
> > > a
> > > > valid time (06:00:00.000 or 22:00:00.000).
> > > > 
> > > > Can this fail?
> > > > 
> > > > I suppose if a country were to enter DST at 6AM, then for example
> > > > 2017/03/01-06:00:00 might not exist locally [1]. In this case, we
probably
> > > want
> > > > 7AM, but any approximation might be good enough. Can we approximate
> somehow?
> > I
> > > > imagine the old behavior on posix/mac might have been what we wanted.
> > > > 
> > > > [1] Or maybe I have it backwards and this kind of thing happens when
> leaving
> > > > DST. I hate DST.
> > > 
> > > [1] Me neither.
> > > 
> > > Well, it seems like it shouldn't fail in this case. If you read the
comments
> > in
> > > time.h, it says the conversion is treated as failed when a day of month is
> set
> > > to 31 on a 28-30 day month.
> > > 
> > > In other words, what is done there is after time has been converted, it is
> > > converted back to the exploded structure and checked if both the original
> and
> > > converted times are same. That is treated as correct converted time.
> > 
> > TL;DR this code can fail in Western Greenland.
> > 
> > See http://crrev.com/2108823003. If we call FromLocalExploded() on a time
that
> > we're skipping over due to DST, then it'll now fail. In these cases, we can
> > un-explode the time just fine (even on Windows, I think?), but when we
explode
> > it again, we get a corresponding valid time, which doesn't match.
> > 
> > Most places don't enter DST at 10PM or 6AM (our times here), but Western
> > Greenland does. Previously, we would've gotten back 11PM. That's good enough
> for
> > us. Skipping the time is also probably good enough for us.
> > 
> > I guess the best thing to do here would be "return GetRescheduleTime(now +
> > base::TimeDelta::FromDays(1))". We might skip an update or two, but that's
> > better than rescheduling at Time(0).
> 
> Check the latests patch. Is it fine now? Did I understand you right?

[sfiera, 2016-06-30 08:41:50]

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
On 2016/06/30 07:33:14, maksims wrote:
> On 2016/06/29 09:15:11, sfiera wrote:
> > On 2016/06/29 05:48:59, maksims wrote:
> > > On 2016/06/28 14:13:28, sfiera wrote:
> > > > Here, we are combining YMD of a valid date (from LocalExplode()) with
HMSm
> > of
> > > a
> > > > valid time (06:00:00.000 or 22:00:00.000).
> > > > 
> > > > Can this fail?
> > > > 
> > > > I suppose if a country were to enter DST at 6AM, then for example
> > > > 2017/03/01-06:00:00 might not exist locally [1]. In this case, we
probably
> > > want
> > > > 7AM, but any approximation might be good enough. Can we approximate
> somehow?
> > I
> > > > imagine the old behavior on posix/mac might have been what we wanted.
> > > > 
> > > > [1] Or maybe I have it backwards and this kind of thing happens when
> leaving
> > > > DST. I hate DST.
> > > 
> > > [1] Me neither.
> > > 
> > > Well, it seems like it shouldn't fail in this case. If you read the
comments
> > in
> > > time.h, it says the conversion is treated as failed when a day of month is
> set
> > > to 31 on a 28-30 day month.
> > > 
> > > In other words, what is done there is after time has been converted, it is
> > > converted back to the exploded structure and checked if both the original
> and
> > > converted times are same. That is treated as correct converted time.
> > 
> > TL;DR this code can fail in Western Greenland.
> > 
> > See http://crrev.com/2108823003. If we call FromLocalExploded() on a time
that
> > we're skipping over due to DST, then it'll now fail. In these cases, we can
> > un-explode the time just fine (even on Windows, I think?), but when we
explode
> > it again, we get a corresponding valid time, which doesn't match.
> > 
> > Most places don't enter DST at 10PM or 6AM (our times here), but Western
> > Greenland does. Previously, we would've gotten back 11PM. That's good enough
> for
> > us. Skipping the time is also probably good enough for us.
> > 
> > I guess the best thing to do here would be "return GetRescheduleTime(now +
> > base::TimeDelta::FromDays(1))". We might skip an update or two, but that's
> > better than rescheduling at Time(0).
> 
> Check the latests patch. Is it fine now? Did I understand you right?

I was suggesting:

  return GetRescheduleTime(now + base::TimeDelta::FromDays(1))

meaning, if we fail to get a reschedule time for today, get one for this time
tomorrow.

[Not at Google. Contact bengr, 2016-06-30 18:48:32]

Thanks for doing this.

https://codereview.chromium.org/2095553002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_usage_store.cc (right):

https://codereview.chromium.org/2095553002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_usage_store.cc:61: if
(!base::Time::FromUTCExploded(exploded, &out_time)) {
|exploded| is obtained by calling UTCExplode(..) above. 
Don't think this will ever fail. Suggest using DCHECK:

base::Time out_time;
bool conversion_success = base::Time::FromUTCExploded(exploded, &out_time)
DCHECK(conversion_success);

[Alexei Svitkine (slow), 2016-06-30 19:04:54]

lgtm for variations

[maksims (do not use this acc), 2016-07-04 07:34:04]

gentle reminder for hashimoto@, fukino@, megjablon@, kundaji@

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
On 2016/06/30 08:41:50, sfiera wrote:
> On 2016/06/30 07:33:14, maksims wrote:
> > On 2016/06/29 09:15:11, sfiera wrote:
> > > On 2016/06/29 05:48:59, maksims wrote:
> > > > On 2016/06/28 14:13:28, sfiera wrote:
> > > > > Here, we are combining YMD of a valid date (from LocalExplode()) with
> HMSm
> > > of
> > > > a
> > > > > valid time (06:00:00.000 or 22:00:00.000).
> > > > > 
> > > > > Can this fail?
> > > > > 
> > > > > I suppose if a country were to enter DST at 6AM, then for example
> > > > > 2017/03/01-06:00:00 might not exist locally [1]. In this case, we
> probably
> > > > want
> > > > > 7AM, but any approximation might be good enough. Can we approximate
> > somehow?
> > > I
> > > > > imagine the old behavior on posix/mac might have been what we wanted.
> > > > > 
> > > > > [1] Or maybe I have it backwards and this kind of thing happens when
> > leaving
> > > > > DST. I hate DST.
> > > > 
> > > > [1] Me neither.
> > > > 
> > > > Well, it seems like it shouldn't fail in this case. If you read the
> comments
> > > in
> > > > time.h, it says the conversion is treated as failed when a day of month
is
> > set
> > > > to 31 on a 28-30 day month.
> > > > 
> > > > In other words, what is done there is after time has been converted, it
is
> > > > converted back to the exploded structure and checked if both the
original
> > and
> > > > converted times are same. That is treated as correct converted time.
> > > 
> > > TL;DR this code can fail in Western Greenland.
> > > 
> > > See http://crrev.com/2108823003. If we call FromLocalExploded() on a time
> that
> > > we're skipping over due to DST, then it'll now fail. In these cases, we
can
> > > un-explode the time just fine (even on Windows, I think?), but when we
> explode
> > > it again, we get a corresponding valid time, which doesn't match.
> > > 
> > > Most places don't enter DST at 10PM or 6AM (our times here), but Western
> > > Greenland does. Previously, we would've gotten back 11PM. That's good
enough
> > for
> > > us. Skipping the time is also probably good enough for us.
> > > 
> > > I guess the best thing to do here would be "return GetRescheduleTime(now +
> > > base::TimeDelta::FromDays(1))". We might skip an update or two, but that's
> > > better than rescheduling at Time(0).
> > 
> > Check the latests patch. Is it fine now? Did I understand you right?
> 
> I was suggesting:
> 
>   return GetRescheduleTime(now + base::TimeDelta::FromDays(1))
> 
> meaning, if we fail to get a reschedule time for today, get one for this time
> tomorrow.

Done.

https://codereview.chromium.org/2095553002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_usage_store.cc (right):

https://codereview.chromium.org/2095553002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_usage_store.cc:61: if
(!base::Time::FromUTCExploded(exploded, &out_time)) {
On 2016/06/30 18:48:32, kundaji wrote:
> |exploded| is obtained by calling UTCExplode(..) above. 
> Don't think this will ever fail. Suggest using DCHECK:
> 
> base::Time out_time;
> bool conversion_success = base::Time::FromUTCExploded(exploded, &out_time)
> DCHECK(conversion_success);

Done.

[sfiera, 2016-07-04 07:48:20]

lgtm

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2095553002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:144: if
(!base::Time::FromLocalExploded(exploded, &reschedule)) {
On 2016/07/04 07:34:04, maksims wrote:
> On 2016/06/30 08:41:50, sfiera wrote:
> > On 2016/06/30 07:33:14, maksims wrote:
> > > On 2016/06/29 09:15:11, sfiera wrote:
> > > > On 2016/06/29 05:48:59, maksims wrote:
> > > > > On 2016/06/28 14:13:28, sfiera wrote:
> > > > > > Here, we are combining YMD of a valid date (from LocalExplode())
with
> > HMSm
> > > > of
> > > > > a
> > > > > > valid time (06:00:00.000 or 22:00:00.000).
> > > > > > 
> > > > > > Can this fail?
> > > > > > 
> > > > > > I suppose if a country were to enter DST at 6AM, then for example
> > > > > > 2017/03/01-06:00:00 might not exist locally [1]. In this case, we
> > probably
> > > > > want
> > > > > > 7AM, but any approximation might be good enough. Can we approximate
> > > somehow?
> > > > I
> > > > > > imagine the old behavior on posix/mac might have been what we
wanted.
> > > > > > 
> > > > > > [1] Or maybe I have it backwards and this kind of thing happens when
> > > leaving
> > > > > > DST. I hate DST.
> > > > > 
> > > > > [1] Me neither.
> > > > > 
> > > > > Well, it seems like it shouldn't fail in this case. If you read the
> > comments
> > > > in
> > > > > time.h, it says the conversion is treated as failed when a day of
month
> is
> > > set
> > > > > to 31 on a 28-30 day month.
> > > > > 
> > > > > In other words, what is done there is after time has been converted,
it
> is
> > > > > converted back to the exploded structure and checked if both the
> original
> > > and
> > > > > converted times are same. That is treated as correct converted time.
> > > > 
> > > > TL;DR this code can fail in Western Greenland.
> > > > 
> > > > See http://crrev.com/2108823003. If we call FromLocalExploded() on a
time
> > that
> > > > we're skipping over due to DST, then it'll now fail. In these cases, we
> can
> > > > un-explode the time just fine (even on Windows, I think?), but when we
> > explode
> > > > it again, we get a corresponding valid time, which doesn't match.
> > > > 
> > > > Most places don't enter DST at 10PM or 6AM (our times here), but Western
> > > > Greenland does. Previously, we would've gotten back 11PM. That's good
> enough
> > > for
> > > > us. Skipping the time is also probably good enough for us.
> > > > 
> > > > I guess the best thing to do here would be "return GetRescheduleTime(now
+
> > > > base::TimeDelta::FromDays(1))". We might skip an update or two, but
that's
> > > > better than rescheduling at Time(0).
> > > 
> > > Check the latests patch. Is it fine now? Did I understand you right?
> > 
> > I was suggesting:
> > 
> >   return GetRescheduleTime(now + base::TimeDelta::FromDays(1))
> > 
> > meaning, if we fail to get a reschedule time for today, get one for this
time
> > tomorrow.
> 
> Done.

Thanks! I also looked at the other changes you've made, and I think this is the
only one that suffered from a DST problem.

[maksims (do not use this acc), 2016-07-06 10:00:29]

hashimoto, noyau, kundaji, jwd, blundell, fukino, megjablon,
would you please review the following files/folders? 

components/data_reduction_proxy/core/browser/*
components/drive/file_system/touch_operation_unittest.cc
components/drive/resource_entry_conversion_unittest.cc
components/drive/service/fake_drive_service_unittest.cc

[fukino, 2016-07-06 11:14:23]

Hi,
I reviewed components/drive/

https://codereview.chromium.org/2095553002/diff/100001/components/drive/file_...
File components/drive/file_system/touch_operation_unittest.cc (right):

https://codereview.chromium.org/2095553002/diff/100001/components/drive/file_...
components/drive/file_system/touch_operation_unittest.cc:35: base::Time
LastAccessTimeUTC;
base::Time last_access_time_utc;
to follow the style guide.

https://codereview.chromium.org/2095553002/diff/100001/components/drive/file_...
components/drive/file_system/touch_operation_unittest.cc:36: base::Time
LastModifiedTimeUTC;
ditto

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
File components/drive/service/fake_drive_service_unittest.cc (right):

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
components/drive/service/fake_drive_service_unittest.cc:1069: base::Time
ModifiedDateUTC;
base::Time modified_date_utc;
to follow the style guide.

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
components/drive/service/fake_drive_service_unittest.cc:1165: base::Time
ModifiedDateUTC;
ditto

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
components/drive/service/fake_drive_service_unittest.cc:1166: base::Time
ViewedDateUTC;
ditto

[maksims (do not use this acc), 2016-07-06 12:00:20]

https://codereview.chromium.org/2095553002/diff/100001/components/drive/file_...
File components/drive/file_system/touch_operation_unittest.cc (right):

https://codereview.chromium.org/2095553002/diff/100001/components/drive/file_...
components/drive/file_system/touch_operation_unittest.cc:35: base::Time
LastAccessTimeUTC;
On 2016/07/06 11:14:23, fukino wrote:
> base::Time last_access_time_utc;
> to follow the style guide.

Done.

https://codereview.chromium.org/2095553002/diff/100001/components/drive/file_...
components/drive/file_system/touch_operation_unittest.cc:36: base::Time
LastModifiedTimeUTC;
On 2016/07/06 11:14:23, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
File components/drive/service/fake_drive_service_unittest.cc (right):

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
components/drive/service/fake_drive_service_unittest.cc:1069: base::Time
ModifiedDateUTC;
On 2016/07/06 11:14:23, fukino wrote:
> base::Time modified_date_utc;
> to follow the style guide.

Done.

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
components/drive/service/fake_drive_service_unittest.cc:1165: base::Time
ModifiedDateUTC;
On 2016/07/06 11:14:23, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2095553002/diff/100001/components/drive/servi...
components/drive/service/fake_drive_service_unittest.cc:1166: base::Time
ViewedDateUTC;
On 2016/07/06 11:14:23, fukino wrote:
> ditto

Done.

[Not at Google. Contact bengr, 2016-07-06 16:10:09]

lgtm

LGTM for components/data_reduction_proxy/*

[fukino, 2016-07-07 05:58:02]

components/drive lgtm

[maksims (do not use this acc), 2016-07-07 06:21:38]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-07-07 06:21:38]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
treib@chromium.org, asvitkine@chromium.org, sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2095553002/#ps120001
(title: "fukino's comments")

[commit-bot: I haz the power, 2016-07-07 06:21:59]

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-07-07 06:57:10]

Description was changed from

==========
Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with 
https://codereview.chromium.org/1988663002/ 

BUG=601900
==========

to

==========
Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with 
https://codereview.chromium.org/1988663002/ 

BUG=601900
==========

[commit-bot: I haz the power, 2016-07-07 06:57:12]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-07-07 06:57:19]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 07:00:06]

Description was changed from

==========
Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with 
https://codereview.chromium.org/1988663002/ 

BUG=601900
==========

to

==========
Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG=601900

Committed: https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011
Cr-Commit-Position: refs/heads/master@{#404095}
==========

[commit-bot: I haz the power, 2016-07-07 07:00:08]

Patchset 6 (id:??) landed as
https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011
Cr-Commit-Position: refs/heads/master@{#404095}