Porting refactoring changes:...

Porting refactoring changes:

- move env_util into base/sys_info
- move rand_util to base and simplify its public interface

Please test it on Windows. I had to touch some files which
are not yet included in Linux build. The changes are basically find/replace
in them, but mistakes may happen.

This is related to http://codereview.chromium.org/4247 (porting in browser/ )

[Dean McNamee, 2008-09-25 12:46:44]

Overall looks good.  Might want to mention something about thread safety for
RandInt, and maybe add a RandFloat.  We really care more about thread safety
that secureness, because we don't really use random generators for anything
important (except random process selection for new tabs).

Sorry to make you do a bunch of refactoring.

Anybody else have thoughts on rand_util and env_util.  Should they be their own
files, or is there a better place for them to live?

http://codereview.chromium.org/4079/diff/1/7
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/1/7#newcode19
Line 19: #if defined(OS_WIN)
We should probably just break this into two files.  It's a bit of a pain I
guess, but oh well.  rand_util_win and rand_util_posix.  It's nicer than ifdef's
everywhere.

http://codereview.chromium.org/4079/diff/1/9
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/1/9#newcode11
Line 11: // Cryptographically secure.
I don't think we should guarantee this, because we won't necessarily use a
"cryptographically" secure, and we shouldn't depend on it for crypto.  We should
just have a "good" one.

http://codereview.chromium.org/4079/diff/1/5
File base/sys_info.h (right):

http://codereview.chromium.org/4079/diff/1/5#newcode35
Line 35: static std::wstring GetEnvironmentVariable(const char* var);
I think maybe the two Environment ones could find a better home than sys_info,
but I'm not sure where.  I think the ones below qualify for sys_info.

http://codereview.chromium.org/4079/diff/1/2
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/4079/diff/1/2#newcode89
Line 89: std::string SysInfo::OperatingSystemName() {
Again, maybe we can just get this from uname() instead?

http://codereview.chromium.org/4079/diff/1/2#newcode102
Line 102: return "n/a";
Maybe call uname(2) here, and piece together whatever from release/version. 
Something like the result of uname -r is probably good.

http://codereview.chromium.org/4079/diff/1/2#newcode106
Line 106: std::string SysInfo::CPUArchitecture() {
uname -m ?

http://codereview.chromium.org/4079/diff/1/8
File base/sys_info_unittest.cc (right):

http://codereview.chromium.org/4079/diff/1/8#newcode38
Line 38: // We aren't actually testing that it's correct, just that it's sane.
This will blow the NOTIMPLEMENTED in a debug build for Linux, no?

http://codereview.chromium.org/4079/diff/1/13
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4079/diff/1/13#newcode72
Line 72: back_off_fuzz_ = static_cast<float>(base::RandInt(1, INT_MAX)) /
INT_MAX;
Do we want a RandFloat() that returns between [0, 1) ?

[Mark Mentovai, 2008-09-25 14:00:23]

This mostly looks good, just a few comments.

http://codereview.chromium.org/4079/diff/1/9
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/1/9#newcode11
Line 11: // Cryptographically secure.
deanm wrote:
> I don't think we should guarantee this, because we won't necessarily use a
> "cryptographically" secure, and we shouldn't depend on it for crypto.  We
should
> just have a "good" one.

I agree.  In fact, the POSIX implementation here isn't even cryptographically
secure.

http://codereview.chromium.org/4079/diff/1/5
File base/sys_info.h (right):

http://codereview.chromium.org/4079/diff/1/5#newcode41
Line 41: static std::string OperatingSystemVersion();
Maybe we should define that this is just major.minor without any patch
level/service pack/whatever else information included.

http://codereview.chromium.org/4079/diff/1/5#newcode43
Line 43: // Returns the CPU architecture of the system.
Is it important to define what strings we use for what architectures, for
compatibility between platforms?

http://codereview.chromium.org/4079/diff/1/2
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/4079/diff/1/2#newcode91
Line 91: return "Linux";
These names obviously aren't being localized here.  Where are we going to use
these?

http://codereview.chromium.org/4079/diff/1/2#newcode100
Line 100: std::string SysInfo::OperatingSystemVersion() {
At the very least, OSVer and the display stuff will have different
implementations on the Mac.

http://codereview.chromium.org/4079/diff/1/8
File base/sys_info_unittest.cc (right):

http://codereview.chromium.org/4079/diff/1/8#newcode28
Line 28: // Every setup should have non-empty PATH...
Eh, I don't know how I feel about this...

http://codereview.chromium.org/4079/diff/1/4
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/1/4#newcode70
Line 70: return "Windows";
If we're using OperatingSystemVersion's numbers, we might want to call this
"Windows NT", and maybe check dwPlatformId == VER_PLATFORM_WIN32_NT.

http://codereview.chromium.org/4079/diff/1/4#newcode80
Line 80: memset(result, 0, arraysize(result));
Better: char result[20] = {0};

http://codereview.chromium.org/4079/diff/1/4#newcode83
Line 83: return std::string(result);
Even better: since we want a std::string, just use StringPrintf from
base/string_util.h.

http://codereview.chromium.org/4079/diff/1/19
File chrome/browser/render_process_host.h (right):

http://codereview.chromium.org/4079/diff/1/19#newcode274
Line 274: base::RandInt(0, kint32max));
INT_MAX, because we're using int, not int32.

http://codereview.chromium.org/4079/diff/1/15
File chrome/browser/views/bug_report_view.cc (right):

http://codereview.chromium.org/4079/diff/1/15#newcode333
Line 333: //   base::SysInfo::OperatingSystemVersion returns an std::string
Because of this, maybe sys_info needs an OperatingSystemVersionComplete that
includes patchlevel/service pack too.

[please use my chromium address, 2008-09-25 15:25:20]

http://codereview.chromium.org/4079/diff/1/7
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/1/7#newcode19
Line 19: #if defined(OS_WIN)
On 2008/09/25 12:46:44, deanm wrote:
> We should probably just break this into two files.  It's a bit of a pain I
> guess, but oh well.  rand_util_win and rand_util_posix.  It's nicer than
ifdef's
> everywhere.

Done. I could remove #error now, and there is small duplication betwen _posix
and _win versions. What would you think about additional function RandInt() with
no arguments, which would be platform-dependent, and then platform-independent
function to return a random int in given range?

http://codereview.chromium.org/4079/diff/1/9
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/1/9#newcode11
Line 11: // Cryptographically secure.
On 2008/09/25 12:46:44, deanm wrote:
> I don't think we should guarantee this, because we won't necessarily use a
> "cryptographically" secure, and we shouldn't depend on it for crypto.  We
should
> just have a "good" one.

Done.

http://codereview.chromium.org/4079/diff/1/5
File base/sys_info.h (right):

http://codereview.chromium.org/4079/diff/1/5#newcode41
Line 41: static std::string OperatingSystemVersion();
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Maybe we should define that this is just major.minor without any patch
> level/service pack/whatever else information included.

I don't know how best to solve it. On Linux this would be kernel version number.
The meaning depends on operating system name anyway.

http://codereview.chromium.org/4079/diff/1/5#newcode43
Line 43: // Returns the CPU architecture of the system.
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Is it important to define what strings we use for what architectures, for
> compatibility between platforms?

(leaving quote here so that this question doesn't get lost)

http://codereview.chromium.org/4079/diff/1/2
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/4079/diff/1/2#newcode89
Line 89: std::string SysInfo::OperatingSystemName() {
On 2008/09/25 12:46:44, deanm wrote:
> Again, maybe we can just get this from uname() instead?

Great, I didn't think about it. Done.

http://codereview.chromium.org/4079/diff/1/2#newcode91
Line 91: return "Linux";
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> These names obviously aren't being localized here.  Where are we going to use
> these?

(quoting to prevent the question getting lost). Note: with uname it won't be
localized either. And it will be almost certainly harder to localize. The only
usage I found was browser/metrics_log.cc (for OperatingSystemName).

http://codereview.chromium.org/4079/diff/1/8
File base/sys_info_unittest.cc (right):

http://codereview.chromium.org/4079/diff/1/8#newcode28
Line 28: // Every setup should have non-empty PATH...
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Eh, I don't know how I feel about this...

Ok, this is just a crude sanity test... Should work, but probably doesn't help
much either. Remove it?

http://codereview.chromium.org/4079/diff/1/8#newcode38
Line 38: // We aren't actually testing that it's correct, just that it's sane.
On 2008/09/25 12:46:44, deanm wrote:
> This will blow the NOTIMPLEMENTED in a debug build for Linux, no?

Probably removing it is the best choice for now, then.

http://codereview.chromium.org/4079/diff/1/4
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/1/4#newcode70
Line 70: return "Windows";
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> If we're using OperatingSystemVersion's numbers, we might want to call this
> "Windows NT", and maybe check dwPlatformId == VER_PLATFORM_WIN32_NT.

I'm not sure how to do it. Should I get dwPlatformId using GetVersionEx? And one
more thing - it seems all Windows versions Chrome runs on are NT. Sorry for my
doubts, I'm just unfamiliar with programming on Windows.

http://codereview.chromium.org/4079/diff/1/4#newcode83
Line 83: return std::string(result);
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Even better: since we want a std::string, just use StringPrintf from
> base/string_util.h.

Done.

http://codereview.chromium.org/4079/diff/1/19
File chrome/browser/render_process_host.h (right):

http://codereview.chromium.org/4079/diff/1/19#newcode274
Line 274: base::RandInt(0, kint32max));
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> INT_MAX, because we're using int, not int32.

Done.

http://codereview.chromium.org/4079/diff/1/13
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4079/diff/1/13#newcode72
Line 72: back_off_fuzz_ = static_cast<float>(base::RandInt(1, INT_MAX)) /
INT_MAX;
On 2008/09/25 12:46:44, deanm wrote:
> Do we want a RandFloat() that returns between [0, 1) ?

Added it, looks nice for me. Please check.

http://codereview.chromium.org/4079/diff/1/15
File chrome/browser/views/bug_report_view.cc (right):

http://codereview.chromium.org/4079/diff/1/15#newcode333
Line 333: //   base::SysInfo::OperatingSystemVersion returns an std::string
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Because of this, maybe sys_info needs an OperatingSystemVersionComplete that
> includes patchlevel/service pack too.

Maybe I should add a TODO in SysInfo? Unfortunately I'm not familiar with
Windows programming.

[sgk, 2008-09-25 20:04:10]

SCons lgtm

[please use my chromium address, 2008-09-26 15:37:36]

*ping* - please review my updates (SCons is LGTM-ed).

[Dean McNamee, 2008-09-26 15:58:57]

Overall looks really good.  A bunch of comments.  I will need to look at the
random stuff more closely to make sure everything we're doing is correct.  I can
commit most of this on Monday.

http://codereview.chromium.org/4079/diff/69/276
File base/build/base.vcproj (right):

http://codereview.chromium.org/4079/diff/69/276#newcode563
Line 563: </File>
And rand_util_win

http://codereview.chromium.org/4079/diff/69/277
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/69/277#newcode12
Line 12: return static_cast<float>(base::RandInt(1, INT_MAX)) / INT_MAX;
Should this be 0, INT_MAX ?  Also, I think most APIs that do this, they don't
include 1, so it's 0 - .9999

http://codereview.chromium.org/4079/diff/69/280
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/4079/diff/69/280#newcode17
Line 17: FILE* urandom = fopen("/dev/urandom", "r");
Instead I would:

if (!urandom) {
  CHECK(false);  // we should also crash in release.
}

http://codereview.chromium.org/4079/diff/69/281
File base/rand_util_win.cc (right):

http://codereview.chromium.org/4079/diff/69/281#newcode19
Line 19: DCHECK(rv == 0) << "rand_s failed with error " << rv;
Again probably a CHECK, since we want this to happen in release, if we're
depending on randomness for something.  Also, can probably drop the string and
just do CHECK(rv == 0), otherwise we carry the string around for a very unlikely
case.

http://codereview.chromium.org/4079/diff/69/281#newcode24
Line 24: (number / (UINT_MAX + 1.0)));
This seems a bit complicated, let me think about it...

http://codereview.chromium.org/4079/diff/69/275
File base/sys_info.h (right):

http://codereview.chromium.org/4079/diff/69/275#newcode32
Line 32: static bool HasEnvironmentVariable(const char* var);
We should file a cleanup bug to find these a better home, but it's ok for now.

http://codereview.chromium.org/4079/diff/69/272
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/4079/diff/69/272#newcode93
Line 93: return "POSIX/compatible";  // That's all we know. Better than nothing.
Maybe also a NOTREACHED() before the return, so we crash in debug.

Maybe an empty string is better?

http://codereview.chromium.org/4079/diff/69/272#newcode101
Line 101: if (uname(&info) < 0) {
NOTREACHED()

Maybe an empty string is better?

http://codereview.chromium.org/4079/diff/69/272#newcode110
Line 110: if (uname(&info) < 0) {
NOTREACHED()

Maybe an empty string is better?

http://codereview.chromium.org/4079/diff/69/274
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/69/274#newcode55
Line 55: wchar_t* value = malloc(value_length * sizeof(*value));
We don't use malloc, and you leak value?  Maybe use a ScopedArray<> and new
wchar[123];

http://codereview.chromium.org/4079/diff/69/291
File chrome/browser/render_process_host.h (right):

http://codereview.chromium.org/4079/diff/69/291#newcode274
Line 274: base::RandInt(0, INT_MAX));
Why drop the kint32max, I think we prefer that over INT_MAX.

http://codereview.chromium.org/4079/diff/69/298
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/4079/diff/69/298#newcode19
Line 19: return static_cast<uint32>(base::RandInt(0, kint32max));
Not your fault, but isn't this code a bit incorrect?  It's only going to get 31
bits for a hash?

[please use my chromium address, 2008-09-26 18:20:58]

On 2008/09/26 15:58:57, deanm wrote:
\> http://codereview.chromium.org/4079/diff/69/277#newcode12
> Line 12: return static_cast<float>(base::RandInt(1, INT_MAX)) / INT_MAX;
> Should this be 0, INT_MAX ?  Also, I think most APIs that do this, they don't
> include 1, so it's 0 - .9999

I changed it to return number in range [0, 1) - please check for correctness.

> http://codereview.chromium.org/4079/diff/69/281#newcode24
> Line 24: (number / (UINT_MAX + 1.0)));
> This seems a bit complicated, let me think about it...

I changed it to something simple. It looks correct to me, but please double
check.

> http://codereview.chromium.org/4079/diff/69/274#newcode55
> Line 55: wchar_t* value = malloc(value_length * sizeof(*value));
> We don't use malloc, and you leak value?  Maybe use a ScopedArray<> and new
> wchar[123];

Ooops. Corrected. But not checked, as this is a Windows-only file ATM.

> http://codereview.chromium.org/4079/diff/69/291
> File chrome/browser/render_process_host.h (right):
> 
> http://codereview.chromium.org/4079/diff/69/291#newcode274
> Line 274: base::RandInt(0, INT_MAX));
> Why drop the kint32max, I think we prefer that over INT_MAX.

Changed using std::numeric_limits as you said on chat.

> http://codereview.chromium.org/4079/diff/69/298
> File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):
> 
> http://codereview.chromium.org/4079/diff/69/298#newcode19
> Line 19: return static_cast<uint32>(base::RandInt(0, kint32max));
> Not your fault, but isn't this code a bit incorrect?  It's only going to get
31
> bits for a hash?

To be checked, as said on chat.

Rest of things you wrote about are fixed.

Please remember about issues pointed by Mark Mentovai. I would need some more
guidance to resolve them.

[eroman, 2008-09-27 02:24:06]

Cool, thanks for doing this.
I also need rand_util to be in base/, so that it can be used from the net
module.

http://codereview.chromium.org/4079/diff/406/414
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/406/414#newcode6
Line 6: #define BASE_RAND_UTIL_H__
drive by nit: can you change this to be a single trailing underscore?
The google style guide was amended since the original file.

http://codereview.chromium.org/4079/diff/406/414#newcode8
Line 8: // These functions are not guaranteed to be cryptographically secure.
It would be great to document that implementations are guaranteed thread-safe.

[please use my chromium address, 2008-09-27 06:41:54]

On 2008/09/27 02:24:06, eroman wrote:
> http://codereview.chromium.org/4079/diff/406/414
> File base/rand_util.h (right):
> 
> http://codereview.chromium.org/4079/diff/406/414#newcode6
> Line 6: #define BASE_RAND_UTIL_H__
> drive by nit: can you change this to be a single trailing underscore?
> The google style guide was amended since the original file.

Done.

> http://codereview.chromium.org/4079/diff/406/414#newcode8
> Line 8: // These functions are not guaranteed to be cryptographically secure.
> It would be great to document that implementations are guaranteed thread-safe.

I documented it in a per-function manner, so it will be harder to miss it when
making further changes.

On POSIX simultaneous reading /dev/urandom is thread-safe, and in Windows rand_s
is thread-safe because Chrome links with CRT using /MTd switch (see
http://msdn.microsoft.com/en-us/library/abx4dbyh(VS.80).aspx).

[Mark Mentovai, 2008-09-27 19:50:17]

http://codereview.chromium.org/4079/diff/439/605
File base/build/base.vcproj (right):

http://codereview.chromium.org/4079/diff/439/605#newcode565
Line 565: RelativePath=".\rand_util_posix.cc"
This is the vcproj, which is only used for the Windows build.  I don't think we
want to build rand_util_posix on Windows.

http://codereview.chromium.org/4079/diff/439/606
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/439/606#newcode15
Line 15: return static_cast<float>(base::RandInt(0, int_max - 1)) / int_max;
This is the naïve implementation, which may be OK for our purposes, but carries
minimal precision through from the generator to the floating-point number.  If
you're interested, take a look at an implementation of drand48, which carries a
full 48 bits of precision.  The idea is that RandFloat() * n should carry
maximum precision to a range of [0, n).  Also see
http://c-faq.com/lib/rand48.html.

Why are we using float and not double here?

http://codereview.chromium.org/4079/diff/439/608
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/439/608#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Nit: just 2008 on new files.  Same comment in other newly-added files.

http://codereview.chromium.org/4079/diff/439/609
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/4079/diff/439/609#newcode13
Line 13: int RandInt(int min, int max) {
Note: both the POSIX and Windows implementations here do not have uniform
distribution for certain weird and large (relative to UINT_MAX) but valid values
of (max - min).  As an example, assume a simple RNG with a range [0, 10), and a
call to RandInt(0, 8).  You'll get uniform distribution of 0s and 1s, and
uniform distribution of 2s through 7s, but you'll have more 1s than 2s.

http://codereview.chromium.org/4079/diff/439/609#newcode17
Line 17: FILE* urandom = fopen("/dev/urandom", "r");
I don't know that I'd bring in stdio to do this simple read instead of just
using raw open/read/close directly.

http://codereview.chromium.org/4079/diff/439/609#newcode23
Line 23: return min + number % (max - min + 1);
This will crash in exactly one very important case:

RandInt(INT_MIN, INT_MAX)

INT_MAX - INT_MIN + 1 = 0 :(

Same goes for the Windows version.

http://codereview.chromium.org/4079/diff/439/610
File base/rand_util_win.cc (right):

http://codereview.chromium.org/4079/diff/439/610#newcode20
Line 20: return min + number % (max - min + 1);
The comments (except for file I/O) from the POSIX implementation also apply
here.

http://codereview.chromium.org/4079/diff/439/604
File base/sys_info.h (right):

http://codereview.chromium.org/4079/diff/439/604#newcode31
Line 31: // Return true if the given environment variable is defined.
We want to eventually find a better home for the environment variable stuff,
right?  If so, add a TODO.  If not, ignore this comment.  (I just thought I
remembered seeing some talk between you and Dean on this point.)

http://codereview.chromium.org/4079/diff/439/604#newcode34
Line 34: // Return value of given environment variable or empty string if not
defined.
Proper grammar: returns THE value of THE given environment variable or AN empty
string

http://codereview.chromium.org/4079/diff/439/601
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/4079/diff/439/601#newcode85
Line 85: bool SysInfo::HasEnvironmentVariable(const char* var) {
The .h file puts this before GetEnvironmentVariable, since everything else
follows the order in the header, these guys should match.

http://codereview.chromium.org/4079/diff/439/603
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/439/603#newcode56
Line 56: scoped_array<wchar_t> value(new wchar_t[value_length *
sizeof(*value)]);
what's sizeof(*value) doing there?

value_length doesn't include a terminating NUL character per the Windows
documentation.  Either add space for it, or pass value_length as an argument to
the wstring constructor below, or both.

http://codereview.chromium.org/4079/diff/439/603#newcode58
Line 58: value, value_length);
That doesn't seem right.  This should be value.get(), right?  Or &value[0].

(I'd prefer a std::vector<wchar_t> anyway, but I know Dean told you to do
scoped_array, so that's fine...)

http://codereview.chromium.org/4079/diff/439/603#newcode71
Line 71: return "Windows";
Again, I'd prefer "Windows NT" so that OperatingSystemVersion at least
corresponds to this.

http://codereview.chromium.org/4079/diff/439/617
File chrome/browser/importer/importer_unittest.cc (left):

http://codereview.chromium.org/4079/diff/439/617#oldcode277
Line 277: // Skips in Win2000 for the running environment can not be set up.
Nice, thanks!

http://codereview.chromium.org/4079/diff/439/615
File chrome/browser/metrics_log.cc (right):

http://codereview.chromium.org/4079/diff/439/615#newcode473
Line 473: base::SysInfo::GetOperatingSystemVersion());
Should be OperatingSystemVersion.

http://codereview.chromium.org/4079/diff/439/627
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/4079/diff/439/627#newcode19
Line 19: return static_cast<uint32>(base::RandInt(0, kint32max));
Don't we want kuint32max here?

http://codereview.chromium.org/4079/diff/439/614
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4079/diff/439/614#newcode279
Line 279: next_update_sec_ = base::RandInt(15, 45) * 60;
15 to 45 minutes.  Is that right?  15 * 60, 45 * 60 might be clearer then.

http://codereview.chromium.org/4079/diff/439/623
File chrome/common/process_watcher.cc (right):

http://codereview.chromium.org/4079/diff/439/623#newcode52
Line 52: // If running the distributed tests, give the renderer a little time to
figure out
Fix this to be less than 80 characters since you're working right here.

http://codereview.chromium.org/4079/diff/439/612
File chrome/test/automated_ui_tests/automated_ui_tests.cc (right):

http://codereview.chromium.org/4079/diff/439/612#newcode708
Line 708: - 1);
The - 1 can fit on the preceding line now?  If not, at least indent it by an
extra 4 spaces so that it's clear it's attached to arraysize and is not an
argument of its own.

[Dean McNamee, 2008-09-28 13:31:51]

Oh man, Mark pointed out a really good issue on the distribution.  I also agree
with making in double.  Sorry this has gotten so complicated.

Why don't we read 8 bytes from /dev/urandom into a uint64, then divide it into a
double, or something?  We can then just call rand_s twice to get 64 bits on
Windows.  This should give use a much smaller "step".  Then, we use
RandomDouble() to implement the min / max.

Mark, thoughts?

On 2008/09/27 19:50:17, Mark Mentovai wrote:
> http://codereview.chromium.org/4079/diff/439/605
> File base/build/base.vcproj (right):
> 
> http://codereview.chromium.org/4079/diff/439/605#newcode565
> Line 565: RelativePath=".\rand_util_posix.cc"
> This is the vcproj, which is only used for the Windows build.  I don't think
we
> want to build rand_util_posix on Windows.
> 
> http://codereview.chromium.org/4079/diff/439/606
> File base/rand_util.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/606#newcode15
> Line 15: return static_cast<float>(base::RandInt(0, int_max - 1)) / int_max;
> This is the naïve implementation, which may be OK for our purposes, but
carries
> minimal precision through from the generator to the floating-point number.  If
> you're interested, take a look at an implementation of drand48, which carries
a
> full 48 bits of precision.  The idea is that RandFloat() * n should carry
> maximum precision to a range of [0, n).  Also see
> http://c-faq.com/lib/rand48.html.
> 
> Why are we using float and not double here?
> 
> http://codereview.chromium.org/4079/diff/439/608
> File base/rand_util.h (right):
> 
> http://codereview.chromium.org/4079/diff/439/608#newcode1
> Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
> Nit: just 2008 on new files.  Same comment in other newly-added files.
> 
> http://codereview.chromium.org/4079/diff/439/609
> File base/rand_util_posix.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/609#newcode13
> Line 13: int RandInt(int min, int max) {
> Note: both the POSIX and Windows implementations here do not have uniform
> distribution for certain weird and large (relative to UINT_MAX) but valid
values
> of (max - min).  As an example, assume a simple RNG with a range [0, 10), and
a
> call to RandInt(0, 8).  You'll get uniform distribution of 0s and 1s, and
> uniform distribution of 2s through 7s, but you'll have more 1s than 2s.
> 
> http://codereview.chromium.org/4079/diff/439/609#newcode17
> Line 17: FILE* urandom = fopen("/dev/urandom", "r");
> I don't know that I'd bring in stdio to do this simple read instead of just
> using raw open/read/close directly.
> 
> http://codereview.chromium.org/4079/diff/439/609#newcode23
> Line 23: return min + number % (max - min + 1);
> This will crash in exactly one very important case:
> 
> RandInt(INT_MIN, INT_MAX)
> 
> INT_MAX - INT_MIN + 1 = 0 :(
> 
> Same goes for the Windows version.
> 
> http://codereview.chromium.org/4079/diff/439/610
> File base/rand_util_win.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/610#newcode20
> Line 20: return min + number % (max - min + 1);
> The comments (except for file I/O) from the POSIX implementation also apply
> here.
> 
> http://codereview.chromium.org/4079/diff/439/604
> File base/sys_info.h (right):
> 
> http://codereview.chromium.org/4079/diff/439/604#newcode31
> Line 31: // Return true if the given environment variable is defined.
> We want to eventually find a better home for the environment variable stuff,
> right?  If so, add a TODO.  If not, ignore this comment.  (I just thought I
> remembered seeing some talk between you and Dean on this point.)
> 
> http://codereview.chromium.org/4079/diff/439/604#newcode34
> Line 34: // Return value of given environment variable or empty string if not
> defined.
> Proper grammar: returns THE value of THE given environment variable or AN
empty
> string
> 
> http://codereview.chromium.org/4079/diff/439/601
> File base/sys_info_posix.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/601#newcode85
> Line 85: bool SysInfo::HasEnvironmentVariable(const char* var) {
> The .h file puts this before GetEnvironmentVariable, since everything else
> follows the order in the header, these guys should match.
> 
> http://codereview.chromium.org/4079/diff/439/603
> File base/sys_info_win.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/603#newcode56
> Line 56: scoped_array<wchar_t> value(new wchar_t[value_length *
> sizeof(*value)]);
> what's sizeof(*value) doing there?
> 
> value_length doesn't include a terminating NUL character per the Windows
> documentation.  Either add space for it, or pass value_length as an argument
to
> the wstring constructor below, or both.
> 
> http://codereview.chromium.org/4079/diff/439/603#newcode58
> Line 58: value, value_length);
> That doesn't seem right.  This should be value.get(), right?  Or &value[0].
> 
> (I'd prefer a std::vector<wchar_t> anyway, but I know Dean told you to do
> scoped_array, so that's fine...)
> 
> http://codereview.chromium.org/4079/diff/439/603#newcode71
> Line 71: return "Windows";
> Again, I'd prefer "Windows NT" so that OperatingSystemVersion at least
> corresponds to this.
> 
> http://codereview.chromium.org/4079/diff/439/617
> File chrome/browser/importer/importer_unittest.cc (left):
> 
> http://codereview.chromium.org/4079/diff/439/617#oldcode277
> Line 277: // Skips in Win2000 for the running environment can not be set up.
> Nice, thanks!
> 
> http://codereview.chromium.org/4079/diff/439/615
> File chrome/browser/metrics_log.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/615#newcode473
> Line 473: base::SysInfo::GetOperatingSystemVersion());
> Should be OperatingSystemVersion.
> 
> http://codereview.chromium.org/4079/diff/439/627
> File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/627#newcode19
> Line 19: return static_cast<uint32>(base::RandInt(0, kint32max));
> Don't we want kuint32max here?
> 
> http://codereview.chromium.org/4079/diff/439/614
> File chrome/browser/safe_browsing/protocol_manager.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/614#newcode279
> Line 279: next_update_sec_ = base::RandInt(15, 45) * 60;
> 15 to 45 minutes.  Is that right?  15 * 60, 45 * 60 might be clearer then.
> 
> http://codereview.chromium.org/4079/diff/439/623
> File chrome/common/process_watcher.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/623#newcode52
> Line 52: // If running the distributed tests, give the renderer a little time
to
> figure out
> Fix this to be less than 80 characters since you're working right here.
> 
> http://codereview.chromium.org/4079/diff/439/612
> File chrome/test/automated_ui_tests/automated_ui_tests.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/612#newcode708
> Line 708: - 1);
> The - 1 can fit on the preceding line now?  If not, at least indent it by an
> extra 4 spaces so that it's clear it's attached to arraysize and is not an
> argument of its own.

[cpu_(ooo_6.6-7.5), 2008-09-29 00:21:36]

One thing, in windows we rely on the rand to be crypto strong, the reason is
that it is used as the pipe name, this prevents a compromised renderer to to
guess the pipe name for a plugin.

There is a long buganizer bug about this.

On 2008/09/28 13:31:51, deanm wrote:
> Oh man, Mark pointed out a really good issue on the distribution.  I also
agree
> with making in double.  Sorry this has gotten so complicated.
> 
> Why don't we read 8 bytes from /dev/urandom into a uint64, then divide it into
a
> double, or something?  We can then just call rand_s twice to get 64 bits on
> Windows.  This should give use a much smaller "step".  Then, we use
> RandomDouble() to implement the min / max.
> 
> Mark, thoughts?
> 
> On 2008/09/27 19:50:17, Mark Mentovai wrote:
> > http://codereview.chromium.org/4079/diff/439/605
> > File base/build/base.vcproj (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/605#newcode565
> > Line 565: RelativePath=".\rand_util_posix.cc"
> > This is the vcproj, which is only used for the Windows build.  I don't think
> we
> > want to build rand_util_posix on Windows.
> > 
> > http://codereview.chromium.org/4079/diff/439/606
> > File base/rand_util.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/606#newcode15
> > Line 15: return static_cast<float>(base::RandInt(0, int_max - 1)) / int_max;
> > This is the naïve implementation, which may be OK for our purposes, but
> carries
> > minimal precision through from the generator to the floating-point number. 
If
> > you're interested, take a look at an implementation of drand48, which
carries
> a
> > full 48 bits of precision.  The idea is that RandFloat() * n should carry
> > maximum precision to a range of [0, n).  Also see
> > http://c-faq.com/lib/rand48.html.
> > 
> > Why are we using float and not double here?
> > 
> > http://codereview.chromium.org/4079/diff/439/608
> > File base/rand_util.h (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/608#newcode1
> > Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
> > Nit: just 2008 on new files.  Same comment in other newly-added files.
> > 
> > http://codereview.chromium.org/4079/diff/439/609
> > File base/rand_util_posix.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/609#newcode13
> > Line 13: int RandInt(int min, int max) {
> > Note: both the POSIX and Windows implementations here do not have uniform
> > distribution for certain weird and large (relative to UINT_MAX) but valid
> values
> > of (max - min).  As an example, assume a simple RNG with a range [0, 10),
and
> a
> > call to RandInt(0, 8).  You'll get uniform distribution of 0s and 1s, and
> > uniform distribution of 2s through 7s, but you'll have more 1s than 2s.
> > 
> > http://codereview.chromium.org/4079/diff/439/609#newcode17
> > Line 17: FILE* urandom = fopen("/dev/urandom", "r");
> > I don't know that I'd bring in stdio to do this simple read instead of just
> > using raw open/read/close directly.
> > 
> > http://codereview.chromium.org/4079/diff/439/609#newcode23
> > Line 23: return min + number % (max - min + 1);
> > This will crash in exactly one very important case:
> > 
> > RandInt(INT_MIN, INT_MAX)
> > 
> > INT_MAX - INT_MIN + 1 = 0 :(
> > 
> > Same goes for the Windows version.
> > 
> > http://codereview.chromium.org/4079/diff/439/610
> > File base/rand_util_win.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/610#newcode20
> > Line 20: return min + number % (max - min + 1);
> > The comments (except for file I/O) from the POSIX implementation also apply
> > here.
> > 
> > http://codereview.chromium.org/4079/diff/439/604
> > File base/sys_info.h (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/604#newcode31
> > Line 31: // Return true if the given environment variable is defined.
> > We want to eventually find a better home for the environment variable stuff,
> > right?  If so, add a TODO.  If not, ignore this comment.  (I just thought I
> > remembered seeing some talk between you and Dean on this point.)
> > 
> > http://codereview.chromium.org/4079/diff/439/604#newcode34
> > Line 34: // Return value of given environment variable or empty string if
not
> > defined.
> > Proper grammar: returns THE value of THE given environment variable or AN
> empty
> > string
> > 
> > http://codereview.chromium.org/4079/diff/439/601
> > File base/sys_info_posix.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/601#newcode85
> > Line 85: bool SysInfo::HasEnvironmentVariable(const char* var) {
> > The .h file puts this before GetEnvironmentVariable, since everything else
> > follows the order in the header, these guys should match.
> > 
> > http://codereview.chromium.org/4079/diff/439/603
> > File base/sys_info_win.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/603#newcode56
> > Line 56: scoped_array<wchar_t> value(new wchar_t[value_length *
> > sizeof(*value)]);
> > what's sizeof(*value) doing there?
> > 
> > value_length doesn't include a terminating NUL character per the Windows
> > documentation.  Either add space for it, or pass value_length as an argument
> to
> > the wstring constructor below, or both.
> > 
> > http://codereview.chromium.org/4079/diff/439/603#newcode58
> > Line 58: value, value_length);
> > That doesn't seem right.  This should be value.get(), right?  Or &value[0].
> > 
> > (I'd prefer a std::vector<wchar_t> anyway, but I know Dean told you to do
> > scoped_array, so that's fine...)
> > 
> > http://codereview.chromium.org/4079/diff/439/603#newcode71
> > Line 71: return "Windows";
> > Again, I'd prefer "Windows NT" so that OperatingSystemVersion at least
> > corresponds to this.
> > 
> > http://codereview.chromium.org/4079/diff/439/617
> > File chrome/browser/importer/importer_unittest.cc (left):
> > 
> > http://codereview.chromium.org/4079/diff/439/617#oldcode277
> > Line 277: // Skips in Win2000 for the running environment can not be set up.
> > Nice, thanks!
> > 
> > http://codereview.chromium.org/4079/diff/439/615
> > File chrome/browser/metrics_log.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/615#newcode473
> > Line 473: base::SysInfo::GetOperatingSystemVersion());
> > Should be OperatingSystemVersion.
> > 
> > http://codereview.chromium.org/4079/diff/439/627
> > File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/627#newcode19
> > Line 19: return static_cast<uint32>(base::RandInt(0, kint32max));
> > Don't we want kuint32max here?
> > 
> > http://codereview.chromium.org/4079/diff/439/614
> > File chrome/browser/safe_browsing/protocol_manager.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/614#newcode279
> > Line 279: next_update_sec_ = base::RandInt(15, 45) * 60;
> > 15 to 45 minutes.  Is that right?  15 * 60, 45 * 60 might be clearer then.
> > 
> > http://codereview.chromium.org/4079/diff/439/623
> > File chrome/common/process_watcher.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/623#newcode52
> > Line 52: // If running the distributed tests, give the renderer a little
time
> to
> > figure out
> > Fix this to be less than 80 characters since you're working right here.
> > 
> > http://codereview.chromium.org/4079/diff/439/612
> > File chrome/test/automated_ui_tests/automated_ui_tests.cc (right):
> > 
> > http://codereview.chromium.org/4079/diff/439/612#newcode708
> > Line 708: - 1);
> > The - 1 can fit on the preceding line now?  If not, at least indent it by an
> > extra 4 spaces so that it's clear it's attached to arraysize and is not an
> > argument of its own.

[please use my chromium address, 2008-09-29 12:01:36]

On 2008/09/27 19:50:17, Mark Mentovai wrote:
> http://codereview.chromium.org/4079/diff/439/605
> File base/build/base.vcproj (right):
> 
> http://codereview.chromium.org/4079/diff/439/605#newcode565
> Line 565: RelativePath=".\rand_util_posix.cc"
> This is the vcproj, which is only used for the Windows build.  I don't think
we
> want to build rand_util_posix on Windows.

Sorry, my stupid mistake.

> http://codereview.chromium.org/4079/diff/439/606
> File base/rand_util.cc (right):
> 
> http://codereview.chromium.org/4079/diff/439/606#newcode15
> Line 15: return static_cast<float>(base::RandInt(0, int_max - 1)) / int_max;
> This is the naïve implementation, which may be OK for our purposes, but
carries
> minimal precision through from the generator to the floating-point number.  If
> you're interested, take a look at an implementation of drand48, which carries
a
> full 48 bits of precision.  The idea is that RandFloat() * n should carry
> maximum precision to a range of [0, n).  Also see
> http://c-faq.com/lib/rand48.html.

Now it should be better. This implementation is based on NetBSD's libc's
drand48.

> Why are we using float and not double here?

Oh, no particular reason. Probably only because RandFloat was suggested as a
name.

> http://codereview.chromium.org/4079/diff/439/609#newcode13
> Line 13: int RandInt(int min, int max) {
> Note: both the POSIX and Windows implementations here do not have uniform
> distribution for certain weird and large (relative to UINT_MAX) but valid
values
> of (max - min).  As an example, assume a simple RNG with a range [0, 10), and
a
> call to RandInt(0, 8).  You'll get uniform distribution of 0s and 1s, and
> uniform distribution of 2s through 7s, but you'll have more 1s than 2s.

Please verify whether this still applies and if so, what should be done about
it.

> http://codereview.chromium.org/4079/diff/439/609#newcode17
> Line 17: FILE* urandom = fopen("/dev/urandom", "r");
> I don't know that I'd bring in stdio to do this simple read instead of just
> using raw open/read/close directly.

Fixed, especially after further concerns about stdio.

> http://codereview.chromium.org/4079/diff/439/609#newcode23
> Line 23: return min + number % (max - min + 1);
> This will crash in exactly one very important case:
> 
> RandInt(INT_MIN, INT_MAX)
> 
> INT_MAX - INT_MIN + 1 = 0 :(
> 
> Same goes for the Windows version.

Oops. Now it's done differently and should be ok.

> http://codereview.chromium.org/4079/diff/439/604#newcode31
> Line 31: // Return true if the given environment variable is defined.
> We want to eventually find a better home for the environment variable stuff,
> right?  If so, add a TODO.  If not, ignore this comment.  (I just thought I
> remembered seeing some talk between you and Dean on this point.)

You're right. I added two TODOs.

> http://codereview.chromium.org/4079/diff/439/603#newcode56
> Line 56: scoped_array<wchar_t> value(new wchar_t[value_length *
> sizeof(*value)]);
> what's sizeof(*value) doing there?

Another stupid mistake. Fixed. I should be more careful...

> value_length doesn't include a terminating NUL character per the Windows
> documentation.  Either add space for it, or pass value_length as an argument
to
> the wstring constructor below, or both.

Please verify that my method of doing this is valid.
 
> http://codereview.chromium.org/4079/diff/439/612#newcode708
> Line 708: - 1);
> The - 1 can fit on the preceding line now?  If not, at least indent it by an
> extra 4 spaces so that it's clear it's attached to arraysize and is not an
> argument of its own.

It didn't fit (81 chars).

Mark: you left a few comments at the beginning, for which I didn't know what to.
Please check if I should do something more about them. I will copy them here for
easier reference.

http://codereview.chromium.org/4079/diff/1/5
File base/sys_info.h (right):

http://codereview.chromium.org/4079/diff/1/5#newcode41
Line 41: static std::string OperatingSystemVersion();
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Maybe we should define that this is just major.minor without any patch
> level/service pack/whatever else information included.

I don't know how best to solve it. On Linux this would be kernel version number.
The meaning depends on operating system name anyway.

http://codereview.chromium.org/4079/diff/1/5#newcode43
Line 43: // Returns the CPU architecture of the system.
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Is it important to define what strings we use for what architectures, for
> compatibility between platforms?

(leaving quote here so that this question doesn't get lost)

http://codereview.chromium.org/4079/diff/1/2#newcode91
Line 91: return "Linux";
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> These names obviously aren't being localized here.  Where are we going to use
> these?

(quoting to prevent the question getting lost). Note: with uname it won't be
localized either. And it will be almost certainly harder to localize. The only
usage I found was browser/metrics_log.cc (for OperatingSystemName).

http://codereview.chromium.org/4079/diff/1/8
File base/sys_info_unittest.cc (right):

http://codereview.chromium.org/4079/diff/1/8#newcode28
Line 28: // Every setup should have non-empty PATH...
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Eh, I don't know how I feel about this...

Ok, this is just a crude sanity test... Should work, but probably doesn't help
much either. Remove it?

http://codereview.chromium.org/4079/diff/1/4
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/1/4#newcode70
Line 70: return "Windows";
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> If we're using OperatingSystemVersion's numbers, we might want to call this
> "Windows NT", and maybe check dwPlatformId == VER_PLATFORM_WIN32_NT.

I'm not sure how to do it. Should I get dwPlatformId using GetVersionEx? And one
more thing - it seems all Windows versions Chrome runs on are NT. Sorry for my
doubts, I'm just unfamiliar with programming on Windows.

http://codereview.chromium.org/4079/diff/1/15
File chrome/browser/views/bug_report_view.cc (right):

http://codereview.chromium.org/4079/diff/1/15#newcode333
Line 333: //   base::SysInfo::OperatingSystemVersion returns an std::string
On 2008/09/25 14:00:23, Mark Mentovai wrote:
> Because of this, maybe sys_info needs an OperatingSystemVersionComplete that
> includes patchlevel/service pack too.

Maybe I should add a TODO in SysInfo? Unfortunately I'm not familiar with
Windows programming.

[Mark Mentovai, 2008-09-29 14:13:09]

http://codereview.chromium.org/4079/diff/476/836
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/476/836#newcode23
Line 23: return min + base::RandDouble() * (max - min);
Implementing RandInt in terms of RandDouble doesn't gain anything over using
RandInt64 directly, except for the sucky conversion from integer to
floating-point domain, and the sucky conversion back.  See my previous e-mail.

Furthermore, the uniformity-of-distribution problem is actually worse this way
than if you were to use int64 directly, because you're mapping from 2^48 (as
written) or 2^52 (as suggested below) distinct values to 2^32, instead of 2^64
to 2^32.

http://codereview.chromium.org/4079/diff/476/836#newcode30
Line 30: return ldexp(number.split[0], -48) +
Good, that'll do the trick.  (See, I told you this would be fun!)

Let's take this one step further: what we have here gives 48 bits of precision,
because it's based on drand48, which is specified to give exactly 48 bits of
precision.  We have no such constraint, though.  An IEEE 754 double carries 52
bits of precision in its mantissa.  Let's use all of them, since RandInt64 is
giving us 64 bits.  I'd mix in each of the components of split, with one being
ANDed with 0xf and then raised to 2^-52.  Toss in a comment saying that we do
this for maximum precision given IEEE 754 doubles, and maybe for paranoia, toss
in a DCHECK that the returned double >= 0.0 and < 1.0.

http://codereview.chromium.org/4079/diff/476/838
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/476/838#newcode10
Line 10: // These functions are not guaranteed to be cryptographically secure.
Maybe they are now (see Carlos' message); the way we're using RandInt64 now,
there's got to be at least some kind of guarantee about its behavior.

http://codereview.chromium.org/4079/diff/476/840
File base/rand_util_win.cc (right):

http://codereview.chromium.org/4079/diff/476/840#newcode17
Line 17: return reinterpret_cast<int32>(number);
Don't know why we're hopping back and forth between signed and unsigned, I think
it'd be OK to implement RandInt64 (and 32 here) as returning uint64 in the range
[0, kuint64max] if it makes things easier.  For this, signed and unsigned are
equally valid, I think.

http://codereview.chromium.org/4079/diff/476/840#newcode20
Line 20: }
}  // namespace

http://codereview.chromium.org/4079/diff/476/833
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/476/833#newcode62
Line 62: value_length++;  // Allocate space for terminating NUL.
Ugh, now the docs on this have me confused:

If the function succeeds, the return value is the number of characters stored in
the buffer pointed to by lpBuffer, not including the terminating null character.

If lpBuffer is not large enough to hold the data, the return value is the buffer
size, in characters, required to hold the string and its terminating null
character.

You should have someone on Windows check this for you, the docs seem to indicate
that you shouldn't increment value_length here, but you should expect
value_length == real_value_length + 1 below.

http://codereview.chromium.org/4079/diff/476/833#newcode72
Line 72: return "Windows NT";
Thanks.  FWIW, the reason I keep suggesting "Windows NT" is that when taken with
the numbers returned by OperatingSystemVersion, this only makes sense as Windows
NT.  There's no such thing as Windows 5.2, but there is Windows NT 5.2 (marketed
as XP and Server 2003).

[please use my chromium address, 2008-09-29 16:47:41]

On 2008/09/29 14:13:09, Mark Mentovai wrote:
> http://codereview.chromium.org/4079/diff/476/836
> File base/rand_util.cc (right):
> 
> http://codereview.chromium.org/4079/diff/476/836#newcode23
> Line 23: return min + base::RandDouble() * (max - min);
> Implementing RandInt in terms of RandDouble doesn't gain anything over using
> RandInt64 directly, except for the sucky conversion from integer to
> floating-point domain, and the sucky conversion back.  See my previous e-mail.
> 
> Furthermore, the uniformity-of-distribution problem is actually worse this way
> than if you were to use int64 directly, because you're mapping from 2^48 (as
> written) or 2^52 (as suggested below) distinct values to 2^32, instead of 2^64
> to 2^32.

Now it should be right.

I thought about a unit test that would run RandInt(int_min, int_max) many times
(say 10^7). It would break when returned value != int_min and != int_max (so in
average case it should return quite quickly). In testing I discovered a bug that
would manifest this way. It required additional cast to fix it. Now the only
problem with this proposed test is that it's not deterministic. False-negative
is not possible, but there is an extremely small chance of a false-positive.
What do you think?

> http://codereview.chromium.org/4079/diff/476/836#newcode30
> Line 30: return ldexp(number.split[0], -48) +
> Good, that'll do the trick.  (See, I told you this would be fun!)
> 
> Let's take this one step further: what we have here gives 48 bits of
precision,
> because it's based on drand48, which is specified to give exactly 48 bits of
> precision.  We have no such constraint, though.  An IEEE 754 double carries 52
> bits of precision in its mantissa.  Let's use all of them, since RandInt64 is
> giving us 64 bits.  I'd mix in each of the components of split, with one being
> ANDed with 0xf and then raised to 2^-52.  Toss in a comment saying that we do
> this for maximum precision given IEEE 754 doubles, and maybe for paranoia,
toss
> in a DCHECK that the returned double >= 0.0 and < 1.0.

Seems to work in a "smoke test" scenario. DCHECK is in place.

> http://codereview.chromium.org/4079/diff/476/838
> File base/rand_util.h (right):
> 
> http://codereview.chromium.org/4079/diff/476/838#newcode10
> Line 10: // These functions are not guaranteed to be cryptographically secure.
> Maybe they are now (see Carlos' message); the way we're using RandInt64 now,
> there's got to be at least some kind of guarantee about its behavior.

How should I handle it in the comments?

> http://codereview.chromium.org/4079/diff/476/840
> File base/rand_util_win.cc (right):
> 
> http://codereview.chromium.org/4079/diff/476/840#newcode17
> Line 17: return reinterpret_cast<int32>(number);
> Don't know why we're hopping back and forth between signed and unsigned, I
think
> it'd be OK to implement RandInt64 (and 32 here) as returning uint64 in the
range
> [0, kuint64max] if it makes things easier.  For this, signed and unsigned are
> equally valid, I think.

Yes, that makes things just easier. Thanks.

> http://codereview.chromium.org/4079/diff/476/833
> File base/sys_info_win.cc (right):
> 
> http://codereview.chromium.org/4079/diff/476/833#newcode62
> Line 62: value_length++;  // Allocate space for terminating NUL.
> Ugh, now the docs on this have me confused:
> 
> If the function succeeds, the return value is the number of characters stored
in
> the buffer pointed to by lpBuffer, not including the terminating null
character.
> 
> If lpBuffer is not large enough to hold the data, the return value is the
buffer
> size, in characters, required to hold the string and its terminating null
> character.
> 
> You should have someone on Windows check this for you, the docs seem to
indicate
> that you shouldn't increment value_length here, but you should expect
> value_length == real_value_length + 1 below.

First I thought about including an explanatory comment. But then it turned out
to be too long, and I decided to drop the check. What do you think? It looks
cleaner now, and failing check would mean a bug in some Windows library, or an
environment variable that just changed its length. We may actually want to
handle somehow the latter - what do you think?

[please use my chromium address, 2008-09-29 16:53:35]

Added rand_util to the Windows build. Sorry for some noise it generated. :-/

[Mark Mentovai, 2008-09-29 18:08:18]

I think this looks good with the one major bit here addressed.  Let's freeze
this and just fix that, so we can get it in (see below).

http://codereview.chromium.org/4079/diff/574/1329
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/574/1329#newcode27
Line 27: // to test with min=kint32min and max=kint32max.
Should specify INT_MIN and INT_MAX as the types are int, not int32.  But...

http://codereview.chromium.org/4079/diff/574/1329#newcode30
Line 30: (static_cast<int64>(max) - min + 1.0) * (number / (kuint64max + 1.0)));
What's with the floating-point arithmetic here?

If we're not going to worry about the uniformity because it's not a big deal
when mapping from 2^64 values to at most 2^32, I don't see why integer mod won't
work.  (Unless you had something else in mind?)  I don't think the RNGs we're
using are any less random in the low bits than in the high, so we should be fine
with just a single integer divide (mod).

int64 range = static_cast<int64>(max) - min + 1;
int result = min + number % range;
DCHECK(result >= min && result <= max);
return result;

http://codereview.chromium.org/4079/diff/574/1334
File base/rand_util_unittest.cc (right):

http://codereview.chromium.org/4079/diff/574/1334#newcode11
Line 11: const int int_min = std::numeric_limits<int>::min();
static or anonymous namespace.

The name should be kIntMin per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...
.

http://codereview.chromium.org/4079/diff/574/1334#newcode14
Line 14: TEST(RandUtilTest, SameMinAndMax) {
I guess this is the test you asked about in your e-mail.

I'd be OK with the following, and maybe more, but this is what comes to mind
initially:

- A test that runs a smallish (100 - 1000 iterations) loop to verify that
nothing returned by RandInt lies outside of a passed-in [min, max] range.  Same
for RandDouble and [0, 1).
- Extending the above to ensure that it returns more than 1 (or some other
conservative number) unique value.  Sure, it's possible for the RNG to output 9,
9, 9, 9, ..., but it's unlikely; it's far more likely that such a sequence would
be the result of bad arithmetic.
- Running the test you propose, around a similarly-sized loop, to verify that
the RNG produces values other than min and max.  You can break on better input,
or if we have the second test above, you can just integrate it into that test
and do something like set a flag when we see something that's not min and not
max, and then EXPECT_TRUE(saw_not_min_and_not_max); or whatever.

Another thing to consider would be replacing the system RNG with a mock RNG for
at least some testing purposes.  That would let us test the math without
depending on real random data.

But writing tests to exercise the RNG is a pain for the reasons you point out. 
I suggest leaving this alone right now and not adding anything new to this
change.  Let's fix up the couple of remaining bits, get it tested on Windows,
and commit it.  I'd encourage you to extend the tests as you see fit after this
is checked in.  As it is now, this change has grown into something of a monster,
and it's not doing anyone any good sitting in the code review tool.

[please use my chromium address, 2008-09-29 19:05:19]

On 2008/09/29 18:08:18, Mark Mentovai wrote:
> http://codereview.chromium.org/4079/diff/574/1329#newcode30
> Line 30: (static_cast<int64>(max) - min + 1.0) * (number / (kuint64max +
1.0)));
> What's with the floating-point arithmetic here?
> 
> If we're not going to worry about the uniformity because it's not a big deal
> when mapping from 2^64 values to at most 2^32, I don't see why integer mod
won't
> work.  (Unless you had something else in mind?)  I don't think the RNGs we're
> using are any less random in the low bits than in the high, so we should be
fine
> with just a single integer divide (mod).
> 
> int64 range = static_cast<int64>(max) - min + 1;
> int result = min + number % range;
> DCHECK(result >= min && result <= max);
> return result;

I just wanted to be sure... but yeah, since we're using int64 here, that is a
lot simpler. Thanks.

> http://codereview.chromium.org/4079/diff/574/1334#newcode14
> Line 14: TEST(RandUtilTest, SameMinAndMax) {
> I guess this is the test you asked about in your e-mail.
> 
> I'd be OK with the following, and maybe more

Thanks. Had to upload second time because at first it gave me 502 Gateway Error.
:-/

[Mark Mentovai, 2008-09-29 22:20:05]

LGTM with additional changes noted below for the Windows build.  Committed at
r2697.  Please look over the changes I made and make sure you're OK with them.

This was a biggie, I think we'd all be better served in the future if things
were broken up a little bit more if possible.  I know that sometimes that's not
possible, but in this case, at least the random parts and the environment
variable parts could have been broken up into different changes.

Thanks for the patch.

http://codereview.chromium.org/4079/diff/701/703
File base/SConscript (right):

http://codereview.chromium.org/4079/diff/701/703#newcode53
Line 53: 'rand_util.cc',
Added the files to base/base.xcodeproj too.

http://codereview.chromium.org/4079/diff/701/706
File base/build/base.vcproj (right):

http://codereview.chromium.org/4079/diff/701/706#newcode561
Line 561: RelativePath=".\rand_util.cc"
These need to be ..\whatever.

http://codereview.chromium.org/4079/diff/701/706#newcode565
Line 565: RelativePath=".\rand_util_win.cc"
Should follow rand_util.h.  (The VS IDE sorts these automatically anyway, so
doing it the way it wants from the beginning prevents it from moving things
around unnecessarily.)

http://codereview.chromium.org/4079/diff/701/708
File base/rand_util.cc (right):

http://codereview.chromium.org/4079/diff/701/708#newcode28
Line 28: int result = min + number % range;
This needed to be static_cast<int>(number % range) to placate MSVC, which warned
about the loss of precision.  This is safe, because range can never be >
UINT_MAX + 1, so number % range can never be > UINT_MAX.

http://codereview.chromium.org/4079/diff/701/708#newcode39
Line 39: double result = ldexp(number.split[0] & 0xf, -52) +
MSVC is strict about these too, because its C library overloads ldexp to accept
double, long double, and float arguments instead of requiring the caller to
choose between ldexp, ldexpl, and ldexpf.  (Actually, it provides the distinct
names too, but also makes the overloads available.)

Added static_cast<double> around the first parameter in each call here.

http://codereview.chromium.org/4079/diff/701/710
File base/rand_util.h (right):

http://codereview.chromium.org/4079/diff/701/710#newcode24
Line 24: 
Removed trailing blank line.

http://codereview.chromium.org/4079/diff/701/712
File base/rand_util_win.cc (right):

http://codereview.chromium.org/4079/diff/701/712#newcode14
Line 14: uint32 RandInt32() {
Changed this to RandUInt32 for congruity with RandUInt64.

http://codereview.chromium.org/4079/diff/701/709
File base/sys_info_unittest.cc (right):

http://codereview.chromium.org/4079/diff/701/709#newcode9
Line 9: TEST(SysInfoTest, NumProcs) {
I moved these to TEST_F in r2603, I've updated the two new TESTs below to TEST_F
as well.

r2603 was from September 25, in the future, it might be a good idea to sync your
tree periodically to ensure that your changes still work with the ever-changing
state of the world.

http://codereview.chromium.org/4079/diff/701/704
File base/sys_info_win.cc (right):

http://codereview.chromium.org/4079/diff/701/704#newcode56
Line 56: std::wstring SysInfo::GetEnvironmentVariable(const char* var) {
There's a problem here.  On Windows, the platform SDK (<windows.h>) does:

#define GetEnvironmentVariable GetEnvironmentVariableW

Since we only have <windows.h> here but not in the header, this code winds up
trying to implement SysInfo::GetEnvironmentVariableW, although it was declared
without the W.

A secondary problem is that you call GetEnvironmentVariable (the SDK function)
here without qualifying it, so it looks like a call to
SysInfo::GetEnvironmentVariable.

I've renamed GetEnvironmentVariable to GetEnvVar and HasEnvironmentVariable to
HasEnvVar (for congruity) in the interest of checking this in.  I've also
renamed related things like comments and test names.  We can fix it later if
people hate the new names.  (I'm not particularly fond of them myself, but I
don't want to hold this anymore, and certainly not for this.)

http://codereview.chromium.org/4079/diff/701/704#newcode64
Line 64: return std::wstring(value);
Changed this to value.get().

http://codereview.chromium.org/4079/diff/701/730
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/4079/diff/701/730#newcode19
Line 19: return static_cast<uint32>(base::RandInt(0, kuint32max));
This is wrong and trips our new DCHECK(min <= max) in RandInt, because RandInt
operates on integers, not unsigned integers.  I changed this to
base::RandInt(INT_MIN, INT_MAX) and ensured those macros are available by
#including <limits.h>.

http://codereview.chromium.org/4079/diff/701/717
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/4079/diff/701/717#newcode25
Line 25: static const int kSbTimerStartIntervalSec = 300;
Changed this to 5 * 60 for improved awesomeness.

http://codereview.chromium.org/4079/diff/701/717#newcode72
Line 72: back_off_fuzz_ = base::RandDouble();
MSVC still requires static_cast<float> here, "possible loss of precision."  We
may actually want to make back_off_fuzz_ a double, I don't know, I'm just trying
to do the least invasive thing possible for these final fixes.  back_off_fuzz_
doesn't actually seem to need the precision of a double, the only gain would be
getting rid of the cast because our random interface uses double.

http://codereview.chromium.org/4079/diff/701/717#newcode375
Line 375: if (base::SysInfo::HasEnvironmentVariable(env_vars::kHeadless))
env_vars::kHeadless is a const wchar_t*, but HasEnvVar accepts a const char*. 
The same goes for the other env_vars constants.  I had to update GetEnvVar and
HasEnvVar to use a wchar_t* parameter, update the Windows and POSIX
implementations, and update the test.  These changes were all in
base/sys_info.*.  Sucky as it is, that's how our current strings picture looks. 
This was the least invasive fix, and we were already using wstrings for output
anyway.

[please use my chromium address, 2008-09-30 06:51:31]

On 2008/09/29 22:20:05, Mark Mentovai wrote:
> LGTM with additional changes noted below for the Windows build.  Committed at
> r2697.  Please look over the changes I made and make sure you're OK with them.

Thanks a lot. Compiles fine, passes tests, looks good.

> This was a biggie, I think we'd all be better served in the future if things
> were broken up a little bit more if possible.  I know that sometimes that's
not
> possible, but in this case, at least the random parts and the environment
> variable parts could have been broken up into different changes.

OK. Sorry for making this more difficult to review.

> http://codereview.chromium.org/4079/diff/701/709#newcode9
> Line 9: TEST(SysInfoTest, NumProcs) {
> I moved these to TEST_F in r2603, I've updated the two new TESTs below to
TEST_F
> as well.
> 
> r2603 was from September 25, in the future, it might be a good idea to sync
your
> tree periodically to ensure that your changes still work with the
ever-changing
> state of the world.

Yeah. At some point I got frustrated at build errors after syncing so I reverted
to a known-good revision... I could probably re-sync later. I will try more
often next time.

> http://codereview.chromium.org/4079/diff/701/717#newcode375
> Line 375: if (base::SysInfo::HasEnvironmentVariable(env_vars::kHeadless))
> env_vars::kHeadless is a const wchar_t*, but HasEnvVar accepts a const char*. 
> The same goes for the other env_vars constants.  I had to update GetEnvVar and
> HasEnvVar to use a wchar_t* parameter, update the Windows and POSIX
> implementations, and update the test.  These changes were all in
> base/sys_info.*.  Sucky as it is, that's how our current strings picture
looks. 
> This was the least invasive fix, and we were already using wstrings for output
> anyway.

Thanks again. I suspected something will happen on Windows with this wchar
handling.