Fixing gcc 4.7 building problems.

Fixing gcc 4.7 building problems.

a) - gcc-4.7 improved the implicit headers that it includes. with
<4.7, the gthr-default.h file always pulls in unistd.h. with >=4.7,
they avoided that include when possible. so code that isn't including
unistd.h itself but needs it now breaks.

b) - narrowing conversion in initiliazation list now raises an
'ill-formed conversion' warning, which causes error when -Werror is
given.

[THIS PART IS NOW REVERTED IN THE PATCH} c) - included patches from pastebin - http://pastebin.com/raw.php?i=p3UKs7Cg

Note - this may not be fixing all the gcc 4.7 build problems for all
parts, but rather than submitting one big-fix-for-all CL, we'd better
do it incrementally (given that all the modification is reasonable and
minor) so that at least some parts get a successful gcc 4.7 build.

BUG=None
TEST=Built successfully using GCC-4.7 under chromium chroot


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140470

[Han, 2012-05-29 18:21:21]

Hi guys, mind taking a look at this CL? Thanks.

[brettw, 2012-05-29 18:23:19]

Please list who you would like to review what.

[agl, 2012-05-29 18:33:21]

crypto/, ipc/ LGTM

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_channel.h#n...
ipc/ipc_channel.h:10: #include <sys/types.h>
Wrap with #if defined(OS_POSIX)?

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_platform_fi...
File ipc/ipc_platform_file.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_platform_fi...
ipc/ipc_platform_file.cc:7: #if defined(OS_LINUX)
OS_POSIX again?

[Han, 2012-05-29 18:42:33]

Thanks, Adam.

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_channel.h#n...
ipc/ipc_channel.h:10: #include <sys/types.h>
On 2012/05/29 18:33:22, agl wrote:
> Wrap with #if defined(OS_POSIX)?

Done.

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_platform_fi...
File ipc/ipc_platform_file.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/2001/ipc/ipc_platform_fi...
ipc/ipc_platform_file.cc:7: #if defined(OS_LINUX)
On 2012/05/29 18:33:22, agl wrote:
> OS_POSIX again?

Done.

[Han, 2012-05-29 18:44:35]

On 2012/05/29 18:23:19, brettw wrote:
> Please list who you would like to review what.

Yeah, listed reviewers/parts below -

brettw - 
   base/...
   chrome/...
   content/

agl - 
  crypto/
  ipc/
  net/

jamesr - 
   webkit/

sky - 
   ui/
   third_party/tcmalloc/

yzshen
    - ppapi/

[brettw, 2012-05-29 19:30:45]

LGTM

https://chromiumcodereview.appspot.com/10451068/diff/9001/base/time_posix.cc
File base/time_posix.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/base/time_posix.cc#...
base/time_posix.cc:37: static_cast<long int>(microseconds *
Time::kNanosecondsPerMicrosecond)};
Can you just do "long" here (it's the same, right?)

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/process_proxy/process_output_watcher.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/process_proxy/process_output_watcher.cc:13: 
No blank line here (all C includes to together).

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_prepopulate_data.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/sear...
chrome/browser/search_engines/template_url_prepopulate_data.h:12: #include
<cstddef>
C++ includes should go together and be in order (so above <string>)

https://chromiumcodereview.appspot.com/10451068/diff/9001/crypto/ec_private_k...
File crypto/ec_private_key_nss.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/crypto/ec_private_k...
crypto/ec_private_key_nss.cc:131: static_cast<unsigned
int>(encrypted_private_key_info_len)
unsigned int -> unsigned (same elsewhere)

[jochen (gone - plz use gerrit), 2012-05-29 19:37:33]

Where does the pastebin diff come from, esp. who holds the copyright?
On May 29, 2012 9:30 PM, <brettw@chromium.org> wrote:

> LGTM
>
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
>
9001/base/time_posix.cc<https://chromiumcodereview.appspot.com/10451068/diff/9001/base/time_posix.cc>
> File base/time_posix.cc (right):
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
>
9001/base/time_posix.cc#**newcode37<https://chromiumcodereview.appspot.com/10451068/diff/9001/base/time_posix.cc#newcode37>
> base/time_posix.cc:37: static_cast<long int>(microseconds *
> Time::**kNanosecondsPerMicrosecond)};
> Can you just do "long" here (it's the same, right?)
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
>
9001/chrome/browser/chromeos/**process_proxy/process_output_**watcher.cc<https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/chromeos/process_proxy/process_output_watcher.cc>
> File chrome/browser/chromeos/**process_proxy/process_output_**watcher.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
> 9001/chrome/browser/chromeos/**process_proxy/process_output_**
>
watcher.cc#newcode13<https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/chromeos/process_proxy/process_output_watcher.cc#newcode13>
> chrome/browser/chromeos/**process_proxy/process_output_**watcher.cc:13:
> No blank line here (all C includes to together).
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
>
9001/chrome/browser/search_**engines/template_url_**prepopulate_data.h<https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/search_engines/template_url_prepopulate_data.h>
> File chrome/browser/search_engines/**template_url_prepopulate_data.**h
> (right):
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
> 9001/chrome/browser/search_**engines/template_url_**
>
prepopulate_data.h#newcode12<https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/search_engines/template_url_prepopulate_data.h#newcode12>
> chrome/browser/search_engines/**template_url_prepopulate_data.**h:12:
> #include <cstddef>
> C++ includes should go together and be in order (so above <string>)
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
>
9001/crypto/ec_private_key_**nss.cc<https://chromiumcodereview.appspot.com/10451068/diff/9001/crypto/ec_private_key_nss.cc>
> File crypto/ec_private_key_nss.cc (right):
>
> https://chromiumcodereview.**appspot.com/10451068/diff/**
>
9001/crypto/ec_private_key_**nss.cc#newcode131<https://chromiumcodereview.appspot.com/10451068/diff/9001/crypto/ec_private_key_nss.cc#newcode131>
> crypto/ec_private_key_nss.cc:**131: static_cast<unsigned
> int>(encrypted_private_key_**info_len)
> unsigned int -> unsigned (same elsewhere)
>
>
https://chromiumcodereview.**appspot.com/10451068/<https://chromiumcodereview...
>

[Han, 2012-05-29 20:57:04]

Hi Brett, thanks. Done.

https://chromiumcodereview.appspot.com/10451068/diff/9001/base/time_posix.cc
File base/time_posix.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/base/time_posix.cc#...
base/time_posix.cc:37: static_cast<long int>(microseconds *
Time::kNanosecondsPerMicrosecond)};
On 2012/05/29 19:30:46, brettw wrote:
> Can you just do "long" here (it's the same, right?)

Yes, 'long' is just an alias for 'long int'.

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/process_proxy/process_output_watcher.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/process_proxy/process_output_watcher.cc:13: 
On 2012/05/29 19:30:46, brettw wrote:
> No blank line here (all C includes to together).

Done.

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_prepopulate_data.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/chrome/browser/sear...
chrome/browser/search_engines/template_url_prepopulate_data.h:12: #include
<cstddef>
On 2012/05/29 19:30:46, brettw wrote:
> C++ includes should go together and be in order (so above <string>)

Done.

https://chromiumcodereview.appspot.com/10451068/diff/9001/crypto/ec_private_k...
File crypto/ec_private_key_nss.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/9001/crypto/ec_private_k...
crypto/ec_private_key_nss.cc:131: static_cast<unsigned
int>(encrypted_private_key_info_len)
On 2012/05/29 19:30:46, brettw wrote:
> unsigned int -> unsigned (same elsewhere)

Done.

[yzshen1, 2012-05-29 20:59:49]

ppapi/ LGTM
Thanks!

[Han, 2012-05-29 21:01:53]

On 2012/05/29 19:37:33, jochen wrote:
> Where does the pastebin diff come from, esp. who holds the copyright?
> On May 29, 2012 9:30 PM, <mailto:brettw@chromium.org> wrote:
> 
Hi John, I think these are reasonable/simple/straightforward fixes.

But Andres could you answer John's the question?

Thanks,
-Han

[Mattias Nissler (ping if slow), 2012-05-30 08:02:23]

LGTM with a nit.

https://chromiumcodereview.appspot.com/10451068/diff/7002/chrome/browser/poli...
File chrome/browser/policy/policy_path_parser_linux.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/7002/chrome/browser/poli...
chrome/browser/policy/policy_path_parser_linux.cc:6: #include <unistd.h>
I guess this one is for geteuid? While you're at it, can you also add the
missing sys/types.h (for geteuid and getpwuid)?

[Andreas Reis, 2012-05-30 13:33:42]

On 2012/05/29 21:01:53, Han wrote:
> On 2012/05/29 19:37:33, jochen wrote:
> > Where does the pastebin diff come from, esp. who holds the copyright?
> > On May 29, 2012 9:30 PM, <mailto:brettw@chromium.org> wrote:
> > 
> Hi John, I think these are reasonable/simple/straightforward fixes.
> 
> But Andres could you answer John's the question?
> 
> Thanks,
> -Han

The <cstddef>s came from the Arch Linux build script, AFAIK introduced by Pierre
Schmitz here:
https://projects.archlinux.org/svntogit/packages.git/commit/trunk?h=packages/...

The other parts of the pastebin diff were added by me. Since they are trivial I
doubt that they warrant a copyright.

(sandbox_init.cc might be better of with "(OS_POSIX)" instead of "(OS_ANDROID)
|| defined(OS_LINUX)", like ipc_platform_file.cc, esp. since both files closely
resemble each other.)

[jochen (gone - plz use gerrit), 2012-05-30 13:42:56]

On Wed, May 30, 2012 at 3:33 PM, <andreas.reis@gmail.com> wrote:

> On 2012/05/29 21:01:53, Han wrote:
>
>> On 2012/05/29 19:37:33, jochen wrote:
>> > Where does the pastebin diff come from, esp. who holds the copyright?
>> > On May 29, 2012 9:30 PM, <mailto:brettw@chromium.org> wrote:
>> >
>> Hi John, I think these are reasonable/simple/**straightforward fixes.
>>
>
>  But Andres could you answer John's the question?
>>
>
>  Thanks,
>> -Han
>>
>
> The <cstddef>s came from the Arch Linux build script, AFAIK introduced by
> Pierre
> Schmitz here:
> https://projects.archlinux.**org/svntogit/packages.git/**
> commit/trunk?h=packages/**chromium&id=**b18a2979663cbb653fac9e6e5c04e8**
>
ef7631bc68<https://projects.archlinux.org/svntogit/packages.git/commit/trunk?h=packages/chromium&id=b18a2979663cbb653fac9e6e5c04e8ef7631bc68>
>
> The other parts of the pastebin diff were added by me. Since they are
> trivial I
> doubt that they warrant a copyright.
>

I don't know from what point on the rules apply, but I can't find either of
you in the CLA signers list

Adding mal for clarification


>
> (sandbox_init.cc might be better of with "(OS_POSIX)" instead of
> "(OS_ANDROID)
> || defined(OS_LINUX)", like ipc_platform_file.cc, esp. since both files
> closely
> resemble each other.)
>
>
https://chromiumcodereview.**appspot.com/10451068/<https://chromiumcodereview...
>

[Han, 2012-05-30 16:09:27]

Mattias, thanks, done!

https://chromiumcodereview.appspot.com/10451068/diff/7002/chrome/browser/poli...
File chrome/browser/policy/policy_path_parser_linux.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/7002/chrome/browser/poli...
chrome/browser/policy/policy_path_parser_linux.cc:6: #include <unistd.h>
On 2012/05/30 08:02:25, Mattias Nissler wrote:
> I guess this one is for geteuid? While you're at it, can you also add the
> missing sys/types.h (for geteuid and getpwuid)?

Yeah, right, both the headers are required for geteuid.
Done.

[Han, 2012-05-31 17:06:45]

On 2012/05/29 18:44:35, Han wrote:
> On 2012/05/29 18:23:19, brettw wrote:
> > Please list who you would like to review what.
> 
> Yeah, listed reviewers/parts below -
> 
> brettw - 
>    base/...
>    chrome/...
>    content/
> 
> agl - 
>   crypto/
>   ipc/
>   net/
> 
> jamesr - 
>    webkit/
> 
> sky - 
>    ui/
>    third_party/tcmalloc/
> 
> yzshen
>     - ppapi/

Hi James, could you take a look at "webkit/"? Thanks!

[jamesr, 2012-05-31 17:11:28]

The CLA situation should be sorted out before spending time reviewing.

[Han, 2012-05-31 17:14:40]

On 2012/05/31 17:11:28, jamesr wrote:
> The CLA situation should be sorted out before spending time reviewing.

Hi James, the modifications under "webkit/" is solely contributed by me, it is
not part of "pastebin". I'll revert modifications from "pastebin" if CLA is not
sorted out by tomorrow. Thanks,

-Han

[jamesr, 2012-05-31 17:33:40]

https://chromiumcodereview.appspot.com/10451068/diff/2004/webkit/glue/webcurs...
File webkit/glue/webcursor_gtk_data.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/2004/webkit/glue/webcurs...
webkit/glue/webcursor_gtk_data.h:35: #pragma GCC diagnostic ignored
"-Wnarrowing"
This seems very weird to me - we normally control compilation flags from the
build system, not inline in the code.  I can't find any other instances of this
sort of thing today - why can't you set this in the gypi?

[mal, 2012-05-31 18:24:20]

I would strongly prefer that the original authors of the code sign the CLA to
make it clear that they're giving the Chromium Authors rights to their creative
work. Our rights to use someone else's work don't depend on
complexity/triviality.

[Han, 2012-05-31 18:25:38]

On 2012/05/31 18:24:20, mal wrote:
> I would strongly prefer that the original authors of the code sign the CLA to
> make it clear that they're giving the Chromium Authors rights to their
creative
> work. Our rights to use someone else's work don't depend on
> complexity/triviality.

Hi Mark, thanks! I'll revert modifications from pastebin.

-Han

[mal, 2012-05-31 18:27:19]

We should at least get a statement from the authors that the changes are ok for
us to use if the CLA isn't signed.

[Andreas Reis, 2012-05-31 18:35:05]

On 2012/05/31 18:27:19, mal wrote:
> We should at least get a statement from the authors that the changes are ok
for
> us to use if the CLA isn't signed.

As for my parts, use them as you like; I give you any rights whatsoever
necessary…

Can't speak for Pierre Schmitz, of course.

[Han, 2012-05-31 20:32:23]

On 2012/05/31 17:33:40, jamesr wrote:
>
https://chromiumcodereview.appspot.com/10451068/diff/2004/webkit/glue/webcurs...
> File webkit/glue/webcursor_gtk_data.h (right):
> 
>
https://chromiumcodereview.appspot.com/10451068/diff/2004/webkit/glue/webcurs...
> webkit/glue/webcursor_gtk_data.h:35: #pragma GCC diagnostic ignored
> "-Wnarrowing"
> This seems very weird to me - we normally control compilation flags from the
> build system, not inline in the code.  I can't find any other instances of
this
> sort of thing today - why can't you set this in the gypi?

Hi James thanks! Moved the flag change to webkit_glue.gypi.

-Han

[jamesr, 2012-05-31 20:34:18]

webkit/ LGTM

https://chromiumcodereview.appspot.com/10451068/diff/5004/webkit/glue/webcurs...
File webkit/glue/webcursor_gtk_data.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/5004/webkit/glue/webcurs...
webkit/glue/webcursor_gtk_data.h:312: 
spurious newline here

[Han, 2012-05-31 20:36:50]

On 2012/05/31 18:35:05, Andreas Reis wrote:
> On 2012/05/31 18:27:19, mal wrote:
> > We should at least get a statement from the authors that the changes are ok
> for
> > us to use if the CLA isn't signed.
> 
> As for my parts, use them as you like; I give you any rights whatsoever
> necessary…
> 
> Can't speak for Pierre Schmitz, of course.

Also note, I've reverted the changes from pastebin.

Hi Andreas, no worry, I'll upload another bunch of changes regarding gcc 4.7
changes, you can send me YOUR patches. Of course you can upload it for review by
yourself, it's easy.

Thanks,
-Han

[Han, 2012-05-31 20:41:45]

https://chromiumcodereview.appspot.com/10451068/diff/5004/webkit/glue/webcurs...
File webkit/glue/webcursor_gtk_data.h (right):

https://chromiumcodereview.appspot.com/10451068/diff/5004/webkit/glue/webcurs...
webkit/glue/webcursor_gtk_data.h:312: 
On 2012/05/31 20:34:19, jamesr wrote:
> spurious newline here

Done.

[sky, 2012-06-02 22:41:20]

https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
File ui/gfx/skia_utils_gtk.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
ui/gfx/skia_utils_gtk.cc:25: static_cast<guint16>(SkColorGetR(color) *
kSkiaToGDKMultiplier),
http://developer.gnome.org/gdk/stable/gdk-Colormaps-and-Colors.html says the
range is 0 and 65535 ?

[Han, 2012-06-04 20:10:28]

Thanks, Scott!

https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
File ui/gfx/skia_utils_gtk.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
ui/gfx/skia_utils_gtk.cc:25: static_cast<guint16>(SkColorGetR(color) *
kSkiaToGDKMultiplier),
On 2012/06/02 22:41:20, sky wrote:
> http://developer.gnome.org/gdk/stable/gdk-Colormaps-and-Colors.html says the
> range is 0 and 65535 ?
Right, the 2nd, 3rd, 4th field of GdkColor is guint16, ranged 0-65535.

[sky, 2012-06-04 21:13:27]

LGTM

https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
File ui/gfx/skia_utils_gtk.cc (right):

https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
ui/gfx/skia_utils_gtk.cc:25: static_cast<guint16>(SkColorGetR(color) *
kSkiaToGDKMultiplier),
On 2012/06/04 20:10:29, Han wrote:
> On 2012/06/02 22:41:20, sky wrote:
> > http://developer.gnome.org/gdk/stable/gdk-Colormaps-and-Colors.html says the
> > range is 0 and 65535 ?
> Right, the 2nd, 3rd, 4th field of GdkColor is guint16, ranged 0-65535.

My mistake. For some reason I was thinking SkColorGetXXX returned 0-1, but it
returns 256 so that this returns 0-65535.

[Han, 2012-06-04 21:46:38]

On 2012/06/04 21:13:27, sky wrote:
> LGTM
> 
>
https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
> File ui/gfx/skia_utils_gtk.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10451068/diff/2005/ui/gfx/skia_utils_g...
> ui/gfx/skia_utils_gtk.cc:25: static_cast<guint16>(SkColorGetR(color) *
> kSkiaToGDKMultiplier),
> On 2012/06/04 20:10:29, Han wrote:
> > On 2012/06/02 22:41:20, sky wrote:
> > > http://developer.gnome.org/gdk/stable/gdk-Colormaps-and-Colors.html says
the
> > > range is 0 and 65535 ?
> > Right, the 2nd, 3rd, 4th field of GdkColor is guint16, ranged 0-65535.
> 
> My mistake. For some reason I was thinking SkColorGetXXX returned 0-1, but it
> returns 256 so that this returns 0-65535.

Hi all the files LGTMed, can any one help land the CL? Thanks!

[Avi (use Gerrit), 2012-06-04 21:49:03]

That's up to you. Click the "Commit: [ ]" checkbox under your patch to add it to
the commit queue.

[commit-bot: I haz the power, 2012-06-04 21:54:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shenhan@google.com/10451068/2005

[commit-bot: I haz the power, 2012-06-04 22:12:18]

Try job failure for 10451068-2005 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-05 00:03:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shenhan@google.com/10451068/28002

[commit-bot: I haz the power, 2012-06-05 01:54:56]

Change committed as 140470