WinDDK ATL and MSVC express compatability

WinDDK ATL and MSVC express 2008/2005 compatability 

Gives 'out of the box' compatability with VC2008 express - i.e. no modifications needed to system headers or .gyp* files or related hacks. Just set the GYP_MSVS_VERSION to 200Xe (e for express, same as the linked blog on the issue and first google hit as well). 

1) Changes to build\common.gypi to turn off to define COMPILER_MSVC_EXPRESS if a express is detected (through the GYP_MSVC_VERSION environment variable), turn off the _SECURE_ATL define (which is flagged as an error with WinDDK's ATL, _SECURE_ATL adds [more] CRT checks to other ATL versions) and hard to link to the atl stdthunk library with express editions. Also, explicitly link to the base WinSDK libraries in common.gypi and mini_installer.gypi for MSVC 2005 express. 
2) Fixes a few .cc files that have the wrong include order with the ATL headers when using the Windows DDK ATL. The Windows DDK ATL brings in intsafe.h (WinSDK, not WinDDK) with atlwin.h and generates multiple INTXX_MIN/MAX def warnings which get 
flagged as errors with the warnings as errors flag if not included before other libraries that have the same definitions like ICU. 
3) Changes to .rc files to avoid pulling in afxres.h (an MFC header - 
it's available in the WinDDK) and winres.h which VCExpress doesn't have (it's available in a WinSDK sample, but that kind of the purpose of it). The main Chromium .rc files are already structured this way, I just changed the rest and changed the output of grit to do the same. 
4) Removes the memset obj file linking in mini_installer.gyp and simply implements memset for mini_installer.cc. 

Only changes to the chromium branch now. There are some .rc files in the Python26, Native Client, and Angle in samples that could #3 changes. They are not required for Chromium, however. 
------ 
VC2005SP1 can be downloaded at 
http://www.microsoft.com/downloads/en/details.aspx?FamilyId=7B0B0339-613A-46E6-AB4D-080D4D4A8C4E&displaylang=en
(or non-SP1 at http://download.microsoft.com/download/8/3/a/83aad8f9-38ba-4503-b3cd-ba28c360c27b/ENU/vcsetup.exe)

VC2008SP1 can be downloaded at 
http://www.microsoft.com/downloads/en/details.aspx?FamilyId=F3FBB04E-92C2-4701-B4BA-92E26E408569&displaylang=en 
currently. 

The base developer instructions work fine afterwards with a couple tweaks for express versions: 
http://www.chromium.org/developers/how-tos/build-instructions-windows 

Under "Additional (free) downloads" under step 2: 
X) Only the first 3 Non-SP KB Patches are needed for express. Don't forget to forget GYP_MSVS_VERSION environment as appropriate - 2008e for Visual C++ 2008 Express, for example. 

Under "Additional (free) downloads" after step 5
[These only apply to 2008 express, 2005 works fine out of the box]: 
6A) Download the WinDDK for the atl headers and libs - 
http://msdn.microsoft.com/en-us/windows/hardware/gg487463.aspx. It works fine, but you do not need to install the whole DDK. In the WDK directory just install headers.msi, libs_x86fre.msi, and libs_x64fre.msi; just grab the atl headers and atl*.lib libs; right click the installers and uninstall afterwards. Add the appropriate include and lib paths to your global settings. 
6B) To build x64 targets (x64 Native Client) download: 
http://www.cppblog.com/Files/xcpp/VCE64BIT_WIN7SDK.zip 
Follow the readme instructions. 
Further information behind that and x64 target building with express: 
- http://jenshuebel.wordpress.com/2009/02/12/visual-c-2008-express-edition-and-64-bit-targets/ 
- http://www.cppblog.com/xcpp/archive/2009/09/09/vc2008express_64bit_win7sdk.html 
---------------- 
BUG=1433, 5026, 72885
TEST=Compiles in both Debug and Release mode in Visual C++ Express 2008/2005 and does not effect other build setups. In addition, the WinDDK ATL compiles with the secure_atl=0 in the GYP_DEFINES environment variable on non-express versions of MSVC.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78921

[RN, 2011-03-16 02:55:49]

Take a look and let me know what you think. 

Original discuss thread where it was suggested for a review for each repository:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre....

Have yet to do the other 3, they'll come shortly unless this is shot down as not
preferred.

Thanks for your time!

[RN, 2011-03-16 09:57:05]

Updated description and patch; no modifications no longer needed to other
repositories. 

The new changes include switching the approach to simply use the
COMPILER_MSVCEXPRESS ifdef to bring in the atl headers first instead of
rearranging the chromium headers in the 3 offending files that trigger that
intsafe.h multiple as there were some concerns expressing about that approach.

All lint errors are now fixed in the modified files, even a couple old ones that
are a bit unrelated (a comment missing a space, using unsigned long instead of
DWORD for ::GetLastError()).

[Mark Mentovai, 2011-03-16 16:25:19]

Overall, I think you’ve done a nice job. I’d like a Windows expert to comment on
the Windows-specific bits.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1480
build/common.gypi:1480: 'AdditionalOptions': ['/DCOMPILER_MSVCEXPRESS'],
All of the other COMPILER_* macros are set in build/build_config.h. Isn’t there
some predefined macro that we can switch on in that file to set
COMPILER_MSVCEXPRESS?

I’d also favor COMPILER_MSVC_EXPRESS.

We’ll also want to nail down properly whether the “express” macro is a subset of
the ordinary COMPILER_MSVC macro.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1481
build/common.gypi:1481: },{
Most of the “else” blocks in this file put a space between the , and {. Try to
observe the surrounding style. Same on line 1486, where you’re changing an
existing “}, {” unnecessarily.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1482
build/common.gypi:1482: 'AdditionalOptions': ['/D_SECURE_ATL'], #WinDDKATL +
_SECURE_ATL = error
The general style would be two spaces before the # and one space after. Also
observe this convention on line 1509.

I don’t know what WinDDKATL is. It doesn’t show up anywhere else in this file
and Google’s results are less than helpful. I can figure out what I think you
mean from your changelist description, but I shouldn’t have to do that to
decipher a comment.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1487
build/common.gypi:1487: 'ExceptionHandling': '0',
Adding unnecessary whitespace here?

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1510
build/common.gypi:1510: ['MSVS_VERSION=="2010e" or MSVS_VERSION=="2008e" or
MSVS_VERSION=="2005e"', {
Perhaps it would make sense to have some sort of “flavor” variable that’s set to
1 when an “express” variant is in use. It would be handy if this condition comes
up frequently.

http://codereview.chromium.org/6676030/diff/4002/chrome/browser/ui/views/auto...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/browser/ui/views/auto...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #ifdef
COMPILER_MSVCEXPRESS
I’d like a Windows expert to weigh in on whether doing these #includes early for
COMPILER_MSVCEXPRESS is right, or if there’s something else to key off here.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
File chrome/installer/mini_installer.gyp (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer.gyp:65: # but not 2010. Here we pull in the
2008/vc9 crt obj files,
Nit: don’t write “we” in comments. Who are “we,” anyway?

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:32: #include <intrin.h>  // 
__stosb for memset
Two spaces before the //, one after.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:713: extern "C" {
Don’t indent this line at all.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:714: #pragma function(memset)
Never indent C preprocessor directives.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:715: void* memset(void *dest,
int ci, size_t count) {
The indentation in here is wrong, too.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:721: 
Get rid of the blank line at EOF.

[M-A Ruel, 2011-03-16 17:48:55]

For safety reasons, I'd prefer this patch to be split in two, having the gypi
part being checked in after. It will make reverting simpler.

http://codereview.chromium.org/6676030/diff/4002/chrome/browser/ui/views/auto...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/browser/ui/views/auto...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #ifdef
COMPILER_MSVCEXPRESS
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> I’d like a Windows expert to weigh in on whether doing these #includes early
for
> COMPILER_MSVCEXPRESS is right, or if there’s something else to key off here.

I'd favor something that doesn't need fixes like this if possible because the
only thing I know for sure is that it'll keep on breaking.

http://codereview.chromium.org/6676030/diff/4002/chrome/default_plugin/plugin...
File chrome/default_plugin/plugin_database_handler.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/default_plugin/plugin...
chrome/default_plugin/plugin_database_handler.cc:126: DWORD error =
::GetLastError();
why?

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:32: #include <intrin.h>  // 
__stosb for memset
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Two spaces before the //, one after.

Just include it, no need for #ifdef.

[RN, 2011-03-17 06:33:24]

New set uploaded that should the comments - thanks for the reviews. Let me know
if you think the header check approach is OK.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1480
build/common.gypi:1480: 'AdditionalOptions': ['/DCOMPILER_MSVCEXPRESS'],
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> All of the other COMPILER_* macros are set in build/build_config.h. Isn’t
there
> some predefined macro that we can switch on in that file to set
> COMPILER_MSVCEXPRESS?
> 
> I’d also favor COMPILER_MSVC_EXPRESS.
> 
> We’ll also want to nail down properly whether the “express” macro is a subset
of
> the ordinary COMPILER_MSVC macro.

I do not think there is a way to get a macro for express editions. 

Changed to COMPILER_MSVC_EXPRESS.

As for whether to nail down whether or not make a subset of COMPILER_MSVC, I'm
not sure about that. I'll let the Chromium team decide that, though with this
approach at least for now I don't breaking code for other versions.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1481
build/common.gypi:1481: },{
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Most of the “else” blocks in this file put a space between the , and {. Try to
> observe the surrounding style. Same on line 1486, where you’re changing an
> existing “}, {” unnecessarily.

I thought I was changing to the norm of the rest of the other .gyp* files, then
again it is a bit inconsistent. Changed.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1482
build/common.gypi:1482: 'AdditionalOptions': ['/D_SECURE_ATL'], #WinDDKATL +
_SECURE_ATL = error
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> The general style would be two spaces before the # and one space after. Also
> observe this convention on line 1509.

Right, that was a bit terse. I changed that area a bit, I simply eliminated the
_SECURE_ATL define altogether - there is no clean way to get the Windows DDK's
(Driver Development Kit) version that gives an error when it is defined in the
same base header that gives the _ATL_VERSION define. _SECURE_ATL adds extra
checks to CRT calls it makes internally; the DDK version has it on by default.

> I don’t know what WinDDKATL is. It doesn’t show up anywhere else in this file
> and Google’s results are less than helpful. I can figure out what I think you
> mean from your changelist description, but I shouldn’t have to do that to
> decipher a comment.

Yeah, it was a bit terse and unneeded.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1487
build/common.gypi:1487: 'ExceptionHandling': '0',
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Adding unnecessary whitespace here?

Ug. It seems something keeps adding extra whitespace in odd places. Anyway,
fixed; thanks.

http://codereview.chromium.org/6676030/diff/4002/build/common.gypi#newcode1510
build/common.gypi:1510: ['MSVS_VERSION=="2010e" or MSVS_VERSION=="2008e" or
MSVS_VERSION=="2005e"', {
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Perhaps it would make sense to have some sort of “flavor” variable that’s set
to
> 1 when an “express” variant is in use. It would be handy if this condition
comes
> up frequently.

Done.

http://codereview.chromium.org/6676030/diff/4002/chrome/browser/ui/views/auto...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/browser/ui/views/auto...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #ifdef
COMPILER_MSVCEXPRESS
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> I’d like a Windows expert to weigh in on whether doing these #includes early
for
> COMPILER_MSVCEXPRESS is right, or if there’s something else to key off here.

It's not exclusive to the express versions, just the Windows DDK's version of
the ATL. Therefore, I'm taking a different approach and eliminating the ifdefs
and making a include for when other people are using non-DDK ATL versions to
toss warnings/errors when the include order is wrong (a.k.a. when the
atlbase+atlwin combo is needed explicitly in the source file). This way, the
Chromium devs won't have to worry about breaking that aspect of the express
compilation process.

http://codereview.chromium.org/6676030/diff/4002/chrome/default_plugin/plugin...
File chrome/default_plugin/plugin_database_handler.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/default_plugin/plugin...
chrome/default_plugin/plugin_database_handler.cc:126: DWORD error =
::GetLastError();
On 2011/03/16 17:48:55, Marc-Antoine Ruel wrote:
> why?

It was a lint error. WinSDK GetLastError returns a DWORD, not an unsigned long
(guess it's afraid if long ever becomes 64-bit etc.).

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
File chrome/installer/mini_installer.gyp (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer.gyp:65: # but not 2010. Here we pull in the
2008/vc9 crt obj files,
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Nit: don’t write “we” in comments. Who are “we,” anyway?

Work habit. Changed.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:32: #include <intrin.h>  // 
__stosb for memset
On 2011/03/16 17:48:55, Marc-Antoine Ruel wrote:
> On 2011/03/16 16:25:20, Mark Mentovai wrote:
> > Two spaces before the //, one after.
> 
> Just include it, no need for #ifdef.

Done and Done.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:713: extern "C" {
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Don’t indent this line at all.

Done.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:714: #pragma function(memset)
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Never indent C preprocessor directives.

Done.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:715: void* memset(void *dest,
int ci, size_t count) {
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> The indentation in here is wrong, too.

Think I fixed this.

http://codereview.chromium.org/6676030/diff/4002/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:721: 
On 2011/03/16 16:25:20, Mark Mentovai wrote:
> Get rid of the blank line at EOF.

Done.

[RN, 2011-03-17 13:22:53]

Some final touchups to common.gypi to use it's built-in defines rather then the
/D compiler flag. Also, after hours of reading reading gyp information figured
out a decent way to handle the _SECURE_ATL issue. 

_SECURE_ATL is back to being turned on by default for non-express editions now,
but is now user-configurable as well in case they use the WinDDK ATL with
non-express versions and vice versa (through GYP_DEFINES's secure_atl).

[M-A Ruel, 2011-03-17 14:01:24]

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1467
build/common.gypi:1467: 'COMPILER_MSVC_EXPRESS',
I prefer COMPILER_MSVC_EXPRESS to be set in build/build_config.h like
COMPILER_MSVC and COMPILER_GCC.

http://codereview.chromium.org/6676030/diff/4039/chrome/browser/ui/views/auto...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/4039/chrome/browser/ui/views/auto...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #include
<atlbase.h>
You cannot do this, atlbase.h doesn't exist on mac or linux. By unconditional, I
meant "on windows" with #if defined(OS_WIN)

[Mark Mentovai, 2011-03-17 16:45:10]

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode457
build/common.gypi:457: 'secure_atl%': 0,
For my own benefit, what’s “secure” ATL? Is this related to what you were
discussing about the DDK ATL?

My current understanding is that Express doesn’t have its own ATL so you’re
forced to use an external ATL, which you’re getting from the DDK, and it’s
somewhat different than the ATL that ships with non-Express MSVC. Is this
correct?

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1465
build/common.gypi:1465: [ 'msvs_express', {
It’s unusual to put a space between the [ and ' as is done here and on 1470,
1493, and 1496. The style above on 1460 and below on 1522 is much more common.

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1467
build/common.gypi:1467: 'COMPILER_MSVC_EXPRESS',
This is now only used in exactly one location. If we can kill that location, we
can kill this macro altogether, which would probably be the best solution.

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1522
build/common.gypi:1522: ['msvs_express', {
Does this section have to do with Express or with secure_atl?

http://codereview.chromium.org/6676030/diff/4039/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/4039/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:31: #include <intrin.h>  //
__stosb for MSVC express memset implementation
Maybe you can avoid diverging here by #including <intrin.h> to get __stosb and
then defining a static MyMemset function in this file that will be used with
both Express and non-Express. You can adjust the largeish comment above to
reflect what’s happening. This will allow you to get rid of the goofy
__sse2_availalbe hack, it may even reduce mini_installer’s size.

memset isn’t a complicated function to implement and performance isn’t
ultra-critical here so I’m a little surprised that the “link the object
files” approach was even taken in the first place.

http://codereview.chromium.org/6676030/diff/4039/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:831: void* memset(void *dest,
int ci, size_t count) {
This and line 834 should be indented flush with the left margin. No indentation
within an extern "C" block. Instead, put a blank line between lines 829 and 830,
and another between 834 and 835 to set them off. Lines 832 and 833 should be
indented at 2 spaces.  Here’s what I’d write:

extern "C" {

#pragma function(memset)
void* memset(void *dest, int ci, size_t count) {
  __stosb((unsigned char*)dest, (unsigned char)ci, count);
  return dest;
}

}  // extern "C"

[Mark Mentovai, 2011-03-17 16:49:48]

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode457
build/common.gypi:457: 'secure_atl%': 0,
Mark Mentovai wrote:
> For my own benefit, what’s “secure” ATL? Is this related to what you were
> discussing about the DDK ATL?

I looked back through your responses to my previous questions and saw that you
covered this. Sorry for asking the question again, I understand what’s going on
now. Thanks for your explanation.

[RN, 2011-03-17 16:50:27]

New patch with merge conflict resolved and the platform independence in source
files taken care of.

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1467
build/common.gypi:1467: 'COMPILER_MSVC_EXPRESS',
On 2011/03/17 14:01:24, Marc-Antoine Ruel wrote:
> I prefer COMPILER_MSVC_EXPRESS to be set in build/build_config.h like
> COMPILER_MSVC and COMPILER_GCC.

So would I, but it isn't possible to determine an express version at
compile/preprocessor time. 

Unless... you mean you want to literally do a different named define here and
then put it in build/build_config.h also.

http://codereview.chromium.org/6676030/diff/4039/chrome/browser/ui/views/auto...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/4039/chrome/browser/ui/views/auto...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #include
<atlbase.h>
On 2011/03/17 14:01:24, Marc-Antoine Ruel wrote:
> You cannot do this, atlbase.h doesn't exist on mac or linux. By unconditional,
I
> meant "on windows" with #if defined(OS_WIN)

Nice catch. Done here and in the other source files.

[Mark Mentovai, 2011-03-17 16:54:18]

http://codereview.chromium.org/6676030/diff/16001/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/16001/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #if
defined(OS_WIN)
http://www.chromium.org/developers/coding-style:

#includes should be ordered per Google style, except platform-specific (#if
defined(..)) headers to follow the cross platform headers in a separate section,
e.g.:

#include "foo/bar/someclass.h"

#include <set>

#include "foo/bar/a.h"
#include "foo/bar/b.h"

#if defined(OS_WIN)
#include "foo/bar/a_win.h"
#include "foo/bar/b_win.h"
#elif defined(OS_POSIX)
#include "foo/bar/a_posix.h"
#endif

See also
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

[Randy Smith (Not in Mondays), 2011-03-17 17:17:23]

Ryan: This is a fairly large patch, and I don't see systems I have any specific
expertise in on the list, so I'm not sure why I'm on the reviewer list.  Are
there specific changes you'd like me to look at?  I don't (yet) feel like I'm a
good person to do large scale reviews in the chromium code base.

[RN, 2011-03-17 20:15:11]

Yet another patch with most of the comments addressed I think. I can't address
the header ordering as noted by Mark Mentovai - I provided a detailed
explanation as to why.

On 2011/03/17 17:17:23, rdsmith wrote:
> Ryan: This is a fairly large patch, and I don't see systems I have any
specific
> expertise in on the list, so I'm not sure why I'm on the reviewer list.  Are
> there specific changes you'd like me to look at?  I don't (yet) feel like I'm
a
> good person to do large scale reviews in the chromium code base.

I asked on IRC, they suggested to check OWNERS and svn blame, and I added a few
that showed up the most. The rest were added by others I think.

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1465
build/common.gypi:1465: [ 'msvs_express', {
On 2011/03/17 16:45:10, Mark Mentovai wrote:
> It’s unusual to put a space between the [ and ' as is done here and on 1470,
> 1493, and 1496. The style above on 1460 and below on 1522 is much more common.

It's still inconsistent; just following the existing surrounding code as
previously instructed in a similar instance. 

Done.

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1467
build/common.gypi:1467: 'COMPILER_MSVC_EXPRESS',
On 2011/03/17 16:45:10, Mark Mentovai wrote:
> This is now only used in exactly one location. If we can kill that location,
we
> can kill this macro altogether, which would probably be the best solution.

Yeah, I can agree with that. Done.

http://codereview.chromium.org/6676030/diff/4039/build/common.gypi#newcode1522
build/common.gypi:1522: ['msvs_express', {
On 2011/03/17 16:45:10, Mark Mentovai wrote:
> Does this section have to do with Express or with secure_atl?

This is actually express-specific. Non-express versions will try to link this
library automatically; this applies to all ATL implementations and has nothing
to the WinDDK ATL (at least since ~VC6).

http://codereview.chromium.org/6676030/diff/4039/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/4039/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:31: #include <intrin.h>  //
__stosb for MSVC express memset implementation
On 2011/03/17 16:45:10, Mark Mentovai wrote:
> Maybe you can avoid diverging here by #including <intrin.h> to get __stosb and
> then defining a static MyMemset function in this file that will be used with
> both Express and non-Express. You can adjust the largeish comment above to
> reflect what’s happening. This will allow you to get rid of the goofy
> __sse2_availalbe hack, it may even reduce mini_installer’s size.
> 
> memset isn’t a complicated function to implement and performance isn’t
> ultra-critical here so I’m a little surprised that the “link the object
> files”&nbsp;approach was even taken in the first place.

Well, __sse2_available is supposed to automatically determined by the compiler;
really rare to the user to specify it. I'm guessing that's because it was
requiring linking to both P4 and normal memset obj files.

Done.

http://codereview.chromium.org/6676030/diff/4039/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:831: void* memset(void *dest,
int ci, size_t count) {
On 2011/03/17 16:45:10, Mark Mentovai wrote:
> This and line 834 should be indented flush with the left margin. No
indentation
> within an extern "C" block. Instead, put a blank line between lines 829 and
830,
> and another between 834 and 835 to set them off. Lines 832 and 833 should be
> indented at 2 spaces.  Here’s what I’d write:
> 
> extern "C" {
> 
> #pragma function(memset)
> void* memset(void *dest, int ci, size_t count) {
>   __stosb((unsigned char*)dest, (unsigned char)ci, count);
>   return dest;
> }
> 
> }  // extern "C"

OK - Done.

http://codereview.chromium.org/6676030/diff/16001/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/16001/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #if
defined(OS_WIN)
On 2011/03/17 16:54:18, Mark Mentovai wrote:
> http://www.chromium.org/developers/coding-style:
> 
> #includes should be ordered per Google style, except platform-specific (#if
> defined(..)) headers to follow the cross platform headers in a separate
section,
> e.g.:
> 
> #include "foo/bar/someclass.h"
> 
> #include <set>
> 
> #include "foo/bar/a.h"
> #include "foo/bar/b.h"
> 
> #if defined(OS_WIN)
> #include "foo/bar/a_win.h"
> #include "foo/bar/b_win.h"
> #elif defined(OS_POSIX)
> #include "foo/bar/a_posix.h"
> #endif
> 
> See also
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Actually, that's what half (or more, may as well name the WinDDK ATL patch with
a few express tweaks on the side) this patch is about, which has nothing to
VCExpress but everything to do with the Windows DDK ATL and WinSDK header
ordering relative to the Chromium header ordering. 

Mentioned several times already; the reason that the ATL headers are placed
where they are is because if the WinDDK ATL headers come after ICU or other
headers that define XINT/ETC_MIN/MAX you're going to get multiple definition
warnings since WinDDK ATL pulls in intsafe.h which doesn't wrap the
XINT/ETC_MIN/MAX definitions around ifdefs. Since a good part of the Chromium
project files have the warnings as errors flag on... well, they turn into
errors.

Third party headers are pulled in first before system headers included by other
project headers all over the place alphabetically. Yes, lint likes this, but
isn't always practical, as WinSDK/DDK and other system headers are _always_
supposed to come before third party headers.

This patch just sticks to the existing Chromium coding style/header ordering as
much as possible while not breaking the WinDDK ATL. That's the reason for
existence of atlcheck.h - to enforce header ordering if one isn't using the
WinDDK ATL and to cause warnings/errors as if you were.

Hopefully that makes sense.

[Randy Smith (Not in Mondays), 2011-03-17 20:27:06]

On 2011/03/17 20:15:11, RN wrote:
> On 2011/03/17 17:17:23, rdsmith wrote:
> > Ryan: This is a fairly large patch, and I don't see systems I have any
> specific
> > expertise in on the list, so I'm not sure why I'm on the reviewer list.  Are
> > there specific changes you'd like me to look at?  I don't (yet) feel like
I'm
> a
> > good person to do large scale reviews in the chromium code base.
> 
> I asked on IRC, they suggested to check OWNERS and svn blame, and I added a
few
> that showed up the most. The rest were added by others I think.

Well, I've scanned the file list twice now, and I can't see any files I think
I'm a particularly good reviewer for, so I'm going to take myself off the review
list.  If anyone wants to put me back on, feel free, just tell me why :-}.

[Mark Mentovai, 2011-03-17 20:43:42]

I suspect that my review and Marc-Antoine’s are all you’ll need.

http://codereview.chromium.org/6676030/diff/16010/base/win/atlcheck.h
File base/win/atlcheck.h (right):

http://codereview.chromium.org/6676030/diff/16010/base/win/atlcheck.h#newcode5
base/win/atlcheck.h:5: #ifndef BASE_WIN_EVENT_ATLCHECK_H_
Fix the #include guards. The macro should not have EVENT in it.

http://codereview.chromium.org/6676030/diff/16010/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #if
defined(OS_WIN)
OK. If you ever need to do an early OS-specific #include as you say you need to
do here, you need to make sure of the following:

1. #include "build/build_config.h" first, otherwise OS_WIN may not be defined.
In this case, it probably works out fine because autocomplete_result_view.h
ultimately winds up including build_config.h, but if someone changes
build_config.h or some file on which it depends, that may cease to be the case.

2. Provide a quick comment along the lines of

// These headers are intentionally #included before any others.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
File chrome/installer/mini_installer.gyp (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer.gyp:61: 'AdditionalDependencies': [
Did you confirm whether this change results in a measurable change in size? I’m
just curious.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:821: // linking with linking
to the dual x64 and x86 obj files.
Revise “linking with linking to”—pick one. :)

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:822: // extern "C"
__sse2_available =
This comment looks like code but it’s not. defined(_M_IX86) isn’t even really
correct either. I’d either write

// extern "C" int __sse2_available = 0;

or just leave the codeish bit out altogether.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:828: // Typical 32-bit memset
c implementation
This assumes that dest is properly aligned. It’ll work but the performance
advantage may be negated if dest isn’t. I suspect that’s fine given the use
case.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:829: size_t adjcount =
count>>2;
Style nit: spaces around operators like >>.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:830: UINT32 fill =
(c<<24|c<<16|c<<8|c);
Here too. (c << 24 | c << 16 | c << 8 | c)

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:832: UINT8*  dest8 = (unsigned
__int8*)dest;
No need to line up dest8 and dest32, especially when you haven’t done anything
to line up the rest of the line. Rein this one in.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:835: switch (count -
(adjcount<<2)) {
Space around that << operator.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:836: case 3:  dest8[count - 3]
= c;
The assignments here really belong on the next line and suitably indented. We
rarely do this stuff on the same line, and reading it this way makes me think
harder about the “break” omission. (But I see that you do want fall-through.)

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:842: *(dest32++) = fill;
Fix the indent.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:847: }
}  // extern "C"

http://codereview.chromium.org/6676030/diff/16010/chrome_frame/test_utils.h
File chrome_frame/test_utils.h (right):

http://codereview.chromium.org/6676030/diff/16010/chrome_frame/test_utils.h#n...
chrome_frame/test_utils.h:10: #include <atlbase.h>
These should precede <string> (unless there’s some good reason for them not to.)

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
File views/controls/table/table_view_unittest.cc (right):

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:5: #if defined(_WIN32) ||
defined(_WIN64)  // OS_WIN
Can we #include "build/build_config.h" first and then use OS_WIN for real?

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:7: #include <atlwin.h>
With this included, don’t you need "base/win/atlcheck.h" too?

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:8: #endif
Blank line after this now.

[RN, 2011-03-18 09:34:23]

Thanks for another detailed review. This should address the comments, and then
some. I also went ahead and fixed the header order in the headers I touched as
half of them put a line in-between the STL and the C headers and the other
half... didn't.

Also changed atlcheck.h to actually check instead of pulling in a header (I had
to think for quite a few hours correctly in this case).

http://codereview.chromium.org/6676030/diff/16010/base/win/atlcheck.h
File base/win/atlcheck.h (right):

http://codereview.chromium.org/6676030/diff/16010/base/win/atlcheck.h#newcode5
base/win/atlcheck.h:5: #ifndef BASE_WIN_EVENT_ATLCHECK_H_
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Fix the #include guards. The macro should not have EVENT in it.

Done.

http://codereview.chromium.org/6676030/diff/16010/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:7: #if
defined(OS_WIN)
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> OK. If you ever need to do an early OS-specific #include as you say you need
to
> do here, you need to make sure of the following:
> 
> 1. #include "build/build_config.h" first, otherwise OS_WIN may not be defined.
> In this case, it probably works out fine because autocomplete_result_view.h
> ultimately winds up including build_config.h, but if someone changes
> build_config.h or some file on which it depends, that may cease to be the
case.
> 
> 2. Provide a quick comment along the lines of
> 
> // These headers are intentionally #included before any others.

Makes sense. Done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
File chrome/installer/mini_installer.gyp (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer.gyp:61: 'AdditionalDependencies': [
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Did you confirm whether this change results in a measurable change in size?
I’m
> just curious.

I didn't notice a substantial change (maybe saves ~10k), but I haven't done
builds with link time code generation on lately.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:821: // linking with linking
to the dual x64 and x86 obj files.
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Revise “linking with linking to”—pick one. :)

Yeah.... rewrote that comment and explained it a bit more.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:822: // extern "C"
__sse2_available =
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> This comment looks like code but it’s not. defined(_M_IX86) isn’t even really
> correct either. I’d either write
> 
> // extern "C" int __sse2_available = 0;
> 
> or just leave the codeish bit out altogether.

Done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:828: // Typical 32-bit memset
c implementation
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> This assumes that dest is properly aligned. It’ll work but the performance
> advantage may be negated if dest isn’t. I suspect that’s fine given the use
> case.

Put in more comments about this (even if isn't that critical).

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:829: size_t adjcount =
count>>2;
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Style nit: spaces around operators like >>.

Done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:830: UINT32 fill =
(c<<24|c<<16|c<<8|c);
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Here too. (c << 24 | c << 16 | c << 8 | c)

Done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:832: UINT8*  dest8 = (unsigned
__int8*)dest;
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> No need to line up dest8 and dest32, especially when you haven’t done anything
> to line up the rest of the line. Rein this one in.

Nice catch, done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:835: switch (count -
(adjcount<<2)) {
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Space around that << operator.

Done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:836: case 3:  dest8[count - 3]
= c;
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> The assignments here really belong on the next line and suitably indented. We
> rarely do this stuff on the same line, and reading it this way makes me think
> harder about the “break” omission. (But I see that you do want fall-through.)

OK. Also added a longish comment about how it works just in case.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:842: *(dest32++) = fill;
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Fix the indent.

Assuming the 2 spaces rule, done.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:847: }
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> }  // extern "C"

Done.

http://codereview.chromium.org/6676030/diff/16010/chrome_frame/test_utils.h
File chrome_frame/test_utils.h (right):

http://codereview.chromium.org/6676030/diff/16010/chrome_frame/test_utils.h#n...
chrome_frame/test_utils.h:10: #include <atlbase.h>
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> These should precede <string> (unless there’s some good reason for them not
to.)

Yeah, done.

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
File views/controls/table/table_view_unittest.cc (right):

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:5: #if defined(_WIN32) ||
defined(_WIN64)  // OS_WIN
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Can we #include "build/build_config.h" first and then use OS_WIN for real?

It's sort of a gray area in the google guidelines I read, but I did this anyway.
Makes it a lot cleaner, but lint doesn't like it.

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:7: #include <atlwin.h>
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> With this included, don’t you need "base/win/atlcheck.h" too?

atlcheck.h is just for headers (added more docs to the header itself) to enforce
the header order in the source files when not using the WinDDK ATL.

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:8: #endif
On 2011/03/17 20:43:42, Mark Mentovai wrote:
> Blank line after this now.

Done.

[Mark Mentovai, 2011-03-18 14:54:47]

We’re down to the most minor of nits now.

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/6676030/diff/16010/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:842: *(dest32++) = fill;
RN wrote:
> Assuming the 2 spaces rule, done.

Yes, 2 spaces. For reference, we write to the style guide at

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
File views/controls/table/table_view_unittest.cc (right):

http://codereview.chromium.org/6676030/diff/16010/views/controls/table/table_...
views/controls/table/table_view_unittest.cc:5: #if defined(_WIN32) ||
defined(_WIN64)  // OS_WIN
RN wrote:
> It's sort of a gray area in the google guidelines I read, but I did this
anyway.
> Makes it a lot cleaner, but lint doesn't like it.

It’s not a gray area as far as I’m concerned. If lint hates it, you should be
able to say // NOLINT on the line where lint believes there’s a violation to
suppress any warnings.

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h
File base/win/atlcheck.h (right):

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h#newcode8
base/win/atlcheck.h:8: // USAGE: Put after atl headers in all headers; headers
only.
Put after ATL headers in all headers that include ATL

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h#newcode10
base/win/atlcheck.h:10: // Windows DDK 7.1's ATL atlmem.h pulls in intsafe.h
which has it's
it's -> its

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h#newcode19
base/win/atlcheck.h:19: # if defined(INT8_MAX) && !defined(DWORD_ERROR)  //
DWORD_ERROR is intsafe.h
We never indent preprocessor macros at all. Pull all of these guys back so that
they’re flush with their opening #.

[Mark Mentovai, 2011-03-18 14:55:04]

We’re down to the most minor of nits now.

[RN, 2011-03-18 16:39:55]

Added a few related lint things in addition to the comments below: NOLINT to
appease it for the offending headers in the .cc files as well a couple tweaks -
removing a couple blank lines it didn't like and using reinterpret_cast instead
of the C style - to mini_installer.cc.

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h
File base/win/atlcheck.h (right):

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h#newcode8
base/win/atlcheck.h:8: // USAGE: Put after atl headers in all headers; headers
only.
On 2011/03/18 14:54:47, Mark Mentovai wrote:
> Put after ATL headers in all headers that include ATL

Sounds better, done.

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h#newcode10
base/win/atlcheck.h:10: // Windows DDK 7.1's ATL atlmem.h pulls in intsafe.h
which has it's
On 2011/03/18 14:54:47, Mark Mentovai wrote:
> it's -> its

Done.

http://codereview.chromium.org/6676030/diff/19030/base/win/atlcheck.h#newcode19
base/win/atlcheck.h:19: # if defined(INT8_MAX) && !defined(DWORD_ERROR)  //
DWORD_ERROR is intsafe.h
On 2011/03/18 14:54:47, Mark Mentovai wrote:
> We never indent preprocessor macros at all. Pull all of these guys back so
that
> they’re flush with their opening #.

Yeah, done. While the guidelines said no indentation a file search revealed a
lot of preprocessor-heavy files were following that indentation convention;
mostly "third party" libs like tcmalloc, though base\stl_util-inl.h and a few
other Chromium files do it also.

[Mark Mentovai, 2011-03-18 16:49:25]

LGTM

(Still needs a try run before checking in. I can be your checkin-buddy if you
don’t already have one.)

Where are you on the contributor license agreement? It doesn’t seem that you’ve
signed one yet. If you’re an individual contributor, read
http://code.google.com/legal/individual-cla-v1.0.html, and if you agree to it,
sign it. (The electronic option is the fastest.) If you’re working for a company
that owns this work, use http://code.google.com/legal/corporate-cla-v1.0.html
instead.

Also, add yourself to the top-level AUTHORS file. You can include that in this
change.

[M-A Ruel, 2011-03-18 16:53:44]

On 2011/03/18 16:49:25, Mark Mentovai wrote:
> LGTM

IMHO, I still think this change is too large, I'd prefer one with *.rc, *.py and
AUTHORS, the other with the actual code and *.gypi.

[RN, 2011-03-18 22:58:05]

On 2011/03/18 16:49:25, Mark Mentovai wrote:
> (Still needs a try run before checking in. I can be your checkin-buddy if you
> don’t already have one.)

That would be great; the 3 platforms (win/mac/linux) and whatever else you feel
would be helpful for the trybots. Just for sanity checks, though there is a
chance of the windows one failing due to ATL clashes as I don't have the full
version here (companies often never give you it, heh...).
 
On 2011/03/18 16:49:25, Mark Mentovai wrote:
> Where are you on the contributor license agreement? It doesn’t seem that
you’ve
> signed one yet. If you’re an individual contributor, read
> http://code.google.com/legal/individual-cla-v1.0.html, and if you agree to it,
> sign it. (The electronic option is the fastest.) If you’re working for a
company
> that owns this work, use http://code.google.com/legal/corporate-cla-v1.0.html
> instead.

Strange, I thought I signed one a year ago or something (though never
contributed anything to google).... anyway did the electronic option individual
thing.

On 2011/03/18 16:49:25, Mark Mentovai wrote:
> Also, add yourself to the top-level AUTHORS file. You can include that in this
> change.

OK, done.


On 2011/03/18 16:53:44, Marc-Antoine Ruel wrote:
> IMHO, I still think this change is too large, I'd prefer one with *.rc, *.py
and
> AUTHORS, the other with the actual code and *.gypi.

I can understand that point of view, however my preference is just to keep it
this way as everything here is connected to the same goal (and the WinDDK
ATL/express are connected to each other).

[Mark Mentovai, 2011-03-21 16:04:08]

Sent out to the try server. Non-Windows may show a failure during the
update/patch step because of the line ending characters aren’t set properly on a
few of the .rc files. This is a problem in the repository, not in the patch.

[RN, 2011-03-21 16:31:12]

On 2011/03/21 16:04:08, Mark Mentovai wrote:
> Sent out to the try server. Non-Windows may show a failure during the
> update/patch step because of the line ending characters aren’t set properly on
a
> few of the .rc files. This is a problem in the repository, not in the patch.

Thanks, that was fast! No problem with the linux compile; win failed due to
atlcheck on the buildbot - other then that was fine. Think I'll split that
portion into a separate patch, it has been bugging me a bit over the weekend.

I've also got 2005 express working as well (just needed a couple library
includes in the gypi file).

[RN, 2011-03-21 17:02:12]

Ok, patch uploaded and good to go; mind doing one more trybot round? Should be
fine this time. 

As mentioned, I took out the DDK ATL breakage detection (atlcheck.h) as that is
a lot of files changed and I've been second guessing it lately.

[Mark Mentovai, 2011-03-21 21:00:18]

r78921, thanks for the patch!