Web Intents: Remove the flag and compile-time define.

Web Intents: Remove the flag and compile-time define.

BUG=none
TEST=none
R=gbillock

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137488

[commit-bot: I haz the power, 2012-05-16 19:58:55]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Greg Billock, 2012-05-16 19:59:56]

On 2012/05/16 18:37:04, James Hawkins wrote:

lgtm.

Copy jgreenwald to make sure Android is OK, but since the bot is green, it seems
it ought to be fine.

[commit-bot: I haz the power, 2012-05-16 20:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/10387155/1

[commit-bot: I haz the power, 2012-05-16 20:01:43]

Presubmit check for 10387155-1 failed and returned exit status 1.

Running presubmit commit checks ...

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

Presubmit checks took 1.6s to calculate.

[Jesse Greenwald, 2012-05-16 21:52:24]

This flag exists to fix linker errors on Android - we're trying get unit_tests
up and running.  This flag current exists to fix some linker errors. You're not
going to see a bot failure because we don't have a way of making sure "link
errors don't increase", but this will probably cause us problems.

[James Hawkins, 2012-05-16 21:55:15]

On 2012/05/16 21:52:24, jgreenwald1 wrote:
> This flag exists to fix linker errors on Android - we're trying get unit_tests
> up and running.  This flag current exists to fix some linker errors. You're
not
> going to see a bot failure because we don't have a way of making sure "link
> errors don't increase", but this will probably cause us problems.

Can you go into more detail?

[Jesse Greenwald, 2012-05-16 22:19:06]

On 2012/05/16 21:55:15, James Hawkins wrote:
> On 2012/05/16 21:52:24, jgreenwald1 wrote:
> > This flag exists to fix linker errors on Android - we're trying get
unit_tests
> > up and running.  This flag current exists to fix some linker errors. You're
> not
> > going to see a bot failure because we don't have a way of making sure "link
> > errors don't increase", but this will probably cause us problems.
> 
> Can you go into more detail?

Sure.  For Chrome on Android, we don't build everything that's normally built
for desktop Chrome.  This is because we either don't need it or because it makes
the app larger than we can afford.  Web intents is one of those things. 
Specifically, enable_web_intents is 0 for Chrome on Android, and if
enable_web_intents is 0 we exclude everything under browser/intents and
browser/ui/intents from the build (see chrome/chrome_browser.gypi).  The compile
time flags is to make sure we don't try to compile or link against anything
under those two directories.

One of our milestones in upstreaming is getting unit_tests to compile AND link,
and of course actually pass at some point in the future.  At this point, we only
have unit_tests compiling.  The Android bot checks to make sure unit_tests
continue to compile.  However, it doesn't try to link unit_tests.  This is
because there are other link errors from features that we're trying to resolve,
such as extensions.  So my point above was that even though the Android bot may
be green for this change it doesn't mean it's not going to cause us issues -
i.e. more link errors that we'll have to deal with.

[Yaron, 2012-05-17 15:46:38]

On 2012/05/16 22:19:06, jgreenwald1 wrote:
> On 2012/05/16 21:55:15, James Hawkins wrote:
> > On 2012/05/16 21:52:24, jgreenwald1 wrote:
> > > This flag exists to fix linker errors on Android - we're trying get
> unit_tests
> > > up and running.  This flag current exists to fix some linker errors.
You're
> > not
> > > going to see a bot failure because we don't have a way of making sure
"link
> > > errors don't increase", but this will probably cause us problems.
> > 
> > Can you go into more detail?
> 
> Sure.  For Chrome on Android, we don't build everything that's normally built
> for desktop Chrome.  This is because we either don't need it or because it
makes
> the app larger than we can afford.  Web intents is one of those things. 
> Specifically, enable_web_intents is 0 for Chrome on Android, and if
> enable_web_intents is 0 we exclude everything under browser/intents and
> browser/ui/intents from the build (see chrome/chrome_browser.gypi).  The
compile
> time flags is to make sure we don't try to compile or link against anything
> under those two directories.
> 
> One of our milestones in upstreaming is getting unit_tests to compile AND
link,
> and of course actually pass at some point in the future.  At this point, we
only
> have unit_tests compiling.  The Android bot checks to make sure unit_tests
> continue to compile.  However, it doesn't try to link unit_tests.  This is
> because there are other link errors from features that we're trying to
resolve,
> such as extensions.  So my point above was that even though the Android bot
may
> be green for this change it doesn't mean it's not going to cause us issues -
> i.e. more link errors that we'll have to deal with.

Sorry for not chiming in earlier but as Jesse pointed this is an issue for the
Android port. Can you please revert it and/or just remove the command-line flag.
This patch is also incomplete as is (or at least leaves around cruft in gyp):
http://code.google.com/p/chromium/source/search?q=enable_web_intents+file%3Ag...

[James Hawkins, 2012-05-17 16:10:12]

On 2012/05/17 15:46:38, Yaron wrote:
> On 2012/05/16 22:19:06, jgreenwald1 wrote:
> > On 2012/05/16 21:55:15, James Hawkins wrote:
> > > On 2012/05/16 21:52:24, jgreenwald1 wrote:
> > > > This flag exists to fix linker errors on Android - we're trying get
> > unit_tests
> > > > up and running.  This flag current exists to fix some linker errors.
> You're
> > > not
> > > > going to see a bot failure because we don't have a way of making sure
> "link
> > > > errors don't increase", but this will probably cause us problems.
> > > 
> > > Can you go into more detail?
> > 
> > Sure.  For Chrome on Android, we don't build everything that's normally
built
> > for desktop Chrome.  This is because we either don't need it or because it
> makes
> > the app larger than we can afford.  Web intents is one of those things. 
> > Specifically, enable_web_intents is 0 for Chrome on Android, and if
> > enable_web_intents is 0 we exclude everything under browser/intents and
> > browser/ui/intents from the build (see chrome/chrome_browser.gypi).  The
> compile
> > time flags is to make sure we don't try to compile or link against anything
> > under those two directories.
> > 
> > One of our milestones in upstreaming is getting unit_tests to compile AND
> link,
> > and of course actually pass at some point in the future.  At this point, we
> only
> > have unit_tests compiling.  The Android bot checks to make sure unit_tests
> > continue to compile.  However, it doesn't try to link unit_tests.  This is
> > because there are other link errors from features that we're trying to
> resolve,
> > such as extensions.  So my point above was that even though the Android bot
> may
> > be green for this change it doesn't mean it's not going to cause us issues -
> > i.e. more link errors that we'll have to deal with.
> 
> Sorry for not chiming in earlier but as Jesse pointed this is an issue for the
> Android port. Can you please revert it and/or just remove the command-line
flag.

No; the correct fix for Android is to provide a stub implementation of whichever
objects are in question (and to return false from IsEnabled()).

> This patch is also incomplete as is (or at least leaves around cruft in gyp):
>
http://code.google.com/p/chromium/source/search?q=enable_web_intents+file%253...

Your assessment is incorrect.  This define must live around indefinitely due to
WebKit; the implementation is behind this define in WebKit for other ports that
do not implement Intents.

[James Hawkins, 2012-05-17 16:13:16]

On 2012/05/17 16:10:12, James Hawkins wrote:
> On 2012/05/17 15:46:38, Yaron wrote:
> > On 2012/05/16 22:19:06, jgreenwald1 wrote:
> > > On 2012/05/16 21:55:15, James Hawkins wrote:
> > > > On 2012/05/16 21:52:24, jgreenwald1 wrote:
> > > > > This flag exists to fix linker errors on Android - we're trying get
> > > unit_tests
> > > > > up and running.  This flag current exists to fix some linker errors.
> > You're
> > > > not
> > > > > going to see a bot failure because we don't have a way of making sure
> > "link
> > > > > errors don't increase", but this will probably cause us problems.
> > > > 
> > > > Can you go into more detail?
> > > 
> > > Sure.  For Chrome on Android, we don't build everything that's normally
> built
> > > for desktop Chrome.  This is because we either don't need it or because it
> > makes
> > > the app larger than we can afford.  Web intents is one of those things. 
> > > Specifically, enable_web_intents is 0 for Chrome on Android, and if
> > > enable_web_intents is 0 we exclude everything under browser/intents and
> > > browser/ui/intents from the build (see chrome/chrome_browser.gypi).  The
> > compile
> > > time flags is to make sure we don't try to compile or link against
anything
> > > under those two directories.
> > > 
> > > One of our milestones in upstreaming is getting unit_tests to compile AND
> > link,
> > > and of course actually pass at some point in the future.  At this point,
we
> > only
> > > have unit_tests compiling.  The Android bot checks to make sure unit_tests
> > > continue to compile.  However, it doesn't try to link unit_tests.  This is
> > > because there are other link errors from features that we're trying to
> > resolve,
> > > such as extensions.  So my point above was that even though the Android
bot
> > may
> > > be green for this change it doesn't mean it's not going to cause us issues
-
> > > i.e. more link errors that we'll have to deal with.
> > 
> > Sorry for not chiming in earlier but as Jesse pointed this is an issue for
the
> > Android port. Can you please revert it and/or just remove the command-line
> flag.
> 
> No; the correct fix for Android is to provide a stub implementation of
whichever
> objects are in question (and to return false from IsEnabled()).
> 
> > This patch is also incomplete as is (or at least leaves around cruft in
gyp):
> >
>
http://code.google.com/p/chromium/source/search?q=enable_web_intents+file%252...
> 
> Your assessment is incorrect.  This define must live around indefinitely due
to
> WebKit; the implementation is behind this define in WebKit for other ports
that
> do not implement Intents.

Ah I see what you're saying.  Yes, I'll write up a patch to remove the chrome
file-level exclusions.

[Yaron, 2012-05-17 17:04:25]

On 2012/05/17 16:10:12, James Hawkins wrote:
> On 2012/05/17 15:46:38, Yaron wrote:
> > On 2012/05/16 22:19:06, jgreenwald1 wrote:
> > > On 2012/05/16 21:55:15, James Hawkins wrote:
> > > > On 2012/05/16 21:52:24, jgreenwald1 wrote:
> > > > > This flag exists to fix linker errors on Android - we're trying get
> > > unit_tests
> > > > > up and running.  This flag current exists to fix some linker errors.
> > You're
> > > > not
> > > > > going to see a bot failure because we don't have a way of making sure
> > "link
> > > > > errors don't increase", but this will probably cause us problems.
> > > > 
> > > > Can you go into more detail?
> > > 
> > > Sure.  For Chrome on Android, we don't build everything that's normally
> built
> > > for desktop Chrome.  This is because we either don't need it or because it
> > makes
> > > the app larger than we can afford.  Web intents is one of those things. 
> > > Specifically, enable_web_intents is 0 for Chrome on Android, and if
> > > enable_web_intents is 0 we exclude everything under browser/intents and
> > > browser/ui/intents from the build (see chrome/chrome_browser.gypi).  The
> > compile
> > > time flags is to make sure we don't try to compile or link against
anything
> > > under those two directories.
> > > 
> > > One of our milestones in upstreaming is getting unit_tests to compile AND
> > link,
> > > and of course actually pass at some point in the future.  At this point,
we
> > only
> > > have unit_tests compiling.  The Android bot checks to make sure unit_tests
> > > continue to compile.  However, it doesn't try to link unit_tests.  This is
> > > because there are other link errors from features that we're trying to
> > resolve,
> > > such as extensions.  So my point above was that even though the Android
bot
> > may
> > > be green for this change it doesn't mean it's not going to cause us issues
-
> > > i.e. more link errors that we'll have to deal with.
> > 
> > Sorry for not chiming in earlier but as Jesse pointed this is an issue for
the
> > Android port. Can you please revert it and/or just remove the command-line
> flag.
> 
> No; the correct fix for Android is to provide a stub implementation of
whichever
> objects are in question (and to return false from IsEnabled()).
> 

jam@ and ben@ specifically requested we use if-defs for feature-disablement as
opposed to stubs. In this case there aren't many if-defs so it seems like the
cleaner approach. On top of that, the size of the shared library has real
implications on Android so removing unneeded code leads to improvements in
start-up time.

> > This patch is also incomplete as is (or at least leaves around cruft in
gyp):
> >
>
http://code.google.com/p/chromium/source/search?q=enable_web_intents+file%252...
> 
> Your assessment is incorrect.  This define must live around indefinitely due
to
> WebKit; the implementation is behind this define in WebKit for other ports
that
> do not implement Intents.

[James Hawkins, 2012-05-17 17:18:21]

> 
> jam@ and ben@ specifically requested we use if-defs for feature-disablement as
> opposed to stubs. In this case there aren't many if-defs so it seems like the
> cleaner approach. On top of that, the size of the shared library has real
> implications on Android so removing unneeded code leads to improvements in
> start-up time.
> 

Thanks for the background, Yaron.  I started a thread between the four of us to
discuss the issue.