Remove web intents from Chrome on Android build

Remove web intents from Chrome on Android build

Chrome on Android doesn't currently support web intents.  This change fixes linker errors that occuring when trying to build sync_unit_tests.

BUG=113487
TEST=


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

[groby-ooo-7-16, 2012-02-29 00:28:11]

lgtm (purely technical - jhawkins@ might want to chime in on wider implications)

[Greg Billock, 2012-02-29 00:47:22]

http://codereview.chromium.org/9521013/diff/2001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9521013/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4188: #if !defined(OS_ANDROID)
Why not just ensure that web_intents::IsWebIntentsEnabled is false for Android?

[Jesse Greenwald, 2012-02-29 17:59:41]

http://codereview.chromium.org/9521013/diff/2001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9521013/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4188: #if !defined(OS_ANDROID)
On 2012/02/29 00:47:22, Greg Billock wrote:
> Why not just ensure that web_intents::IsWebIntentsEnabled is false for
Android?

We're trying to keep our binary size as small as possible.  This particular
change may seem insignificant, but it adds up.  I actually forgot to exclude
'chrome/browser/intents' from the gyp file in this change which should be
excluded as well.

[Jesse Greenwald, 2012-02-29 21:40:59]

Excluded 'browser/intents' from Android build as well, PTAL.

[Greg Billock, 2012-03-01 00:17:02]

On 2012/02/29 21:40:59, jgreenwald1 wrote:
> Excluded 'browser/intents' from Android build as well, PTAL.

lgtm

[Jesse Greenwald, 2012-03-07 01:39:52]

jhawkins: Did you want to take a look at this before I commit it?  Thanks

[James Hawkins, 2012-03-08 05:21:22]

LGTM with nits.

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/conte...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/conte...
chrome/browser/content_settings/host_content_settings_map.cc:328: if
(!web_intents::IsWebIntentsEnabled()) {
nit: No braces here.

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/ui/br...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/ui/br...
chrome/browser/ui/browser.cc:2872: #endif
// defined(OS_ANDROID)

Here and elsewhere.

[Jesse Greenwald, 2012-03-08 18:35:28]

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/conte...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/conte...
chrome/browser/content_settings/host_content_settings_map.cc:328: if
(!web_intents::IsWebIntentsEnabled()) {
On 2012/03/08 05:21:22, James Hawkins wrote:
> nit: No braces here.

Done.

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/ui/br...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9521013/diff/6001/chrome/browser/ui/br...
chrome/browser/ui/browser.cc:2872: #endif
On 2012/03/08 05:21:22, James Hawkins wrote:
> // defined(OS_ANDROID)
> 
> Here and elsewhere.

Done.

[commit-bot: I haz the power, 2012-03-08 18:35:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/9521013/12001

[commit-bot: I haz the power, 2012-03-08 18:36:04]

Presubmit check for 9521013-12001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/browser/ui/tab_contents
    chrome/browser/profiles

Presubmit checks took 1.2s to calculate.

[Jesse Greenwald, 2012-03-08 19:23:44]

ben: Can you review:
  chrome/browser/ui
  chrome/browser/ui/tab_contents

mirandac: Can you review:
  chrome/browser/profiles

Thanks

[binji, 2012-03-08 19:26:26]

On 2012/03/08 19:23:44, jgreenwald1 wrote:
> ben: Can you review:
>   chrome/browser/ui
>   chrome/browser/ui/tab_contents
> 
> mirandac: Can you review:
>   chrome/browser/profiles
> 
> Thanks

+ben

[Miranda Callahan, 2012-03-08 19:28:43]

LGTM with comment addressed.

https://chromiumcodereview.appspot.com/9521013/diff/12001/chrome/browser/prof...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://chromiumcodereview.appspot.com/9521013/diff/12001/chrome/browser/prof...
chrome/browser/profiles/profile_dependency_manager.cc:196: #endif
I think this should be moved up to the other if !defined(OS_ANDROID) area above
(line 169); otherwise this will get increasingly messy.

[Ben Goodger (Google), 2012-03-08 23:32:38]

Have you guys considered adding USE_ defines for these sorts of things? e.g.
USE_INTENTS

and then define USE_INTENTS when !defined(OS_ANDROID).

https://chromiumcodereview.appspot.com/9521013/diff/12001/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9521013/diff/12001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:2856: void
Browser::RegisterIntentHandlerHelper(WebContents* tab,
Since this is a static function, does it really need to live on Browser? I'd
suggest moving it off into its own file browser/ui/intents.cc in the browser
namespace, and then not building that file on android. That way we can avoid
these ifdefs here.

[Jesse Greenwald, 2012-03-09 19:56:14]

On 2012/03/08 23:32:38, Ben Goodger (Google) wrote:
> Have you guys considered adding USE_ defines for these sorts of things? e.g.
> USE_INTENTS

That sounds like a good idea.  From looking through the code, I see that ENABLE_
looks more common.  Should I do a ENABLE_ flag instead?

[groby-ooo-7-16, 2012-03-09 20:16:20]

There's already an enable_web_intents for gyp files, which results in
ENABLE_WEB_INTENTS being set to 1.

On Fri, Mar 9, 2012 at 11:56 AM, <jgreenwald@chromium.org> wrote:

> On 2012/03/08 23:32:38, Ben Goodger (Google) wrote:
>
>> Have you guys considered adding USE_ defines for these sorts of things?
>> e.g.
>> USE_INTENTS
>>
>
> That sounds like a good idea.  From looking through the code, I see that
> ENABLE_
> looks more common.  Should I do a ENABLE_ flag instead?
>
>
http://codereview.chromium.**org/9521013/<http://codereview.chromium.org/9521...
>

[Yaron, 2012-03-09 20:47:11]

http://codereview.chromium.org/9521013/diff/12001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9521013/diff/12001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4798: ['exclude', '^browser/intents'],
Please also add these exclusions in chrome_tests.gypi

[Jesse Greenwald, 2012-03-10 00:13:26]

Resolved comments and nits.  I also ifdef'd out some unit tests that didn't link
when trying to build unit_tests.

PTAL

http://codereview.chromium.org/9521013/diff/12001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/9521013/diff/12001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_dependency_manager.cc:196: #endif
On 2012/03/08 19:28:43, Miranda Callahan wrote:
> I think this should be moved up to the other if !defined(OS_ANDROID) area
above
> (line 169); otherwise this will get increasingly messy. 

Since I'm now using ENABLED_WEB_INTENTS, I did not combine it with OS_ANDROID

http://codereview.chromium.org/9521013/diff/12001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9521013/diff/12001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:2856: void
Browser::RegisterIntentHandlerHelper(WebContents* tab,
On 2012/03/08 23:32:38, Ben Goodger (Google) wrote:
> Since this is a static function, does it really need to live on Browser? I'd
> suggest moving it off into its own file browser/ui/intents.cc in the browser
> namespace, and then not building that file on android. That way we can avoid
> these ifdefs here.

Done.

http://codereview.chromium.org/9521013/diff/12001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9521013/diff/12001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4798: ['exclude', '^browser/intents'],
On 2012/03/09 20:47:11, Yaron wrote:
> Please also add these exclusions in chrome_tests.gypi

Done.

[Yaron, 2012-03-10 00:28:01]

If you agree, the same goes for chrome/chrome_tests.gypi

http://codereview.chromium.org/9521013/diff/22001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9521013/diff/22001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4423: ['exclude', '^browser/intents'],
I think these new excludes should be split out to a separate conditional block
(not under android) for enable_web_intents==0.

Or alternatively, only include the sources for intents if enable_web_intents==1.

[Ben Goodger (Google), 2012-03-12 16:36:58]

lgtm

[commit-bot: I haz the power, 2012-03-12 18:26:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/9521013/25001

[commit-bot: I haz the power, 2012-03-12 18:50:55]

Try job failure for 9521013-25001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-03-12 21:23:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/9521013/29004

[commit-bot: I haz the power, 2012-03-12 22:23:55]

Try job failure for 9521013-29004 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-03-13 17:34:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jgreenwald@chromium.org/9521013/29004

[commit-bot: I haz the power, 2012-03-13 20:04:59]

Change committed as 126446