Enable breakpad building by default on Android.

Enable breakpad building by default on Android.

After https://chromiumcodereview.appspot.com/10407058 we can compile
breakpad by default and still have it disabled for non-official builds.

Changes the Android build to allow compiling breakpad but not use
it by not creating the crash fd and not passing it to the renderer
process unless breakpad is enabled.

Changes linux and Android to use a switch for enabling breakpad
since that's a lot easier to test with on android then an environment
variable.

BUG=105778, 170530
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178111

[Yaron, 2013-01-17 02:53:00]

You can hold off on review. I missed something.

[Mark Seaborn, 2013-01-17 03:26:01]

https://codereview.chromium.org/11969025/diff/7004/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/11969025/diff/7004/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:139: extern const char kEnableBreakpad[];
BTW, there is already kEnableCrashReporter.  Although that's only used
internally between browser and its subprocesses, it could be a little confusing
to have both "--enable-breakpad" and "--enable-crash-reporter".

Maybe rename kEnableCrashReporter to something like kEnableBreakpadInternal or
kEnableBreakpadInSubprocess?

[Yaron, 2013-01-18 02:29:37]

Ok, should be good for a look now. Thanks for the pointer on
kEnableCrashReporter. I opted to repurpose it for this use as it seems to fit
inline with what we're doing. Please let me know if you feel strongly that it
should be a separate flag.

[Lei Zhang, 2013-01-18 02:41:20]

I would prefer a separate android-only flag. It'll be much easier to search for
when we are looking for its use in the future.

https://codereview.chromium.org/11969025/diff/19001/build/android/pylib/host_...
File build/android/pylib/host_driven/tests_annotations.py (right):

https://codereview.chromium.org/11969025/diff/19001/build/android/pylib/host_...
build/android/pylib/host_driven/tests_annotations.py:76: def
EnormousTest(function):
Should this be part of the CL that actually add the tests?

[Mark Seaborn, 2013-01-18 03:56:12]

On 17 January 2013 18:41, <thestig@chromium.org> wrote:

> I would prefer a separate android-only flag. It'll be much easier to
> search for
> when we are looking for its use in the future.
>

How about we use the same command line flag for Android and non-Android
Linux?  Avoiding further divergence between Android and non-Android builds
seems preferable.  I only used an env var because, on Mac and Windows,
Breakpad is enabled via an env var, CHROME_HEADLESS=1.  It was also very
marginally easier to set an env var in
breakpad_crash_test/crash_dump_tester.py, but on reflection it would be
easy to pass through an "--enable-crash-reporter" option there instead.

Mark

[Lei Zhang, 2013-01-18 03:59:00]

On 2013/01/18 03:56:12, Mark Seaborn wrote:
> On 17 January 2013 18:41, <mailto:thestig@chromium.org> wrote:
> 
> > I would prefer a separate android-only flag. It'll be much easier to
> > search for
> > when we are looking for its use in the future.
> >
> 
> How about we use the same command line flag for Android and non-Android
> Linux?  Avoiding further divergence between Android and non-Android builds
> seems preferable.  I only used an env var because, on Mac and Windows,
> Breakpad is enabled via an env var, CHROME_HEADLESS=1.  It was also very
> marginally easier to set an env var in
> breakpad_crash_test/crash_dump_tester.py, but on reflection it would be
> easy to pass through an "--enable-crash-reporter" option there instead.
> 
> Mark

That's ok with me. kEnableCrashReporterForTesting ?

[Yaron, 2013-01-18 21:17:22]

On 2013/01/18 03:59:00, Lei Zhang wrote:
> On 2013/01/18 03:56:12, Mark Seaborn wrote:
> > On 17 January 2013 18:41, <mailto:thestig@chromium.org> wrote:
> > 
> > > I would prefer a separate android-only flag. It'll be much easier to
> > > search for
> > > when we are looking for its use in the future.
> > >
> > 
> > How about we use the same command line flag for Android and non-Android
> > Linux?  Avoiding further divergence between Android and non-Android builds
> > seems preferable.  I only used an env var because, on Mac and Windows,
> > Breakpad is enabled via an env var, CHROME_HEADLESS=1.  It was also very
> > marginally easier to set an env var in
> > breakpad_crash_test/crash_dump_tester.py, but on reflection it would be
> > easy to pass through an "--enable-crash-reporter" option there instead.
> > 
> > Mark
> 
> That's ok with me. kEnableCrashReporterForTesting ?

PTAL. I think I got the crash reporter tests updated (if you look at patch set
5, which only differs by the python annotation, it appears to run under the
"nacl_integration" step?)

[Lei Zhang, 2013-01-18 21:32:57]

lgtm, but wait for the other reviewers to chime in.

[Mark Seaborn, 2013-01-18 21:49:20]

LGTM with fixes below, plus change s/linus/Linux/ in commit message.

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:48: // Also allow crash reporting to
be enabled with a command-lne flag if the
"line"

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1844: if
(IsCrashReporterEnabled()) {
This change could use an explanation in the commit message

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1848: " be disabled for this
process.";
You have two spaces in the middle of this string

https://codereview.chromium.org/11969025/diff/30001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/11969025/diff/30001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1551: // Used for turning on crash reporting in
a debug environment where crash
Maybe say "Breakpad crash reporting"?  This makes it easier to find
Breakpad-related stuff by grepping.

[Yaron, 2013-01-22 18:49:07]

Thanks for reviews.

jcivelli: ptal

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_linux.cc:48: // Also allow crash reporting to
be enabled with a command-lne flag if the
On 2013/01/18 21:49:20, Mark Seaborn wrote:
> "line"

Done.

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1844: if
(IsCrashReporterEnabled()) {
On 2013/01/18 21:49:20, Mark Seaborn wrote:
> This change could use an explanation in the commit message

Done.

https://codereview.chromium.org/11969025/diff/30001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1848: " be disabled for this
process.";
On 2013/01/18 21:49:20, Mark Seaborn wrote:
> You have two spaces in the middle of this string

Done.

https://codereview.chromium.org/11969025/diff/30001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/11969025/diff/30001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1551: // Used for turning on crash reporting in
a debug environment where crash
On 2013/01/18 21:49:20, Mark Seaborn wrote:
> Maybe say "Breakpad crash reporting"?  This makes it easier to find
> Breakpad-related stuff by grepping.

Done.

[Jay Civelli, 2013-01-22 19:01:23]

lgtm

[commit-bot: I haz the power, 2013-01-22 19:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/11969025/33001

[commit-bot: I haz the power, 2013-01-22 21:53:45]

Change committed as 178111