Porting changes in chrome/{browser,common}, also enabling more unit tests....

Porting changes in chrome/{browser,common}, also enabling more unit tests.

At the moment of submission, with these changes applied,
Hammer/unit_tests runs 48 tests (all pass, of course).

BUG=2333, 2335, 2410

[Dean McNamee, 2008-09-18 19:26:14]

Wow, this is super good.  Thanks.  A bunch of nits for now, and will probably
get deeper next time around.

http://codereview.chromium.org/3149/diff/1/13
File chrome/browser/metrics_response_unittest.cc (right):

http://codereview.chromium.org/3149/diff/1/13#newcode79
Line 79: for (unsigned int i = 0; i < arraysize(response_cases); ++i) {
we generally don't use unsigned, size_t is probably right here.

http://codereview.chromium.org/3149/diff/1/13#newcode104
Line 104: for (unsigned int i = 0; i < arraysize(bogus_responses); ++i) {
same

http://codereview.chromium.org/3149/diff/1/11
File chrome/browser/printing/page_range_unittest.cc (right):

http://codereview.chromium.org/3149/diff/1/11#newcode21
Line 21: ASSERT_EQ(static_cast<size_t>(8), pages.size());
For constants, we have been using the U suffix, so 8U

http://codereview.chromium.org/3149/diff/1/11#newcode35
Line 35: EXPECT_EQ(static_cast<size_t>(0), pages.size());
Same, 0U :\

http://codereview.chromium.org/3149/diff/1/10
File chrome/browser/printing/units_unittest.cc (right):

http://codereview.chromium.org/3149/diff/1/10#newcode35
Line 35: EXPECT_EQ(static_cast<int>(2000000000UL), ConvertUnit(2, 1,
1000000000UL));
static_cast<int>(unsigned long constant) seems wrong?

http://codereview.chromium.org/3149/diff/1/5
File chrome/browser/renderer_security_policy.cc (right):

http://codereview.chromium.org/3149/diff/1/5#newcode75
Line 75: DISALLOW_EVIL_CONSTRUCTORS(SecurityState);
This isn't your fault, but we use the better named DISALLOW_COPY_AND_ASSIGN()
now, so might as well update it while you're here.

http://codereview.chromium.org/3149/diff/1/14
File chrome/browser/session_startup_pref.cc (right):

http://codereview.chromium.org/3149/diff/1/14#newcode43
Line 43: break;
Maybe instead add a 
case default:
  type = 0;
  break

?

http://codereview.chromium.org/3149/diff/1/20
File chrome/common/json_value_serializer_unittest.cc (right):

http://codereview.chromium.org/3149/diff/1/20#newcode214
Line 214: ASSERT_EQ(static_cast<size_t>(1), list->GetSize());
1U again

[please use my chromium address, 2008-09-18 19:51:59]

On 2008/09/18 19:26:14, deanm wrote:
> http://codereview.chromium.org/3149/diff/1/10
> File chrome/browser/printing/units_unittest.cc (right):
> 
> http://codereview.chromium.org/3149/diff/1/10#newcode35
> Line 35: EXPECT_EQ(static_cast<int>(2000000000UL), ConvertUnit(2, 1,
> 1000000000UL));
> static_cast<int>(unsigned long constant) seems wrong?

GCC required me to make these big numbers UL (warning about C90 standard), and
then i had (?) to do the static cast. As indicated I'm not sure whether it must
be done this way.

> http://codereview.chromium.org/3149/diff/1/5
> File chrome/browser/renderer_security_policy.cc (right):
> 
> http://codereview.chromium.org/3149/diff/1/5#newcode75
> Line 75: DISALLOW_EVIL_CONSTRUCTORS(SecurityState);
> This isn't your fault, but we use the better named DISALLOW_COPY_AND_ASSIGN()
> now, so might as well update it while you're here.

No problem.

> http://codereview.chromium.org/3149/diff/1/14
> File chrome/browser/session_startup_pref.cc (right):
> 
> http://codereview.chromium.org/3149/diff/1/14#newcode43
> Line 43: break;
> Maybe instead add a 
> case default:
>   type = 0;
>   break
> 
> ?

I'm not sure. At some point I thought about that, but it turns out that DEFAULT
(uppercase) means here "don't restore session" (see the .h file).

Fixed other issues.

[Dean McNamee, 2008-09-18 20:06:58]

Looks good with a few nits, going to have to ask MA on the printer stuff.  I
will test tomorrow, and commit w/o the printer changes if we haven't figured
that out yet.

Thanks again

http://codereview.chromium.org/3149/diff/26/233
File chrome/browser/printing/units_unittest.cc (right):

http://codereview.chromium.org/3149/diff/26/233#newcode36
Line 36: EXPECT_EQ(static_cast<int>(4000000000UL), ConvertUnit(2, 1,
2000000000UL));
Ok, I don't understand why you should have to for 2000000000, since it's <
2**31.  And the U is definitely wrong... I will add in MA on this one, he wrote
this code.

http://codereview.chromium.org/3149/diff/26/237
File chrome/browser/session_startup_pref.cc (right):

http://codereview.chromium.org/3149/diff/26/237#newcode43
Line 43: break;
Ok, maybe leave the type = 0 above, and do a case default: break;  Only because
I don't know this code, and it keeps it closer to it's original form.  We're
just trying to get it to compile, not to refactor it... It was just a warning
about a switch with no default case?

http://codereview.chromium.org/3149/diff/26/243
File chrome/common/json_value_serializer_unittest.cc (right):

http://codereview.chromium.org/3149/diff/26/243#newcode190
Line 190: ASSERT_EQ(static_cast<size_t>(1), list->GetSize());
1U

[M-A Ruel, 2008-09-18 20:14:17]

http://codereview.chromium.org/3149/diff/26/233
File chrome/browser/printing/units_unittest.cc (right):

http://codereview.chromium.org/3149/diff/26/233#newcode35
Line 35: EXPECT_EQ(static_cast<int>(2000000000UL), ConvertUnit(2, 1,
1000000000UL));
The prototype is:
int ConvertUnit(int value, int old_unit, int new_unit);
Remove these two casts.

http://codereview.chromium.org/3149/diff/26/233#newcode36
Line 36: EXPECT_EQ(static_cast<int>(4000000000UL), ConvertUnit(2, 1,
2000000000UL));
The thing here is that I use 4,000,000,000, which is a negative int when
represented on 32 bits, still equals the value returned by the function. I guess
you could use:
EXPECT_EQ(4000000000U, static_cast<unsigned int>(ConvertUnit(2, 1,
2000000000)));

[sgk, 2008-09-18 21:28:19]

SCons lgtm

http://codereview.chromium.org/3149/diff/26/231
File chrome/browser/SConscript (right):

http://codereview.chromium.org/3149/diff/26/231#newcode68
Line 68: 'google_url_tracker.cc',
nit:  alphabetical order

[please use my chromium address, 2008-09-19 09:58:15]

On 2008/09/18 20:06:58, deanm wrote:
> http://codereview.chromium.org/3149/diff/26/233
> File chrome/browser/printing/units_unittest.cc (right):
> 
> http://codereview.chromium.org/3149/diff/26/233#newcode36
> Line 36: EXPECT_EQ(static_cast<int>(4000000000UL), ConvertUnit(2, 1,
> 2000000000UL));
> Ok, I don't understand why you should have to for 2000000000, since it's <
> 2**31.  And the U is definitely wrong... I will add in MA on this one, he
wrote
> this code.

I removed the U for the right side. For the left side it's still necessary:
browser/printing/units_unittest.cc:36: error: this decimal constant is unsigned
only in ISO C90

On 2008/09/18 20:14:17, M-A wrote:
> http://codereview.chromium.org/3149/diff/26/233#newcode36
> Line 36: EXPECT_EQ(static_cast<int>(4000000000UL), ConvertUnit(2, 1,
> 2000000000UL));
> The thing here is that I use 4,000,000,000, which is a negative int when
> represented on 32 bits, still equals the value returned by the function. I
guess
> you could use:
> EXPECT_EQ(4000000000U, static_cast<unsigned int>(ConvertUnit(2, 1,
> 2000000000)));

Done.

> http://codereview.chromium.org/3149/diff/26/237
> File chrome/browser/session_startup_pref.cc (right):
> 
> http://codereview.chromium.org/3149/diff/26/237#newcode43
> Line 43: break;
> Ok, maybe leave the type = 0 above, and do a case default: break;  Only
because
> I don't know this code, and it keeps it closer to it's original form.  We're
> just trying to get it to compile, not to refactor it... It was just a warning
> about a switch with no default case?

OK. Exact message was browser/session_startup_pref.cc:32: error: enumeration
value 'DEFAULT' not handled in switch.

Fixed rest of issues too.

[please use my chromium address, 2008-09-19 12:45:36]

I forgot to add chrome/SConscript.unit_tests. Added it now.

sgk: Please take a look.

[M-A Ruel, 2008-09-19 12:51:18]

Very good, thanks a lot.

http://codereview.chromium.org/3149/diff/50/342
File chrome/browser/session_startup_pref.cc (right):

http://codereview.chromium.org/3149/diff/50/342#newcode41
Line 41: default:
add 'NOTREACHED();' here.

[Dean McNamee, 2008-09-19 12:58:10]

http://codereview.chromium.org/3149/diff/50/342
File chrome/browser/session_startup_pref.cc (right):

http://codereview.chromium.org/3149/diff/50/342#newcode41
Line 41: default:
That's a lie, it is reached.  It's the case of DEFAULT.

On 2008/09/19 12:51:18, M-A wrote:
> add 'NOTREACHED();' here.

[M-A Ruel, 2008-09-19 12:59:49]

http://codereview.chromium.org/3149/diff/50/342
File chrome/browser/session_startup_pref.cc (right):

http://codereview.chromium.org/3149/diff/50/342#newcode41
Line 41: default:
Ok, don't listen to me.

On 2008/09/19 12:58:10, deanm wrote:
> That's a lie, it is reached.  It's the case of DEFAULT.
> 
> On 2008/09/19 12:51:18, M-A wrote:
> > add 'NOTREACHED();' here.