Include chrome/common/SConscript in Linux build.

Include chrome/common/SConscript in Linux build.

BUG=2410

[please use my chromium address, 2008-09-17 08:59:17]

Please check whether I got reviewers right.

[Seo Sanghyeon, 2008-09-17 09:06:31]

http://codereview.chromium.org/3097/diff/1/8
File chrome/common/libxml_utils.cc (right):

http://codereview.chromium.org/3097/diff/1/8#newcode23
Line 23: #if defined(COMPILER_MSVCC)
Typo here.

[Dean McNamee, 2008-09-17 09:21:04]

Overall looks really good, I'm very impressed.

Reviewers look good, adding maruel.  In general the reviewers you should
probably use:

deanm - Linux and Windows, European time
maruel - Windows and C++, EST / early riser
mark - Mac and C++, EST
sgk - SConscript, PST
evan - Linux, PST

http://codereview.chromium.org/3097/diff/1/7
File chrome/common/SConscript (right):

http://codereview.chromium.org/3097/diff/1/7#newcode170
Line 170: # TODO(port): Port this.
the () part should be a username or a bug number.

http://codereview.chromium.org/3097/diff/1/10
File chrome/common/debug_flags.cc (right):

http://codereview.chromium.org/3097/diff/1/10#newcode8
Line 8: #include <windows.h>
Do we actually need this header for anything?  It looks like if nothing else is
wrapped if an ifdef, we should be able to just drop the include?

http://codereview.chromium.org/3097/diff/1/13
File chrome/common/jpeg_codec.cc (right):

http://codereview.chromium.org/3097/diff/1/13#newcode517
Line 517: memcpy(bitmap->getAddr32(0, 0), &data_vector[0], data_length);
Good change, curious if this was causing a compile failure?

http://codereview.chromium.org/3097/diff/1/9
File chrome/common/json_value_serializer.cc (right):

http://codereview.chromium.org/3097/diff/1/9#newcode40
Line 40: FILE* file = fopen(WideToUTF8(json_file_path_).c_str(), "wb");
This is going to complain on Windows, because of fopen_s.  Also, I don't think
this is the right thing to do here, WideToUTF8 isn't right on Windows, I
believe...

http://codereview.chromium.org/3097/diff/1/9#newcode52
Line 52: FILE* file = fopen(WideToUTF8(json_file_path_).c_str(), "rb");
Same

http://codereview.chromium.org/3097/diff/1/8
File chrome/common/libxml_utils.cc (right):

http://codereview.chromium.org/3097/diff/1/8#newcode25
Line 25: #endif
Use ALLOW_THIS_IN_INITIALIZER_LIST ?

http://codereview.chromium.org/3097/diff/1/6
File chrome/common/net/cookie_monster_sqlite.cc (right):

http://codereview.chromium.org/3097/diff/1/6#newcode109
Line 109: static const PendingOperationsList::size_type kCommitAfterBatchSize =
512;
I wrote this code :\  I will use size_t, and make a commit for it now.  Thanks.

http://codereview.chromium.org/3097/diff/1/4
File chrome/common/pref_member.cc (right):

http://codereview.chromium.org/3097/diff/1/4#newcode16
Line 16: prefs_(NULL) {
I think you should move the bools to the bottom in the header, and then this can
be left the same?

http://codereview.chromium.org/3097/diff/1/5
File chrome/common/pref_member.h (right):

http://codereview.chromium.org/3097/diff/1/5#newcode58
Line 58: bool setting_value_;
I think bools should generally go at the end (and together so they're packed)

http://codereview.chromium.org/3097/diff/1/5#newcode63
Line 63: PrefService* prefs_;
I think we generally want strings first?  You'd have to ask MA, but I think it's
usually something like

strings/classes
pointers
bools

http://codereview.chromium.org/3097/diff/1/11
File chrome/common/stl_util-inl.h (right):

http://codereview.chromium.org/3097/diff/1/11#newcode442
Line 442: return std::vector<T>(values);
:)

I don't see this sort of constructor documented though, will it work on Windows?
 Maybe std::vector<T>(values.begin(), values.end()) (the input iterator
constructor) would be safer?

[Mark Mentovai, 2008-09-17 12:35:32]

Dean already gave a pretty through review, looks good otherwise.

http://codereview.chromium.org/3097/diff/1/13
File chrome/common/jpeg_codec.cc (right):

http://codereview.chromium.org/3097/diff/1/13#newcode517
Line 517: memcpy(bitmap->getAddr32(0, 0), &data_vector[0], data_length);
deanm wrote:
> Good change, curious if this was causing a compile failure?

I bet, "warning: unused variable"

http://codereview.chromium.org/3097/diff/1/9
File chrome/common/json_value_serializer.cc (right):

http://codereview.chromium.org/3097/diff/1/9#newcode40
Line 40: FILE* file = fopen(WideToUTF8(json_file_path_).c_str(), "wb");
This will have to be an #ifdef until our file and path story is straightened
out.

http://codereview.chromium.org/3097/diff/1/8
File chrome/common/libxml_utils.cc (right):

http://codereview.chromium.org/3097/diff/1/8#newcode24
Line 24: MSVC_SUPPRESS_WARNING(4355);  // Okay to pass "this" here.
You don't actually need to wrap MSVC_SUPPRESS_WARNING in an #ifdef, it's defined
as harmless on non-MSVC.

Use the macro Dean suggested, you should wrap the initializer in it:

ALLOW_THIS_IN_INITIALIZER_LIST(error_func_(this, ...))

[M-A Ruel, 2008-09-17 13:40:41]

In general, it looks good to me. Just the fopen() stuff to fix before.

http://codereview.chromium.org/3097/diff/1/10
File chrome/common/debug_flags.cc (right):

http://codereview.chromium.org/3097/diff/1/10#newcode8
Line 8: #include <windows.h>
I agree, this include is not necessary anymore.

http://codereview.chromium.org/3097/diff/1/12
File chrome/common/gfx/color_utils.cc (right):

http://codereview.chromium.org/3097/diff/1/12#newcode9
Line 9: #include <windows.h>
Same here, AFAIK, windows.h is unnecessary. Just plain remove it.

http://codereview.chromium.org/3097/diff/1/9
File chrome/common/json_value_serializer.cc (right):

http://codereview.chromium.org/3097/diff/1/9#newcode40
Line 40: FILE* file = fopen(WideToUTF8(json_file_path_).c_str(), "wb");
Remove that and use
file_util::WriteFile(json_file_path_, json_string.data(), json_string.size());

http://codereview.chromium.org/3097/diff/1/9#newcode52
Line 52: FILE* file = fopen(WideToUTF8(json_file_path_).c_str(), "rb");
Use file_util::ReadFileToString() instead or file_util::ReadFile().

http://codereview.chromium.org/3097/diff/1/5
File chrome/common/pref_member.h (right):

http://codereview.chromium.org/3097/diff/1/5#newcode63
Line 63: PrefService* prefs_;
Sure you'll get better alignment.

[please use my chromium address, 2008-09-17 14:03:16]

Please take a look.

http://codereview.chromium.org/3097/diff/1/7
File chrome/common/SConscript (right):

http://codereview.chromium.org/3097/diff/1/7#newcode170
Line 170: # TODO(port): Port this.
On 2008/09/17 09:21:05, deanm wrote:
> the () part should be a username or a bug number.

In my other recent reviews I was asked to make it TODO(port): - should it be
different here?

http://codereview.chromium.org/3097/diff/1/12
File chrome/common/gfx/color_utils.cc (right):

http://codereview.chromium.org/3097/diff/1/12#newcode9
Line 9: #include <windows.h>
On 2008/09/17 13:40:42, M-A wrote:
> Same here, AFAIK, windows.h is unnecessary. Just plain remove it.

I think here it is necessary for ::GetSysColor. Please check, unfortunately I
can't test on Windows.

http://codereview.chromium.org/3097/diff/1/13
File chrome/common/jpeg_codec.cc (right):

http://codereview.chromium.org/3097/diff/1/13#newcode517
Line 517: memcpy(bitmap->getAddr32(0, 0), &data_vector[0], data_length);
On 2008/09/17 12:35:33, Mark Mentovai wrote:
> deanm wrote:
> > Good change, curious if this was causing a compile failure?
> 
> I bet, "warning: unused variable"

Exactly. There also would be a duplication issue here, so now there are two
problems fixed in one change.

http://codereview.chromium.org/3097/diff/1/5
File chrome/common/pref_member.h (right):

http://codereview.chromium.org/3097/diff/1/5#newcode63
Line 63: PrefService* prefs_;
On 2008/09/17 09:21:05, deanm wrote:
> I think we generally want strings first?  You'd have to ask MA, but I think
it's
> usually something like
> 
> strings/classes
> pointers
> bools

Hopefully I've done it right, but I'm not sure.

http://codereview.chromium.org/3097/diff/1/11
File chrome/common/stl_util-inl.h (right):

http://codereview.chromium.org/3097/diff/1/11#newcode442
Line 442: return std::vector<T>(values);
On 2008/09/17 09:21:05, deanm wrote:
> :)
> 
> I don't see this sort of constructor documented though, will it work on
Windows?
>  Maybe std::vector<T>(values.begin(), values.end()) (the input iterator
> constructor) would be safer?

Indeed, previous version was not documented, and your version is documented (SGI
STL docs). Corrected.

[Dean McNamee, 2008-09-17 14:12:42]

Looking at the newest changes now...

http://codereview.chromium.org/3097/diff/1/7
File chrome/common/SConscript (right):

http://codereview.chromium.org/3097/diff/1/7#newcode170
Line 170: # TODO(port): Port this.
Sorry, I didn't release we were using TODO(port).

On 2008/09/17 14:03:17, phajdan.jr wrote:
> On 2008/09/17 09:21:05, deanm wrote:
> > the () part should be a username or a bug number.
> 
> In my other recent reviews I was asked to make it TODO(port): - should it be
> different here?

http://codereview.chromium.org/3097/diff/1/12
File chrome/common/gfx/color_utils.cc (right):

http://codereview.chromium.org/3097/diff/1/12#newcode9
Line 9: #include <windows.h>
Yep, this one should stay.

On 2008/09/17 14:03:17, phajdan.jr wrote:
> On 2008/09/17 13:40:42, M-A wrote:
> > Same here, AFAIK, windows.h is unnecessary. Just plain remove it.
> 
> I think here it is necessary for ::GetSysColor. Please check, unfortunately I
> can't test on Windows.

[Dean McNamee, 2008-09-17 14:19:57]

Great work.  Punting to SGK on SCons stuff.

http://codereview.chromium.org/3097/diff/222/24
File chrome/common/libxml_utils.cc (right):

http://codereview.chromium.org/3097/diff/222/24#newcode23
Line 23: ALLOW_THIS_IN_INITIALIZER_LIST(error_func_(this,
&XmlReader::GenericErrorCallback)) {
80 chars :(

http://codereview.chromium.org/3097/diff/222/27
File chrome/common/stl_util-inl.h (left):

http://codereview.chromium.org/3097/diff/222/27#oldcode448
Line 448: return result;
Just curious, what was the problem with the older code?  I can just see one
benefit that it calls reserve() first, which might avoid some growing / copying?

[M-A Ruel, 2008-09-17 14:22:23]

LGTM with the two small nits.

http://codereview.chromium.org/3097/diff/222/24
File chrome/common/libxml_utils.cc (right):

http://codereview.chromium.org/3097/diff/222/24#newcode23
Line 23: ALLOW_THIS_IN_INITIALIZER_LIST(error_func_(this,
&XmlReader::GenericErrorCallback)) {
Sorry, line is >80 chars, you need to split it. Example:
      ALLOW_THIS_IN_INITIALIZER_LIST(
          error_func_(this, &XmlReader::GenericErrorCallback)) {

http://codereview.chromium.org/3097/diff/222/22
File chrome/common/pref_member.h (right):

http://codereview.chromium.org/3097/diff/222/22#newcode61
Line 61: protected:
Humm, and I now recall to have written this code. :/

Just add this comment below protected:
  // Ordered the members to compact the class instance.

[please use my chromium address, 2008-09-17 14:27:21]

On 2008/09/17 14:19:57, deanm wrote:
> Great work.  Punting to SGK on SCons stuff.
> 
> http://codereview.chromium.org/3097/diff/222/24
> File chrome/common/libxml_utils.cc (right):
> 
> http://codereview.chromium.org/3097/diff/222/24#newcode23
> Line 23: ALLOW_THIS_IN_INITIALIZER_LIST(error_func_(this,
> &XmlReader::GenericErrorCallback)) {
> 80 chars :(

Corrected.

> http://codereview.chromium.org/3097/diff/222/27
> File chrome/common/stl_util-inl.h (left):
> 
> http://codereview.chromium.org/3097/diff/222/27#oldcode448
> Line 448: return result;
> Just curious, what was the problem with the older code?  I can just see one
> benefit that it calls reserve() first, which might avoid some growing /
copying?

It didn't compile. GCC complained about iterator variables not being declared
(if you want I can reproduce exact error message).

On 2008/09/17 14:22:23, M-A wrote:
> http://codereview.chromium.org/3097/diff/222/22
> File chrome/common/pref_member.h (right):
> 
> http://codereview.chromium.org/3097/diff/222/22#newcode61
> Line 61: protected:
> 
> Just add this comment below protected:
>   // Ordered the members to compact the class instance.

Done.

[Dean McNamee, 2008-09-17 14:39:03]

Sorry about the nits, I will land this and handle them myself.  The "typename"
bit will be important in porting more code, we've run into it a lot.

http://codereview.chromium.org/3097/diff/69/235
File chrome/common/libxml_utils.cc (right):

http://codereview.chromium.org/3097/diff/69/235#newcode23
Line 23: ALLOW_THIS_IN_INITIALIZER_LIST(error_func_(
I would probably write this as
ALLOW_THIS_IN_INITIALIZER_LIST(
    error_func_(...

http://codereview.chromium.org/3097/diff/69/233
File chrome/common/pref_member.h (right):

http://codereview.chromium.org/3097/diff/69/233#newcode62
Line 62: // Ordered the members to compact the class instance.
I would move this above the private.

http://codereview.chromium.org/3097/diff/69/238
File chrome/common/stl_util-inl.h (left):

http://codereview.chromium.org/3097/diff/69/238#oldcode444
Line 444: std::set<T>::const_iterator end = values.end();
Ah, you simple need "typename" here since it's a templatized parameter derived
type.

typename std::set<T>::const iterator end...

http://codereview.chromium.org/3097/diff/69/238#oldcode445
Line 445: for (std::set<T>::const_iterator itr = values.begin(); itr != end;
++itr) {
same here

But I would personally write this:

std::vector<T> result;
result.reserve(values.size());
result.insert(0, values.begin(), values.end());
return result;

I don't know how smart the std::vector constructor is... Ok, I dug into the STL
code.  The iterator constructor just does an insert (begin(), first, last),
which doesn't look smart enough to reserve().  So I would go with the reserve :\

[please use my chromium address, 2008-09-17 14:39:14]

Updated to resolve indentation issue with ALLOW_THIS_IN_INITIALIZER_LIST.