chromeos: Fix shared linking.

chromeos: Fix shared linking.

BUG=make chrome fails with GYP_DEFINES='chromeos=1 library=shared_library'
TEST=see BUG

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

[sadrul, 2010-12-09 04:45:52]

Looks like this was caused by http://codereview.chromium.org/5368002

[The wrong rickcam account, 2010-12-09 16:04:06]

On 2010/12/09 04:45:52, sadrul wrote:
> Looks like this was caused by http://codereview.chromium.org/5368002

What is the failure?  Are you hitting the NOTREACHED?  If so, I think it would
reasonable to remove it.  If it's a linking problem, this could just be a
reflection of my lack of understanding about how the platform-specific gyp magic
works.  If that's the case, I'd be happier to correctly establish the distinct
Linux and ChromeOS definitions.

Beyond that, Launch On Startup is inherent to ChromeOS and I think that the work
done in the Linux implementation may not be appropriate.  I don't think it hurts
anything per se, but it does a bunch of task posting that isn't necessary.

[The wrong rickcam account, 2010-12-09 16:04:07]

On 2010/12/09 04:45:52, sadrul wrote:
> Looks like this was caused by http://codereview.chromium.org/5368002

What is the failure?  Are you hitting the NOTREACHED?  If so, I think it would
reasonable to remove it.  If it's a linking problem, this could just be a
reflection of my lack of understanding about how the platform-specific gyp magic
works.  If that's the case, I'd be happier to correctly establish the distinct
Linux and ChromeOS definitions.

Beyond that, Launch On Startup is inherent to ChromeOS and I think that the work
done in the Linux implementation may not be appropriate.  I don't think it hurts
anything per se, but it does a bunch of task posting that isn't necessary.

[sadrul, 2010-12-09 16:20:33]

On 2010/12/09 16:04:07, Rick Campbell wrote:
> On 2010/12/09 04:45:52, sadrul wrote:
> > Looks like this was caused by http://codereview.chromium.org/5368002
> 
> What is the failure?  Are you hitting the NOTREACHED?  If so, I think it would
> reasonable to remove it.  If it's a linking problem, this could just be a
> reflection of my lack of understanding about how the platform-specific gyp
magic
> works.  If that's the case, I'd be happier to correctly establish the distinct
> Linux and ChromeOS definitions.

The linking error:

  ...
  SOLINK(target) out/Debug/obj.target/chrome/libbrowser.so
out/Debug/obj.target/browser/chrome/browser/background_mode_manager_linux.o: In
function `BackgroundModeManager::EnableLaunchOnStartup(bool)':
/home/sadrul/devel/c/src/chrome/browser/background_mode_manager_linux.cc:56:
multiple definition of `BackgroundModeManager::EnableLaunchOnStartup(bool)'
out/Debug/obj.target/browser/chrome/browser/background_mode_manager_chromeos.o:/home/sadrul/devel/c/src/./base/logging.h:728:
first defined here
out/Debug/obj.target/browser/chrome/browser/background_mode_manager_linux.o: In
function `BackgroundModeManager::GetPreferencesMenuLabel()':
/home/sadrul/devel/c/src/chrome/browser/background_mode_manager_linux.cc:111:
multiple definition of `BackgroundModeManager::GetPreferencesMenuLabel()'
out/Debug/obj.target/browser/chrome/browser/background_mode_manager_chromeos.o:/home/sadrul/devel/c/src/chrome/browser/background_mode_manager_chromeos.cc:14:
first defined here
collect2: ld returned 1 exit status
make: *** [out/Debug/obj.target/chrome/libbrowser.so] Error 1

When building chromeos, both OS_LINUX and OS_CHROMEOS are defined. As a result,
 (1) When functions have different implementations for chromeos, the same
function for linux is usually wrapped inside a !defined(OS_CHROMEOS) (see
views/controls/menu/native_menu_gtk.cc:MenuWrapper::CreateWrapper, for example).
and
 (2) The statements inside OS_LINUX are compiled for chromeos too, unless the
file is explicitly excluded in the gyp files. In this particular case, it looks
like the same bits used in linux can also be used for chromeos, with the
exception of the preference menu label.

[The wrong rickcam account, 2010-12-09 17:33:33]

On 2010/12/09 16:20:33, sadrul wrote:
> On 2010/12/09 16:04:07, Rick Campbell wrote:
> > On 2010/12/09 04:45:52, sadrul wrote:
> > > Looks like this was caused by http://codereview.chromium.org/5368002
> > 
> > What is the failure?  Are you hitting the NOTREACHED?  If so, I think it
would
> > reasonable to remove it.  If it's a linking problem, this could just be a
> > reflection of my lack of understanding about how the platform-specific gyp
> magic
> > works.  If that's the case, I'd be happier to correctly establish the
distinct
> > Linux and ChromeOS definitions.
> 
> The linking error:
> 
>   ...
>   SOLINK(target) out/Debug/obj.target/chrome/libbrowser.so
> out/Debug/obj.target/browser/chrome/browser/background_mode_manager_linux.o:
In
> function `BackgroundModeManager::EnableLaunchOnStartup(bool)':
> /home/sadrul/devel/c/src/chrome/browser/background_mode_manager_linux.cc:56:
> multiple definition of `BackgroundModeManager::EnableLaunchOnStartup(bool)'
>
out/Debug/obj.target/browser/chrome/browser/background_mode_manager_chromeos.o:/home/sadrul/devel/c/src/./base/logging.h:728:
> first defined here
> out/Debug/obj.target/browser/chrome/browser/background_mode_manager_linux.o:
In
> function `BackgroundModeManager::GetPreferencesMenuLabel()':
> /home/sadrul/devel/c/src/chrome/browser/background_mode_manager_linux.cc:111:
> multiple definition of `BackgroundModeManager::GetPreferencesMenuLabel()'
>
out/Debug/obj.target/browser/chrome/browser/background_mode_manager_chromeos.o:/home/sadrul/devel/c/src/chrome/browser/background_mode_manager_chromeos.cc:14:
> first defined here
> collect2: ld returned 1 exit status
> make: *** [out/Debug/obj.target/chrome/libbrowser.so] Error 1
> 
> When building chromeos, both OS_LINUX and OS_CHROMEOS are defined. As a
result,
>  (1) When functions have different implementations for chromeos, the same
> function for linux is usually wrapped inside a !defined(OS_CHROMEOS) (see
> views/controls/menu/native_menu_gtk.cc:MenuWrapper::CreateWrapper, for
example).
> and
>  (2) The statements inside OS_LINUX are compiled for chromeos too, unless the
> file is explicitly excluded in the gyp files. In this particular case, it
looks
> like the same bits used in linux can also be used for chromeos, with the
> exception of the preference menu label.

Got it.  Thank you for the explanation.

My recommendation is to use the same approach to EnableLaunchOnStartup as you
did with GetPreferencesMenuLabel in background_mode_manager_linux.cc, and to
leave background_mode_manager_chromeos.cc unmodified.

I believe that this will resolve the link issue and implements the logic that
Drew and I intended for ChromeOS, where be believe that EnableLaunchOnStartup
should not be invoked.

[The wrong rickcam account, 2010-12-09 17:34:27]

http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
File chrome/browser/background_mode_manager_chromeos.cc (right):

http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
chrome/browser/background_mode_manager_chromeos.cc:8: #include
"grit/generated_resources.h"
I recommend dropping this change in favor of the #ifndef that I recommend in my
comment on background_mode_manager_linux.cc

http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
File chrome/browser/background_mode_manager_linux.cc (right):

http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
chrome/browser/background_mode_manager_linux.cc:55: 
I would recommend this as the point to us #ifndef in the same manner as is done
for BackgroundModeManager::GetPreferencesMenuLabel

[The wrong rickcam account, 2010-12-09 17:39:11]

On 2010/12/09 17:34:27, Rick Campbell wrote:
>
http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
> File chrome/browser/background_mode_manager_chromeos.cc (right):
> 
>
http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
> chrome/browser/background_mode_manager_chromeos.cc:8: #include
> "grit/generated_resources.h"
> I recommend dropping this change in favor of the #ifndef that I recommend in
my
> comment on background_mode_manager_linux.cc
> 
>
http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
> File chrome/browser/background_mode_manager_linux.cc (right):
> 
>
http://codereview.chromium.org/5690001/diff/1/chrome/browser/background_mode_...
> chrome/browser/background_mode_manager_linux.cc:55: 
> I would recommend this as the point to us #ifndef in the same manner as is
done
> for BackgroundModeManager::GetPreferencesMenuLabel

Sorry for the multiple updates and botched wording.  That last comment should
read:
I would recommend using #if !defined(OS_CHROMEOS) in the same manner as you have
done for BackgroundModeManager::GetPreferencesMenuLabel

[Andrew T Wilson (Slow), 2010-12-09 18:09:24]

Agreed with both of Rick's comments. In fact, I actually think this is best
addressed through gyp and not #defines.

[Andrew T Wilson (Slow), 2010-12-09 18:11:49]

On 2010/12/09 18:09:24, Andrew T Wilson wrote:
> Agreed with both of Rick's comments. In fact, I actually think this is best
> addressed through gyp and not #defines.

To clarify, I think the right fix is to add background_mode_manager_linux.cc to
the exclusion list in chrome_browser.gypi:


 ['OS=="linux" and chromeos==1',{
              'sources/': [
                ['exclude', '^browser/extensions/extension_tts_api_linux.cc'],
                ['exclude', '^browser/geolocation/wifi_data_provider_linux.cc'],
                ['exclude', '^browser/geolocation/wifi_data_provider_linux.h'],
                ['exclude', '^browser/notifications/balloon_collection.cc'],
                ['exclude', '^browser/notifications/balloon_collection_impl.h'],
                ['exclude',
'^browser/notifications/balloon_collection_linux.cc'],

[sadrul, 2010-12-09 18:22:14]

On 2010/12/09 18:11:49, Andrew T Wilson wrote:
> On 2010/12/09 18:09:24, Andrew T Wilson wrote:
> > Agreed with both of Rick's comments. In fact, I actually think this is best
> > addressed through gyp and not #defines.
> 
> To clarify, I think the right fix is to add background_mode_manager_linux.cc
to
> the exclusion list in chrome_browser.gypi:
> 
> 
>  ['OS=="linux" and chromeos==1',{
>               'sources/': [
>                 ['exclude', '^browser/extensions/extension_tts_api_linux.cc'],
>                 ['exclude',
'^browser/geolocation/wifi_data_provider_linux.cc'],
>                 ['exclude',
'^browser/geolocation/wifi_data_provider_linux.h'],
>                 ['exclude', '^browser/notifications/balloon_collection.cc'],
>                 ['exclude',
'^browser/notifications/balloon_collection_impl.h'],
>                 ['exclude',
> '^browser/notifications/balloon_collection_linux.cc'],

Indeed. I add the file in the exclusion list in the gpy file (in a different
block). PTAL.

[sadrul, 2010-12-09 18:24:10]

On 2010/12/09 18:11:49, Andrew T Wilson wrote:
> On 2010/12/09 18:09:24, Andrew T Wilson wrote:
> > Agreed with both of Rick's comments. In fact, I actually think this is best
> > addressed through gyp and not #defines.

The NOTIMPLEMENTED in the code lead me to think that the code under OS_LINUX
perhaps was unintentionally removed from chromeos. Since that's not the case, I
agree that updating the gyp file is the correct solution.

[Andrew T Wilson (Slow), 2010-12-09 18:33:09]

LGTM - I'm not entirely clear on which of the various ways of excluding files
from chromeos is the preferred one, but this looks like it should work.

[sadrul, 2010-12-09 19:21:45]

On 2010/12/09 18:33:09, Andrew T Wilson wrote:
> LGTM - I'm not entirely clear on which of the various ways of excluding files
> from chromeos is the preferred one, but this looks like it should work.

Thanks. I have checked it in.

The ordering in the gyp file is not very clear to me either. I usually just add
the conditional include/excludes in the same level, i.e. since
background_mode_manager_linux.cc is included in 'browser':'sources', the
conditional exclusion goes in 'browser':'conditions':'chromeos==1'. But I have a
suspicion there isn't a strongly defined rule for this.