Use the system stdint.h on Windows.

Use the system stdint.h on Windows.

This is now possible since VC2010 and later provide a stdint.h. VC2008
has been deprecated for building Chromium (and Skia) for a full year now.

There are a number of stdint.h implementations in the codebase, some of
which hide others, and most of which are not complete (Skia in particular
requires uintmax_t which this one did not provide).

R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159684

[bungeman-chromium, 2013-10-12 03:54:33]

One possible alternative is to add uintmax_t and intmax_t and otherwise make
this more complete (having the name stdint.h and being on the include search
path but being incomplete is quite limiting). However, since all supported
Windows tool chains now provide stdint.h by default, it seems easiest and most
correct to simply use the system version.

[abarth-chromium, 2013-10-12 05:20:51]

LGTM.  Yay for using the system version.  Is there most stuff in this folder we
can delete?

[tkent, 2013-10-12 07:46:06]

icu\source\common\unicode\pwin32.h has

#if _MSC_VER >= 1700
#include <stdint.h>
#endif

ICU might not work with stdint.h in VS2010.  Adding stdint.h before ICU headers
might resolve build errors.
Anyway, please upload a buildable patch.

[bungeman-chromium, 2013-10-12 19:41:59]

I present, at patch set 2, a work-around for discussion. ICU does work properly
with vs2010 and its stdint.h, but only if the stdint.h is included first, since
ICU has checks around all of it's macro defines and stdint.h does not. I've run
this through one bot now
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/208211
and the two test failures there look common to other builds at that revision, so
I don't think this caused them. I'm sending off another try from here to make
sure.

[abarth-chromium, 2013-10-12 21:16:46]

https://codereview.chromium.org/26358014/diff/9001/Source/wtf/Compiler.h
File Source/wtf/Compiler.h (right):

https://codereview.chromium.org/26358014/diff/9001/Source/wtf/Compiler.h#newc...
Source/wtf/Compiler.h:77: #endif
This seems like an odd place to put this code.  The point of this head is just
do define macros that describe compiler features, not to work around compile
bugs.

Maybe you want to put this in build/win/Precompile.h?

[bungeman-chromium, 2013-10-13 14:40:25]

So, I must admit I'm at something of a loss here. I have a local machine set up
with only vs2010e and have built once without this patch (patch set 3, modifying
Precompile.h) and then once with the patch (I've done this whole process once in
the IDE and once with ninja). This all appears to work fine locally, but it
looks like the trybots are doing something different. I haven't tried running
locally with GOMA (not sure how the GOMA process actually works, but I did
notice that /showInclude doesn't include the forced includes), so I haven't
ruled that out yet. Maybe the empty stdint.h isn't "actually" empty for some
reason and isn't being deleted? Anyhow, if you have any ideas, I'm open to
suggestions on why the bot might be failing.

https://codereview.chromium.org/26358014/diff/9001/Source/wtf/Compiler.h
File Source/wtf/Compiler.h (right):

https://codereview.chromium.org/26358014/diff/9001/Source/wtf/Compiler.h#newc...
Source/wtf/Compiler.h:77: #endif
On 2013/10/12 21:16:46, abarth wrote:
> This seems like an odd place to put this code.  The point of this head is just
> do define macros that describe compiler features, not to work around compile
> bugs.
> 
> Maybe you want to put this in build/win/Precompile.h?

Yes, this would be much better.

[bungeman-chromium, 2013-10-13 20:24:20]

> So, I must admit I'm at something of a loss here. I have a local machine set
up
> with only vs2010e and have built once without this patch (patch set 3,
modifying
> Precompile.h) and then once with the patch (I've done this whole process once
in
> the IDE and once with ninja). This all appears to work fine locally, but it
> looks like the trybots are doing something different. I haven't tried running
> locally with GOMA (not sure how the GOMA process actually works, but I did
> notice that /showInclude doesn't include the forced includes), so I haven't
> ruled that out yet. Maybe the empty stdint.h isn't "actually" empty for some
> reason and isn't being deleted? Anyhow, if you have any ideas, I'm open to
> suggestions on why the bot might be failing.

It isn't goma. I ran a win_cf (build 512) because it seems to be the last trybot
not using goma, and it failed the same way.

Note that this is failing the same way patch set 1 failed. With patch set 3
applied locally, and redirect ninja output so it can be seen, when building the
wtf target the first step run is

[1/61] CXX obj\third_party\webkit\source\build\win\wtf.precompile.obj

which is not happening on the trybots. After trying to figure out for a while
what is going on, I have discovered the chromium_win_pch which, when not set,
means that precompiled headers are not used. So, at the moment, one cannot rely
on Precompile.h to be force included. Indeed, now that I know what to look for,
I see that the trybots set 'chromium_win_pch': '0' in their gyp_defines.

So, while this is a great idea to do this somewhere other than through config.h,
Precompile.h looks to be out for the moment. I do find it somewhat frightening
that the chromium_win_pch gyp variable has so much control over which headers
are available, it seems like for consistency that Precompile.h should always be
force included (though I understand that there might also be issues with that).
As things are it seems like very subtle differences could be introduced between
'Dev' and 'Official' builds (which set or unset chromium_win_pch, respectively).

> 
> https://codereview.chromium.org/26358014/diff/9001/Source/wtf/Compiler.h
> File Source/wtf/Compiler.h (right):
> 
>
https://codereview.chromium.org/26358014/diff/9001/Source/wtf/Compiler.h#newc...
> Source/wtf/Compiler.h:77: #endif
> On 2013/10/12 21:16:46, abarth wrote:
> > This seems like an odd place to put this code.  The point of this head is
just
> > do define macros that describe compiler features, not to work around compile
> > bugs.
> > 
> > Maybe you want to put this in build/win/Precompile.h?
> 
> Yes, this would be much better

[bungeman-chromium, 2013-10-13 21:38:40]

I present for discussion patch set 4, which contains one possible work-around
for Precompile.h not always being force included.

https://codereview.chromium.org/26358014/diff/26001/Source/build/win/precompi...
File Source/build/win/precompile.gypi (right):

https://codereview.chromium.org/26358014/diff/26001/Source/build/win/precompi...
Source/build/win/precompile.gypi:34: ['OS=="win" and chromium_win_pch==0', {
I am of two minds about this.

On the one hand this is a rather sweeping change which may or may not have an
unpredictable impact on build times on the bots. It also means that all files
get all these defines, which could be seen as a downside, but that's already
happening anyway when precompiled headers are being used. However, it does mean
that we stop getting compile errors when something which needs to be included
isn't being included locally (though this is confusing as it means it built for
the developer but will fail on the bots). It seems like IWYU is supposed to take
care of that sort of thing though.

On the other hand, it means that the state of chromium_win_pch isn't changing
the include order. All the files are getting all of these defines consistently.
Also, it makes this change work.

I suppose one alternative to this is to just force include stdint.h to make sure
it always comes first. I'd have to figure out a good place to do that though...
precompile.gypi is already in a nice place to do this.

[abarth-chromium, 2013-10-14 00:46:59]

not lgtm

This CL seems worse than the alternative of just keeping this header.

[bungeman-chromium, 2013-10-14 01:49:47]

On 2013/10/14 00:46:59, abarth wrote:
> not lgtm
> 
> This CL seems worse than the alternative of just keeping this header.

Yeah, I'm getting a bit bewildered by the build in general. For example the
reason why patch set 4 is failing is because for forced includes the paths are
relativized to the gyp file, not to the source files. Which means the directory
containing the gyp needs to be on the include list for relative forced includes
like this to work. I was surprised by this and was wondering how the precompiled
headers work, since they too depend on force includes. It looks like precompiled
headers get around this by force including what is essentially an arbitrary
name, and then stating that that name is actually a previously generated file.
So yes, I don't think using Precompile.h is really the way to go here.

[bungeman-chromium, 2013-10-14 13:44:30]

On 2013/10/12 07:46:06, tkent wrote:
> icu\source\common\unicode\pwin32.h has
> 
> #if _MSC_VER >= 1700
> #include <stdint.h>
> #endif
> 
> ICU might not work with stdint.h in VS2010.  Adding stdint.h before ICU
headers
> might resolve build errors.
> Anyway, please upload a buildable patch.

I thought as well that ICU might have some reason not to work with the system
stdint.h in vs2010, since this preprocessor test appears to be so specific.
However, the preprocessor directives here are not actually part of ICU, but are
patches to ICU46 by Chromium. These were added a year ago to support vs2012
which has issues if the standard stdint.h is *not* included before ICU's
pwin32.h. There is no particular reason why the number here isn't 1600, and this
code is under Chromium's control. As such I have opened
https://codereview.chromium.org/27079003/ to update the patch to Chromium's copy
of ICU. I have run a tryjob, and if the ICU patch change lands (and rolls), then
patch set 1 (just removing the Blink stdint.h) will work. tkent, thanks for
pointing out this bit of code.

[bungeman-chromium, 2013-10-14 22:37:46]

Mahomet made the people believe that he would call a hill to him, and from the
top of it offer up his prayers, for the observers of his law. The people
assembled; Mahomet called the hill to come to him, again and again; and when the
hill stood still, he was never a whit abashed, but said, If the hill will not
come to Mahomet, Mahomet will go to the hill.

Also, what goes around comes around.

Patch Set 5, the same as Patch Set 1, except that it works now because the
patches to ICU have now been updated and rolled into Chromium. Please take a
look.

[abarth-chromium, 2013-10-15 17:32:43]

Awesome!  Thanks for moving the mountain.  LGTM

[commit-bot: I haz the power, 2013-10-15 17:34:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bungeman@chromium.org/26358014/36001

[commit-bot: I haz the power, 2013-10-15 17:54:41]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[bungeman-chromium, 2013-10-15 18:08:34]

Committed patchset #5 manually as r159684 (presubmit successful).