Use base::GetBuildTime in MetricsLog::GetBuildTime instead of reimplementing it.

Use base::GetBuildTime in MetricsLog::GetBuildTime instead of reimplementing it.

BUG=314403, 326158
Committed: https://crrev.com/2f4ed5013d3dcdbcbeaabdad2af733dcdbfe067f
Cr-Commit-Position: refs/heads/master@{#302269}

[Sébastien Marchand, 2014-10-30 21:03:29]

Patchset #1 (id:1) has been deleted

[Sébastien Marchand, 2014-10-30 21:03:53]

sebmarchand@chromium.org changed reviewers:
+ asvitkine@chromium.org, maruel@chromium.org

[Sébastien Marchand, 2014-10-30 21:03:53]

PTAL.

[M-A Ruel, 2014-10-30 21:07:18]

lgtm

Alexei, any particular reason?

[Sébastien Marchand, 2014-10-30 22:04:38]

@Alexei, if this lgty please flip the CQ bit.

[Alexei Svitkine (slow), 2014-10-30 22:24:40]

On 2014/10/30 22:04:38, Sébastien Marchand wrote:
> @Alexei, if this lgty please flip the CQ bit.

I'm all for it - this is http://crbug.com/326158.

However, jar@ had an objection about it on the bug. I'm not sure I buy it
though.

[M-A Ruel, 2014-10-31 14:09:14]

https://codereview.chromium.org/693493006/diff/20001/components/metrics/metri...
File components/metrics/metrics_log.cc (left):

https://codereview.chromium.org/693493006/diff/20001/components/metrics/metri...
components/metrics/metrics_log.cc:161: static const char kDateTime[] = __DATE__
" " __TIME__ " GMT";
Note that the PST / GMT difference was pure hack; base/build_time.cc totally
assumes PST, this totally assumes GMT, both are *wrong* but this one was more
wrong.

The right fix is:
- Remove this implementation (e.g. this CL is right).
- Fix base/build_time.cc to stop assuming PST (which is wrong, it's PDT most of
the time, it assumes timezone settings on the official builder) and use a
resource instead of __TIME__. Other solutions includes toolset specific things
like
http://stackoverflow.com/questions/5844701/how-do-i-get-the-utc-offset-for-time.
This is out of scope for this CL.

I recommend:
- filing a bug for the timezone issue with base::GetBuildTime()
- closing https://code.google.com/p/chromium/issues/detail?id=326158 as Fixed,
as this CL uses ToTimeT() which converts to UTC.
- Take the hit with metrics 1-day data inconsistency while people are upgraded
due to the bug here.

[Alexei Svitkine (slow), 2014-10-31 14:13:25]

We should fix the timezone in the base one before switching this
implementation to use the base one. Else, the metrics values will have to
undergo two changes, rather than one, which is undesirable.

On Fri, Oct 31, 2014 at 10:09 AM, <maruel@chromium.org> wrote:

>
> https://codereview.chromium.org/693493006/diff/20001/
> components/metrics/metrics_log.cc
> File components/metrics/metrics_log.cc (left):
>
> https://codereview.chromium.org/693493006/diff/20001/
> components/metrics/metrics_log.cc#oldcode161
> components/metrics/metrics_log.cc:161: static const char kDateTime[] =
> __DATE__ " " __TIME__ " GMT";
> Note that the PST / GMT difference was pure hack; base/build_time.cc
> totally assumes PST, this totally assumes GMT, both are *wrong* but this
> one was more wrong.
>
> The right fix is:
> - Remove this implementation (e.g. this CL is right).
> - Fix base/build_time.cc to stop assuming PST (which is wrong, it's PDT
> most of the time, it assumes timezone settings on the official builder)
> and use a resource instead of __TIME__. Other solutions includes toolset
> specific things like
> http://stackoverflow.com/questions/5844701/how-do-i-
> get-the-utc-offset-for-time.
> This is out of scope for this CL.
>
> I recommend:
> - filing a bug for the timezone issue with base::GetBuildTime()
> - closing https://code.google.com/p/chromium/issues/detail?id=326158 as
> Fixed, as this CL uses ToTimeT() which converts to UTC.
> - Take the hit with metrics 1-day data inconsistency while people are
> upgraded due to the bug here.
>
> https://codereview.chromium.org/693493006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[M-A Ruel, 2014-10-31 14:25:34]

On 2014/10/31 14:13:25, Alexei Svitkine wrote:
> We should fix the timezone in the base one before switching this
> implementation to use the base one. Else, the metrics values will have to
> undergo two changes, rather than one, which is undesirable.

Ok, I took the bait. Both on OSX and Ubuntu:

$ cat > a.cc
#include <stdio.h>
int main() {printf("%s\n", __TIME__); return 0;}

$ gcc a.cc; ./a.out 
10:17:10
$ ~/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang++ a.cc;
./a.out
10:19:07

As you can see, I took me a long time to find that clang build...


On MSVC:
> echo #include ^<stdio.h^>> a.cc
> echo int main() {printf("%s\n", __TIME__); return 0;}>> a.cc
> cl a.cc
> a
10:22:48

I blame booting up Windows for being that slow. :)

The implementation in base is doing the *less worse* thing. That is, assume
__TIME__ is in local time, which is true on all toolset we care about. So the
only bug remaining is PDT vs PST but that's a lesser issue (4% error)

[Alexei Svitkine (slow), 2014-10-31 14:32:03]

lgtm

[Sébastien Marchand, 2014-10-31 14:37:13]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-10-31 14:38:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693493006/20001

[commit-bot: I haz the power, 2014-10-31 15:28:26]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2014-10-31 15:29:47]

Patchset 1 (id:??) landed as
https://crrev.com/2f4ed5013d3dcdbcbeaabdad2af733dcdbfe067f
Cr-Commit-Position: refs/heads/master@{#302269}