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

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 122 matching lines @@
   } else if (exploded.hour < kWifiFetchingHourMax) {
     exploded.hour = kWifiFetchingHourMax;
   } else {
     next_day = true;
     exploded.hour = kWifiFetchingHourMin;
   }
   // In any case, reschedule at the full hour.
   exploded.minute = 0;
   exploded.second = 0;
   exploded.millisecond = 0;
-  base::Time reschedule = base::Time::FromLocalExploded(exploded);
+  base::Time reschedule;
+  if (!base::Time::FromLocalExploded(exploded, &reschedule)) {

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

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.

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

[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.

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

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]

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

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

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.

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

Done.

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

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.

+    // TODO(maksims): implement error handling here.
+    // We might just return |reschedule|, which is Time(0) here.
+    NOTIMPLEMENTED();
+    return reschedule;
+  }
+
   if (next_day)
     reschedule += base::TimeDelta::FromDays(1);
 
   return reschedule;
 }
 
 // Extracts the hosts from |suggestions| and returns them in a set.
 std::set<std::string> GetSuggestionsHostsImpl(
     const SuggestionsProfile& suggestions) {
   std::set<std::string> hosts;
@@ skipping 652 matching lines @@
       return;
   }
 }
 
 void NTPSnippetsService::ClearDeprecatedPrefs() {
   pref_service_->ClearPref(prefs::kDeprecatedSnippets);
   pref_service_->ClearPref(prefs::kDeprecatedDiscardedSnippets);
 }
 
 }  // namespace ntp_snippets