Unix ifdefs patch for ui/ and webkit/

Made the OS_LINUX ifdefs in ui/ and webkit/ directories more cross-platform, so that these files can be used on multiple POSIX platforms.

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

[ruben, 2011-05-24 00:02:55]

Lei: webkit/ ifdefs
Robert: ui/ ifdefs
Tony: general feedback

[Lei Zhang, 2011-05-24 00:11:32]

http://codereview.chromium.org/6990051/diff/1/webkit/extensions/v8/profiler_e...
File webkit/extensions/v8/profiler_extension.cc (right):

http://codereview.chromium.org/6990051/diff/1/webkit/extensions/v8/profiler_e...
webkit/extensions/v8/profiler_extension.cc:17: #include
"third_party/tcmalloc/chromium/src/google/profiler.h"
does tcmalloc work on non-Linux Unices?

http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.cc
File webkit/glue/webkitclient_impl.cc (right):

http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.c...
webkit/glue/webkitclient_impl.cc:46: #if defined(OS_POSIX) &&
!defined(OS_MACOSX)
there's no reason to do this unless you also fix the #if at line 517.

http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.c...
webkit/glue/webkitclient_impl.cc:209: #if defined(OS_WIN) || defined(OS_POSIX)
ditto, the code won't even compile, so why bother with the #if and the entire
#else section.

http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.h
File webkit/glue/webkitclient_impl.h (right):

http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.h...
webkit/glue/webkitclient_impl.h:93: #if defined(OS_WIN) || defined(OS_POSIX)
You might as well remove the #if.

http://codereview.chromium.org/6990051/diff/1/webkit/plugins/ppapi/ppapi_plug...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/6990051/diff/1/webkit/plugins/ppapi/ppapi_plug...
webkit/plugins/ppapi/ppapi_plugin_instance.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Can you leave ppapi_plugin_instance.* alone for now? There's already another CL
in review that will change the behavior here.

[ruben, 2011-05-24 00:23:49]

On 2011/05/24 00:11:32, Lei Zhang wrote:
>
http://codereview.chromium.org/6990051/diff/1/webkit/extensions/v8/profiler_e...
> webkit/extensions/v8/profiler_extension.cc:17: #include
> "third_party/tcmalloc/chromium/src/google/profiler.h"
> does tcmalloc work on non-Linux Unices?
I've been using it on FreeBSD for the last year without a problem.  It compiles
on Solaris but wouldn't run, I haven't tried to debug that yet.
 
>
http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.c...
> webkit/glue/webkitclient_impl.cc:46: #if defined(OS_POSIX) &&
> !defined(OS_MACOSX)
> there's no reason to do this unless you also fix the #if at line 517.
You're right, fixed.

>
http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.c...
> webkit/glue/webkitclient_impl.cc:209: #if defined(OS_WIN) || defined(OS_POSIX)
> ditto, the code won't even compile, so why bother with the #if and the entire
> #else section.
Removed it, fixed.
 
>
http://codereview.chromium.org/6990051/diff/1/webkit/glue/webkitclient_impl.h...
> webkit/glue/webkitclient_impl.h:93: #if defined(OS_WIN) || defined(OS_POSIX)
> You might as well remove the #if.
Removed it, fixed.

>
http://codereview.chromium.org/6990051/diff/1/webkit/plugins/ppapi/ppapi_plug...
> webkit/plugins/ppapi/ppapi_plugin_instance.h:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> Can you leave ppapi_plugin_instance.* alone for now? There's already another
CL
> in review that will change the behavior here.
OK, removed them.

[Lei Zhang, 2011-05-24 00:37:25]

The other potential problem I see is that the changes to
webkit/plugins/ppapi/ppb_flash_impl.h and webkit/glue/webkitclient_impl.* will
not work, because they depend on files named foo_linux.cc, and those files don't
build on other unices due to the rule in build/common.gypi near line 822.

[ruben, 2011-05-24 00:43:57]

On 2011/05/24 00:37:25, Lei Zhang wrote:
> The other potential problem I see is that the changes to
> webkit/plugins/ppapi/ppb_flash_impl.h and webkit/glue/webkitclient_impl.* will
> not work, because they depend on files named foo_linux.cc, and those files
don't
> build on other unices due to the rule in build/common.gypi near line 822.
As of right now, you're right.  However, when I build on other Unix platforms, I
do compile the *_linux.* files by changing the line you mention in common.gypi. 
The problem is that the 50+ _linux source files need to also be split up into
_posix and _linux versions, but until that's done, most _linux files are
currently reused on other POSIX platforms.

[Lei Zhang, 2011-05-24 00:46:17]

Ok, great. LGTM on the webkit/ bits.

[Robert Sesek, 2011-05-24 16:24:12]

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.cc
File ui/gfx/image_unittest_util.cc (right):

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.c...
ui/gfx/image_unittest_util.cc:37: #elif defined(TOOLKIT_GTK)
USES?

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.c...
ui/gfx/image_unittest_util.cc:47: #elif defined(TOOLKIT_GTK)
USES?

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.h
File ui/gfx/image_unittest_util.h (right):

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.h...
ui/gfx/image_unittest_util.h:18: #elif defined(TOOLKIT_GTK)
TOOLKIT_USES_GTK?

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

http://codereview.chromium.org/6990051/diff/4001/ui/gfx/native_widget_types.h...
ui/gfx/native_widget_types.h:100: #elif defined(TOOLKIT_GTK)
USES?

[ruben, 2011-05-24 20:10:50]

On 2011/05/24 16:24:12, rsesek wrote:
>
http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.c...
> ui/gfx/image_unittest_util.cc:37: #elif defined(TOOLKIT_GTK)
> USES?
TOOLKIT_GTK appears to be the right define for these 4 places.  Look in
build_config.h: TOOLKIT_USES_GTK is used when Gtk code needs to be covered by
non-win TOOLKIT_VIEWS or TOOLKIT_GTK.  When TOOLKIT_VIEWS is not wanted, like in
these 4 places, that's when TOOLKIT_GTK can be used.

>
http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.c...
> ui/gfx/image_unittest_util.cc:47: #elif defined(TOOLKIT_GTK)
> USES? 
>
http://codereview.chromium.org/6990051/diff/4001/ui/gfx/image_unittest_util.h...
> ui/gfx/image_unittest_util.h:18: #elif defined(TOOLKIT_GTK)
> TOOLKIT_USES_GTK?
>
http://codereview.chromium.org/6990051/diff/4001/ui/gfx/native_widget_types.h...
> ui/gfx/native_widget_types.h:100: #elif defined(TOOLKIT_GTK)
> USES?

[Robert Sesek, 2011-05-25 12:39:06]

ui/ changes LGTM. Please send this through the linux_view trybot before landing.

Didn't know about the difference between USES and not. Thanks for clarifying.

[ruben, 2011-05-25 12:57:44]

On 2011/05/25 12:39:06, rsesek wrote:
> ui/ changes LGTM. Please send this through the linux_view trybot before
landing.
I'm not a committer, so I don't believe I have access to the trybots.  I can set
the commit flag here in rietveld and it will get sent to some commitbot, but I
don't know if that includes linux_views.

[tony, 2011-05-25 17:50:51]

On 2011/05/25 12:57:44, ruben wrote:
> On 2011/05/25 12:39:06, rsesek wrote:
> > ui/ changes LGTM. Please send this through the linux_view trybot before
> landing.
> I'm not a committer, so I don't believe I have access to the trybots.  I can
set
> the commit flag here in rietveld and it will get sent to some commitbot, but I
> don't know if that includes linux_views.

Running through the try bots now.

[commit-bot: I haz the power, 2011-05-25 20:43:25]

Try job failure for 6990051-4001 on win:
http://build.chromium.org/p/tryserver.chromium//buildstatus?builder=win&numbe...

[ruben, 2011-05-25 21:11:42]

On 2011/05/25 20:43:25, commit-bot wrote:
> Try job failure for 6990051-4001 on win:
>
http://build.chromium.org/p/tryserver.chromium//buildstatus?builder=win&numbe...
Looks like an unrelated bug in trunk with linux_view, because of the recent
commit 86688, that was fixed in 86691:

http://codereview.chromium.org/6992079

[commit-bot: I haz the power, 2011-05-25 23:45:00]

Change committed as 86742