Move HashMetricName to base namespace.

Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

Depends on
https://codereview.chromium.org/1471073010

BUG=546019
Committed: https://crrev.com/502b229004bece63acb26aa53888626fddd0fccb
Cr-Commit-Position: refs/heads/master@{#362244}

[bcwhite, 2015-11-25 20:58:28]

Description was changed from

==========
Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

BUG=546019
==========

to

==========
Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

Depends on
https://codereview.chromium.org/1471073010

BUG=546019
==========

[bcwhite, 2015-11-25 20:58:59]

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

[Alexei Svitkine (slow), 2015-11-25 21:05:40]

You have some test failures. Otherwise looks good, have some comments below.

https://codereview.chromium.org/1476633003/diff/1/base/metrics/BUILD.gn
File base/metrics/BUILD.gn (right):

https://codereview.chromium.org/1476633003/diff/1/base/metrics/BUILD.gn#newco...
base/metrics/BUILD.gn:54: "metrics_hashes_unittest.cc",
The other unit tests in this folder are listed here:

https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn&l=1372

Probably makes sense to keep them consistent and put it there too.

https://codereview.chromium.org/1476633003/diff/1/base/metrics/metrics_hashes.cc
File base/metrics/metrics_hashes.cc (right):

https://codereview.chromium.org/1476633003/diff/1/base/metrics/metrics_hashes...
base/metrics/metrics_hashes.cc:20: return base::NetToHost64(value);
I think it should still be HostToNet64().

Technically, they're identical, since both conversions do a byteswap. But in
this case, "value" is host defined and we want to swap to network byte order,
no?

[bcwhite, 2015-11-25 22:58:26]

A lot of builds are failing because the existing sys_byteorder.h includes files
that aren't available in some places.

It needs https://codereview.chromium.org/1471073010 to run but that hasn't
received approval and tomorrow is American Thanksgiving so likely won't get any
until next week.

I've done a "build all" on my workstation and everything compiles.

https://codereview.chromium.org/1476633003/diff/1/base/metrics/BUILD.gn
File base/metrics/BUILD.gn (right):

https://codereview.chromium.org/1476633003/diff/1/base/metrics/BUILD.gn#newco...
base/metrics/BUILD.gn:54: "metrics_hashes_unittest.cc",
On 2015/11/25 21:05:40, Alexei Svitkine (slow) wrote:
> The other unit tests in this folder are listed here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn&l=1372
> 
> Probably makes sense to keep them consistent and put it there too.

Ahh, there they are.  Will do.

https://codereview.chromium.org/1476633003/diff/1/base/metrics/metrics_hashes.cc
File base/metrics/metrics_hashes.cc (right):

https://codereview.chromium.org/1476633003/diff/1/base/metrics/metrics_hashes...
base/metrics/metrics_hashes.cc:20: return base::NetToHost64(value);
On 2015/11/25 21:05:40, Alexei Svitkine (slow) wrote:
> I think it should still be HostToNet64().
> 
> Technically, they're identical, since both conversions do a byteswap. But in
> this case, "value" is host defined and we want to swap to network byte order,
> no?

It's a raw byte-stream of hash that we're converting to a host's 64-bit integer.
 Not technically "net" but is "to host" at least.

[Alexei Svitkine (slow), 2015-11-26 15:38:48]

lgtm

[bcwhite, 2015-11-26 19:14:13]

Patchset #2 (id:20001) has been deleted

[bcwhite, 2015-11-26 19:15:19]

bcwhite@chromium.org changed reviewers:
+ mark@chromium.org

[bcwhite, 2015-11-26 19:15:20]

mark@chromium.org: Please review changes in 

base/BUILD.gn
base/base.gyp*

[Mark Mentovai, 2015-11-30 18:49:57]

https://codereview.chromium.org/1476633003/diff/40001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes.cc (left):

https://codereview.chromium.org/1476633003/diff/40001/base/metrics/metrics_ha...
base/metrics/metrics_hashes.cc:18: DCHECK_GE(arraysize(digest.a),
sizeof(value));
Oops!

https://codereview.chromium.org/1476633003/diff/40001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes.h (right):

https://codereview.chromium.org/1476633003/diff/40001/base/metrics/metrics_ha...
base/metrics/metrics_hashes.h:17: BASE_EXPORT uint64 HashMetricName(const
std::string& name);
Can this be <stdint.h> uint64_t now? (remove basictypes.h). Same in the .cc file
and test. You don’t really need to worry about call sites now, but it’s as good
a time to fix this here as any since this is a “new” file. See
https://crbug.com/138542.

[Mark Mentovai, 2015-11-30 18:50:16]

LGTM with or without that change (but I’d prefer with)

[bcwhite, 2015-11-30 19:13:20]

https://codereview.chromium.org/1476633003/diff/40001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes.h (right):

https://codereview.chromium.org/1476633003/diff/40001/base/metrics/metrics_ha...
base/metrics/metrics_hashes.h:17: BASE_EXPORT uint64 HashMetricName(const
std::string& name);
On 2015/11/30 18:49:56, Mark Mentovai wrote:
> Can this be <stdint.h> uint64_t now? (remove basictypes.h). Same in the .cc
file
> and test. You don’t really need to worry about call sites now, but it’s as
good
> a time to fix this here as any since this is a “new” file. See
> https://crbug.com/138542.

Done.

[Mark Mentovai, 2015-11-30 19:20:23]

LGTM

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes.h (right):

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
base/metrics/metrics_hashes.h:11: #include "base/basictypes.h"
#include <stdint.h> instead of this.

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes_unittest.cc (right):

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
base/metrics/metrics_hashes_unittest.cc:25: uint64 hash =
HashMetricName(cases[i].input);
uint64_t here too

[bcwhite, 2015-11-30 20:07:47]

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes.h (right):

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
base/metrics/metrics_hashes.h:11: #include "base/basictypes.h"
On 2015/11/30 19:20:23, Mark Mentovai wrote:
> #include <stdint.h> instead of this.

Done.

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes_unittest.cc (right):

https://codereview.chromium.org/1476633003/diff/60001/base/metrics/metrics_ha...
base/metrics/metrics_hashes_unittest.cc:25: uint64 hash =
HashMetricName(cases[i].input);
On 2015/11/30 19:20:23, Mark Mentovai wrote:
> uint64_t here too

Doh.  Forgot the test file.  Done.

[Mark Mentovai, 2015-11-30 20:24:54]

LGTM

[bcwhite, 2015-11-30 20:28:31]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2015-11-30 20:28:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1476633003/#ps80001
(title: "fixed unittest")

[commit-bot: I haz the power, 2015-11-30 20:28:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476633003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476633003/80001

[commit-bot: I haz the power, 2015-11-30 22:31:25]

Description was changed from

==========
Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

Depends on
https://codereview.chromium.org/1471073010

BUG=546019
==========

to

==========
Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

Depends on
https://codereview.chromium.org/1471073010

BUG=546019
==========

[commit-bot: I haz the power, 2015-11-30 22:31:26]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-11-30 22:32:14]

Description was changed from

==========
Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

Depends on
https://codereview.chromium.org/1471073010

BUG=546019
==========

to

==========
Move HashMetricName to base namespace.

New code that will reside in base will need access to
these methods. (cl/1471073007)

Depends on
https://codereview.chromium.org/1471073010

BUG=546019

Committed: https://crrev.com/502b229004bece63acb26aa53888626fddd0fccb
Cr-Commit-Position: refs/heads/master@{#362244}
==========

[commit-bot: I haz the power, 2015-11-30 22:32:15]

Patchset 4 (id:??) landed as
https://crrev.com/502b229004bece63acb26aa53888626fddd0fccb
Cr-Commit-Position: refs/heads/master@{#362244}