Port some things in browser/{download,history}, minor things in common....

Port some things in browser/{download,history}, minor things in common.
Also enables some unit tests (Hammer/unit_tests runs 61 after applying this
patch). Added os_unix.c file missing for sqlite. Without it you can't link
things really using sqlite.

BUG=2333

[Dean McNamee, 2008-09-24 10:25:20]

A lot of this looks good.  It's a big patch though, I will probably have to
break it into bits, and review it with the right people.  I will commit the
small stuff today (signed / unsigned changes, typename, etc), and I will work on
some of the refactoring I mentioned.

Thanks again

http://codereview.chromium.org/4247/diff/1/22
File chrome/browser/download/download_exe.cc (left):

http://codereview.chromium.org/4247/diff/1/22#oldcode8
Line 8: #include "chrome/browser/download/download_util.h"
In theory this header should stay, since it's where the below stuff is declared,
like InitializeExeTypes.  But I'm guessing that all of the other stuff in the
header causes us not to build on Linux.  I think maybe it would make more sense
to actually have a download_exe.h, and have download_util.h include it, and then
we can include it here.  I will do that.

http://codereview.chromium.org/4247/diff/1/20
File chrome/browser/download/save_file.cc (right):

http://codereview.chromium.org/4247/diff/1/20#newcode98
Line 98: #if defined(OS_WIN)
Hmmm, bummer...

http://codereview.chromium.org/4247/diff/1/12
File chrome/browser/history/text_database.cc (right):

http://codereview.chromium.org/4247/diff/1/12#newcode7
Line 7: #include <wchar.h>
Really?  maybe this should be basictypes below instead?

http://codereview.chromium.org/4247/diff/1/12#newcode126
Line 126: if (swscanf(number_begin, L"%d-%d", &year, &month) != 2)
This will not build on Windows, unfortunately.  The default settings are to
deprecate the "insecure" functions, so you have to use the _s functions, even in
cases where it makes no difference like this one. :\

http://codereview.chromium.org/4247/diff/1/12#newcode352
Line 352: match.title = UTF8ToWide(statement->column_string(1));
Hmm, I'm not sure that's right?

http://codereview.chromium.org/4247/diff/1/7
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/4247/diff/1/7#newcode32
Line 32: for (uint32 i = 0; i < count; ++i) {
size_t ?

http://codereview.chromium.org/4247/diff/1/6
File chrome/browser/template_url.h (left):

http://codereview.chromium.org/4247/diff/1/6#oldcode315
Line 315: }
Not sure if this was inlined in the header for a reason.  When moving stuff to
the .cc file, we're removing opportunities for the compiler to do inlining.  I
doubt it matters here, but I guess we should verify with the author.

http://codereview.chromium.org/4247/diff/1/26
File chrome/common/env_util.h (right):

http://codereview.chromium.org/4247/diff/1/26#newcode35
Line 35: #endif
Seems like a bunch of this stuff should move into sys_info
(GetOperatingSystemName, etc), and we should probably do away with env_util,
it's a bad name and lots of this stuff doesn't fit.  We should probably have a
function to retrieve things from the environment, and then
HasEnvironmentVariable should use that, instead of just doing getenv() directly.
 Sorry, lots of refactoring when porting :\

http://codereview.chromium.org/4247/diff/1/25
File chrome/common/mru_cache.h (right):

http://codereview.chromium.org/4247/diff/1/25#newcode196
Line 196: PayloadType, MRUCacheNullDeletor<PayloadType> >::size_type max_size)
Maybe it would make sense to have a typedef MRUCacheBase< ... >
MRUCacheBaseType...

http://codereview.chromium.org/4247/diff/1/28
File chrome/common/rand_util.cc (right):

http://codereview.chromium.org/4247/diff/1/28#newcode25
Line 25: #elif defined(OS_POSIX)
This should also move into base, and probably be refactored...

http://codereview.chromium.org/4247/diff/1/3
File third_party/sqlite/SConscript (right):

http://codereview.chromium.org/4247/diff/1/3#newcode69
Line 69: 'os_unix.c',
Will that build on Windows?

[sgk, 2008-09-24 16:28:09]

More SCons lgtm goodness.

[please use my chromium address, 2008-09-25 08:32:45]

On 2008/09/24 10:25:20, deanm wrote:
> http://codereview.chromium.org/4247/diff/1/12#newcode352
> Line 352: match.title = UTF8ToWide(statement->column_string(1));
> Hmm, I'm not sure that's right?

On Linux it was required to pass unit test. Otherwise it complained about
invalid unicode characters. However, now it may break on Windows.

> http://codereview.chromium.org/4247/diff/1/3#newcode69
> Line 69: 'os_unix.c',
> Will that build on Windows?

I couldn't test, but it should. It has a big #if OS_UNIX in it, which is only
true when  building on Unix. Similarly os_win.c which is actually included in
the build has #if OS_WIN.

I will wait with fixing other things you pointed out until necessary
refactorings land. The first part is at http://codereview.chromium.org/4079 .
Then changes in file_util and string_util will follow.

[please use my chromium address, 2008-10-01 19:04:56]

Okay. Updated the patch after refactorings landed. Now some earlier comments may
still apply. Please check them, with small hints for me how to fix them.

[Mark Mentovai, 2008-10-01 20:17:58]

LGTM

http://codereview.chromium.org/4247/diff/401/619
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/4247/diff/401/619#newcode8
Line 8: #include "base/logging.h"
Didn't Dean have a comment about this?

http://codereview.chromium.org/4247/diff/401/617
File chrome/browser/download/save_file.cc (right):

http://codereview.chromium.org/4247/diff/401/617#newcode67
Line 67: // with the security that makes sense in the new path. If the last
parameter
This comment no longer applies, revise.

http://codereview.chromium.org/4247/diff/401/617#newcode102
Line 102: // TODO(port): Similarly mark on Mac.
What's this mean?

Oh, now I get it.  Can you move the comment after the OS_WIN ifdef so that
"similarly" has some context?

http://codereview.chromium.org/4247/diff/401/609
File chrome/browser/history/text_database.cc (right):

http://codereview.chromium.org/4247/diff/401/609#newcode121
Line 121: const std::wstring number_begin(
Now that it's not a pointer, why's it still called number_begin?

http://codereview.chromium.org/4247/diff/401/604
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/4247/diff/401/604#newcode27
Line 27: uint32 count = 1000;//100000;
Can you fix the comment to comply with our style guide too?

http://codereview.chromium.org/4247/diff/401/605
File chrome/browser/template_url.cc (right):

http://codereview.chromium.org/4247/diff/401/605#newcode507
Line 507: void TemplateURL::set_keyword(const std::wstring& keyword) {
I guess moving this to the .cc is OK, but I don't know why you've done it.

http://codereview.chromium.org/4247/diff/401/622
File chrome/common/mru_cache.h (right):

http://codereview.chromium.org/4247/diff/401/622#newcode196
Line 196: DISALLOW_EVIL_CONSTRUCTORS(MRUCache);
This should be the last thing in the class.  I think that still holds even if
you have to introduce an extra "private" section before the normal "public"
section.

Also, the new name for this is DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/4247/diff/401/622#newcode201
Line 201: : ParentType(max_size) {
It's four spaces for a continuation line like this.

http://codereview.chromium.org/4247/diff/401/622#newcode228
Line 228: DISALLOW_EVIL_CONSTRUCTORS(OwningMRUCache);
As before.

http://codereview.chromium.org/4247/diff/401/622#newcode233
Line 233: : ParentType(max_size) {
Me too.

[brettw, 2008-10-01 21:58:48]

History stuff looks correct to me.

[please use my chromium address, 2008-10-02 10:14:35]

http://codereview.chromium.org/4247/diff/401/619
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/4247/diff/401/619#newcode8
Line 8: #include "base/logging.h"
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> Didn't Dean have a comment about this?

OK, I made a change he wrote about.

http://codereview.chromium.org/4247/diff/401/617
File chrome/browser/download/save_file.cc (right):

http://codereview.chromium.org/4247/diff/401/617#newcode67
Line 67: // with the security that makes sense in the new path. If the last
parameter
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> This comment no longer applies, revise.

Done.

http://codereview.chromium.org/4247/diff/401/617#newcode102
Line 102: // TODO(port): Similarly mark on Mac.
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> What's this mean?
> 
> Oh, now I get it.  Can you move the comment after the OS_WIN ifdef so that
> "similarly" has some context?

Done.

http://codereview.chromium.org/4247/diff/401/609
File chrome/browser/history/text_database.cc (right):

http://codereview.chromium.org/4247/diff/401/609#newcode121
Line 121: const std::wstring number_begin(
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> Now that it's not a pointer, why's it still called number_begin?

Done. Please check whether the new name is right ("suffix").

http://codereview.chromium.org/4247/diff/401/604
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/4247/diff/401/604#newcode27
Line 27: uint32 count = 1000;//100000;
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> Can you fix the comment to comply with our style guide too?

Done. But please verify if that's what you meant.

http://codereview.chromium.org/4247/diff/401/605
File chrome/browser/template_url.cc (right):

http://codereview.chromium.org/4247/diff/401/605#newcode507
Line 507: void TemplateURL::set_keyword(const std::wstring& keyword) {
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> I guess moving this to the .cc is OK, but I don't know why you've done it.

There are some files this patch moves to Linux build which depend on
template_url.h. Now l10n_util.h is not ported, so I moved this little function
here to remove l10n_util #include from template_url.h.

http://codereview.chromium.org/4247/diff/401/622
File chrome/common/mru_cache.h (right):

http://codereview.chromium.org/4247/diff/401/622#newcode196
Line 196: DISALLOW_EVIL_CONSTRUCTORS(MRUCache);
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> This should be the last thing in the class.  I think that still holds even if
> you have to introduce an extra "private" section before the normal "public"
> section.
> 
> Also, the new name for this is DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/4247/diff/401/622#newcode201
Line 201: : ParentType(max_size) {
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> It's four spaces for a continuation line like this.

Done.

http://codereview.chromium.org/4247/diff/401/622#newcode228
Line 228: DISALLOW_EVIL_CONSTRUCTORS(OwningMRUCache);
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> As before.

Done.

http://codereview.chromium.org/4247/diff/401/622#newcode233
Line 233: : ParentType(max_size) {
On 2008/10/01 20:17:58, Mark Mentovai wrote:
> Me too.

Done.