Upstream native crash handling changes for Android.

Upstream native crash handling changes for Android.

Android native crash handling is almost identical to linux handling with
some differences.

Note that even after this change Chrome on Android will not compile with
the USE_LINUX_BREAKPAD flag. Forthcomming changes in breakpad should
remedy this state of affairs.

BUG=
TEST=


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

[Yaron, 2012-03-26 18:51:15]

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info.h
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info...
base/android/build_info.h:15: // BuildInfo is a singleton class that stores
android build and device
Seems like you could be using base/memory/singleton.h. The comments in there
explain how to use it.

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
File chrome/app/breakpad_posix.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
chrome/app/breakpad_posix.cc:44: #include
"seccompsandbox/linux_syscall_support.h"
Remove this include if it shouldn't be in the android build

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
chrome/app/breakpad_posix.cc:46: #ifdef OS_ANDROID
Nit: preferred style is #if defined(OS_ANDROID)

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
chrome/app/breakpad_posix.cc:700: __android_log_write(ANDROID_LOG_WARN,
"chromium", output_msg);
Why are these printed with "chromium" whereas the others are with
google-breakpad? It's just the activity mentioned in logcat, right? Seems like
we should be consistent. Also, can this juse the log function from above?

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
File chrome/app/breakpad_posix.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
chrome/app/breakpad_posix.cc:341: size_t log(const char* buf, size_t nbytes) {
Woah. Why is log defined twice? Also, you're already defining this in breakpad's
client/linux/log/log.h. Can't you re-use it?

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
chrome/app/breakpad_posix.cc:965: new
google_breakpad::ExceptionHandler(dumps_path.value().c_str(), NULL,
Seems like you could still assign to g_breakpad and maintain the
DCHECK(!g_breakpad);

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
chrome/app/breakpad_posix.cc:1124: return;
Do we always early-return? I thought not, in which case won't your SetClientId
be clobbered by the conditionals below?

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
File chrome/browser/crash_handler_host_posix.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix.cc:5: #include
"chrome/browser/crash_handler_host_linux.h"
Fix include

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
File chrome/browser/crash_handler_host_posix.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix.h:5: #ifndef
CHROME_BROWSER_CRASH_HANDLER_HOST_LINUX_H_
Update these

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
File chrome/browser/crash_handler_host_posix_stub.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix_stub.cc:8: #include
"chrome/browser/crash_handler_host_linux.h"
Fix include

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix_stub.cc:12:
CrashHandlerHostLinux::CrashHandlerHostLinux()
If you are changing these to be in a _posix file, you should rename all the
types accordingly (i.e. this should be CrashHandlerHosPosix)

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/chrome_browse...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/chrome_browse...
chrome/chrome_browser.gypi:4321: ['linux_breakpad==1', {
This should probably also be renamed.

[carlosvaldivia, 2012-03-26 22:37:01]

Thanks. I'll get to this after the merge.

On Mon, Mar 26, 2012 at 11:51 AM, <yfriedman@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9838033/diff/1/**
>
base/android/build_info.h<https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info.h>
> File base/android/build_info.h (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/1/**
>
base/android/build_info.h#**newcode15<https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info.h#newcode15>
> base/android/build_info.h:15: // BuildInfo is a singleton class that
> stores android build and device
> Seems like you could be using base/memory/singleton.h. The comments in
> there explain how to use it.
>
> https://chromiumcodereview.**appspot.com/9838033/diff/1/**
>
chrome/app/breakpad_posix.cc<https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_posix.cc>
> File chrome/app/breakpad_posix.cc (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/1/**
>
chrome/app/breakpad_posix.cc#**newcode44<https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_posix.cc#newcode44>
> chrome/app/breakpad_posix.cc:**44: #include
> "seccompsandbox/linux_syscall_**support.h"
> Remove this include if it shouldn't be in the android build
>
> https://chromiumcodereview.**appspot.com/9838033/diff/1/**
>
chrome/app/breakpad_posix.cc#**newcode46<https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_posix.cc#newcode46>
> chrome/app/breakpad_posix.cc:**46: #ifdef OS_ANDROID
> Nit: preferred style is #if defined(OS_ANDROID)
>
> https://chromiumcodereview.**appspot.com/9838033/diff/1/**
>
chrome/app/breakpad_posix.cc#**newcode700<https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_posix.cc#newcode700>
> chrome/app/breakpad_posix.cc:**700: __android_log_write(ANDROID_**
> LOG_WARN,
> "chromium", output_msg);
> Why are these printed with "chromium" whereas the others are with
> google-breakpad? It's just the activity mentioned in logcat, right?
> Seems like we should be consistent. Also, can this juse the log function
> from above?
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/app/breakpad_posix.cc<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_posix.cc>
> File chrome/app/breakpad_posix.cc (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/app/breakpad_posix.cc#**newcode341<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_posix.cc#newcode341>
> chrome/app/breakpad_posix.cc:**341: size_t log(const char* buf, size_t
> nbytes) {
> Woah. Why is log defined twice? Also, you're already defining this in
> breakpad's client/linux/log/log.h. Can't you re-use it?
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/app/breakpad_posix.cc#**newcode965<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_posix.cc#newcode965>
> chrome/app/breakpad_posix.cc:**965: new
> google_breakpad::**ExceptionHandler(dumps_path.**value().c_str(), NULL,
> Seems like you could still assign to g_breakpad and maintain the
> DCHECK(!g_breakpad);
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/app/breakpad_posix.cc#**newcode1124<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_posix.cc#newcode1124>
> chrome/app/breakpad_posix.cc:**1124: return;
> Do we always early-return? I thought not, in which case won't your
> SetClientId be clobbered by the conditionals below?
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix.cc<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix.cc>
> File chrome/browser/crash_handler_**host_posix.cc (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix.cc#newcode5<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix.cc#newcode5>
> chrome/browser/crash_handler_**host_posix.cc:5: #include
> "chrome/browser/crash_handler_**host_linux.h"
> Fix include
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix.h<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix.h>
> File chrome/browser/crash_handler_**host_posix.h (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix.h#newcode5<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix.h#newcode5>
> chrome/browser/crash_handler_**host_posix.h:5: #ifndef
> CHROME_BROWSER_CRASH_HANDLER_**HOST_LINUX_H_
> Update these
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix_stub.cc<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix_stub.cc>
> File chrome/browser/crash_handler_**host_posix_stub.cc (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix_stub.cc#newcode8<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix_stub.cc#newcode8>
> chrome/browser/crash_handler_**host_posix_stub.cc:8: #include
> "chrome/browser/crash_handler_**host_linux.h"
> Fix include
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/browser/crash_handler_**host_posix_stub.cc#newcode12<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash_handler_host_posix_stub.cc#newcode12>
> chrome/browser/crash_handler_**host_posix_stub.cc:12:
> CrashHandlerHostLinux::**CrashHandlerHostLinux()
> If you are changing these to be in a _posix file, you should rename all
> the types accordingly (i.e. this should be CrashHandlerHosPosix)
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/chrome_browser.gypi<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/chrome_browser.gypi>
> File chrome/chrome_browser.gypi (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/2001/**
>
chrome/chrome_browser.gypi#**newcode4321<https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/chrome_browser.gypi#newcode4321>
> chrome/chrome_browser.gypi:**4321: ['linux_breakpad==1', {
> This should probably also be renamed.
>
>
https://chromiumcodereview.**appspot.com/9838033/<https://chromiumcodereview....
>

[carlosvaldivia, 2012-04-03 06:24:47]

rfal. Note that there is still more to be done with respect to renaming linux to
posix. Especially in light of crash_handler_host_posix.h changes. That said it
is late. I'll pick up the rest tomorrow.

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info.h
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info...
base/android/build_info.h:15: // BuildInfo is a singleton class that stores
android build and device
singleton.h doesn't appear to play well with zero-argument constructors. Also it
seems dangerous to make the various fields non-const.

On 2012/03/26 18:51:15, Yaron wrote:
> Seems like you could be using base/memory/singleton.h. The comments in there
> explain how to use it.

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
File chrome/app/breakpad_posix.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
chrome/app/breakpad_posix.cc:44: #include
"seccompsandbox/linux_syscall_support.h"
On 2012/03/26 18:51:15, Yaron wrote:
> Remove this include if it shouldn't be in the android build

Done.

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
chrome/app/breakpad_posix.cc:46: #ifdef OS_ANDROID
On 2012/03/26 18:51:15, Yaron wrote:
> Nit: preferred style is #if defined(OS_ANDROID)

Done.

https://chromiumcodereview.appspot.com/9838033/diff/1/chrome/app/breakpad_pos...
chrome/app/breakpad_posix.cc:700: __android_log_write(ANDROID_LOG_WARN,
"chromium", output_msg);
breakpad_posix is chromium code. The other cl is google-breakpad. Completely
separate project also used by non-chromium projects.

On 2012/03/26 18:51:15, Yaron wrote:
> Why are these printed with "chromium" whereas the others are with
> google-breakpad? It's just the activity mentioned in logcat, right? Seems like
> we should be consistent. Also, can this juse the log function from above?

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
File chrome/app/breakpad_posix.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
chrome/app/breakpad_posix.cc:341: size_t log(const char* buf, size_t nbytes) {
Mistake. Fixed.

On 2012/03/26 18:51:16, Yaron wrote:
> Woah. Why is log defined twice? Also, you're already defining this in
breakpad's
> client/linux/log/log.h. Can't you re-use it?

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
chrome/app/breakpad_posix.cc:965: new
google_breakpad::ExceptionHandler(dumps_path.value().c_str(), NULL,
On 2012/03/26 18:51:16, Yaron wrote:
> Seems like you could still assign to g_breakpad and maintain the
> DCHECK(!g_breakpad);

Done.

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/app/breakpad_...
chrome/app/breakpad_posix.cc:1124: return;
Isn't that the correct behavior? I would think that the default client id should
be overwritten if specified on the command line.

On 2012/03/26 18:51:16, Yaron wrote:
> Do we always early-return? I thought not, in which case won't your SetClientId
> be clobbered by the conditionals below?

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
File chrome/browser/crash_handler_host_posix.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix.cc:5: #include
"chrome/browser/crash_handler_host_linux.h"
On 2012/03/26 18:51:16, Yaron wrote:
> Fix include

Done.

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
File chrome/browser/crash_handler_host_posix.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix.h:5: #ifndef
CHROME_BROWSER_CRASH_HANDLER_HOST_LINUX_H_
On 2012/03/26 18:51:16, Yaron wrote:
> Update these

Done.

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
File chrome/browser/crash_handler_host_posix_stub.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix_stub.cc:8: #include
"chrome/browser/crash_handler_host_linux.h"
On 2012/03/26 18:51:16, Yaron wrote:
> Fix include

Done.

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/browser/crash...
chrome/browser/crash_handler_host_posix_stub.cc:12:
CrashHandlerHostLinux::CrashHandlerHostLinux()
On 2012/03/26 18:51:16, Yaron wrote:
> If you are changing these to be in a _posix file, you should rename all the
> types accordingly (i.e. this should be CrashHandlerHosPosix)

Done.

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/chrome_browse...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/9838033/diff/2001/chrome/chrome_browse...
chrome/chrome_browser.gypi:4321: ['linux_breakpad==1', {
On 2012/03/26 18:51:16, Yaron wrote:
> This should probably also be renamed.

Done.

[Yaron, 2012-04-03 23:54:20]

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info.h
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info...
base/android/build_info.h:15: // BuildInfo is a singleton class that stores
android build and device
On 2012/04/03 06:24:47, carlosvaldivia wrote:
> singleton.h doesn't appear to play well with zero-argument constructors. 

I see examples of it already. E.g. CountryDataMap in
chrome/browser/autofill/autofill_country.cc

Also it
> seems dangerous to make the various fields non-const.
> 
> On 2012/03/26 18:51:15, Yaron wrote:
> > Seems like you could be using base/memory/singleton.h. The comments in there
> > explain how to use it.
> 

Why is it dangerous? Because they're public?

https://chromiumcodereview.appspot.com/9838033/diff/11001/base/android/build_...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/base/android/build_...
base/android/build_info.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
2012 (throughout your change)

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/app/chrome_m...
File chrome/app/chrome_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/app/chrome_m...
chrome/app/chrome_main_delegate.cc:92: #include "chrome/app/breakpad_linux.h"
breakpad_posix

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:123: #include "base/linux_util.h"
Should this be moved too?

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:124: #include
"chrome/app/breakpad_linux.h"
posix

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1591: // TODO(carlosvaldivia):
Upstream breakpad code for Android and remove this
This needs to be removed. Also tag this patch with the bug number listed

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chromeos/login/wizard_controller.cc:53: #include
"chrome/app/breakpad_linux.h"
Update this throughout

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/cras...
File chrome/browser/crash_handler_host_posix_stub.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/cras...
chrome/browser/crash_handler_host_posix_stub.cc:8: #include
"chrome/browser/crash_handler_host_linux.h"
This file needs to be updated to posix throughout or it won't work. You should
probably try sending this patch to the try servers to ensure you have coverage

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/common/loggi...
File chrome/common/logging_chrome_uitest.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/common/loggi...
chrome/common/logging_chrome_uitest.cc:152: #if defined(OS_LINUX) &&
!defined(USE_POSIX_BREAKPAD)
Should this be || OS_ANDROID?

[carlosvaldivia, 2012-04-04 20:52:34]

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info.h
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/1/base/android/build_info...
base/android/build_info.h:15: // BuildInfo is a singleton class that stores
android build and device
I meant non-zero argument constructors like the one found in this class.

Its dangerous because they are public and mutable and they shouldn't be.

If you insist, I'll use the singleton.h stuff. I do think there is some utility
in conforming to the broader chrome idiom. But in this particular case it
appears to make the code more complicated and fragile. 

On 2012/04/03 23:54:20, Yaron wrote:
> On 2012/04/03 06:24:47, carlosvaldivia wrote:
> > singleton.h doesn't appear to play well with zero-argument constructors. 
> 
> I see examples of it already. E.g. CountryDataMap in
> chrome/browser/autofill/autofill_country.cc
> 
> Also it
> > seems dangerous to make the various fields non-const.
> > 
> > On 2012/03/26 18:51:15, Yaron wrote:
> > > Seems like you could be using base/memory/singleton.h. The comments in
there
> > > explain how to use it.
> > 
> 
> Why is it dangerous? Because they're public?

https://chromiumcodereview.appspot.com/9838033/diff/11001/base/android/build_...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/base/android/build_...
base/android/build_info.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2012/04/03 23:54:20, Yaron wrote:
> 2012 (throughout your change)

Done.

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/app/chrome_m...
File chrome/app/chrome_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/app/chrome_m...
chrome/app/chrome_main_delegate.cc:92: #include "chrome/app/breakpad_linux.h"
On 2012/04/03 23:54:20, Yaron wrote:
> breakpad_posix

Done.

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:123: #include "base/linux_util.h"
How about in the future? I'm afraid that this cl is already becoming bloated
with re-names.

On 2012/04/03 23:54:20, Yaron wrote:
> Should this be moved too?

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:124: #include
"chrome/app/breakpad_linux.h"
On 2012/04/03 23:54:20, Yaron wrote:
> posix

Done.

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1591: // TODO(carlosvaldivia):
Upstream breakpad code for Android and remove this
On 2012/04/03 23:54:20, Yaron wrote:
> This needs to be removed. Also tag this patch with the bug number listed

Done.

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/chro...
chrome/browser/chromeos/login/wizard_controller.cc:53: #include
"chrome/app/breakpad_linux.h"
On 2012/04/03 23:54:20, Yaron wrote:
> Update this throughout

Done.

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/cras...
File chrome/browser/crash_handler_host_posix_stub.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/browser/cras...
chrome/browser/crash_handler_host_posix_stub.cc:8: #include
"chrome/browser/crash_handler_host_linux.h"
On 2012/04/03 23:54:20, Yaron wrote:
> This file needs to be updated to posix throughout or it won't work. You should
> probably try sending this patch to the try servers to ensure you have coverage

Done.

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/common/loggi...
File chrome/common/logging_chrome_uitest.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/11001/chrome/common/loggi...
chrome/common/logging_chrome_uitest.cc:152: #if defined(OS_LINUX) &&
!defined(USE_POSIX_BREAKPAD)
On 2012/04/03 23:54:20, Yaron wrote:
> Should this be || OS_ANDROID?

Done. Similar case elsewhere as well.

[Yaron, 2012-04-04 21:41:04]

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1590: #if defined(OS_ANDROID)
I didn't mean to get rid of just the TODO... You should also get rid of the
#ifdef

[Yaron, 2012-04-04 21:43:58]

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/common/loggi...
File chrome/common/logging_chrome_uitest.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/common/loggi...
chrome/common/logging_chrome_uitest.cc:69: #if (defined(OS_LINUX) ||
defined(OS_POSIX)) \
This should either be OS_LINUX || OS_ANDROID or it should be OS_POSIX.

One difference is that OS_POSIX includes Mac, so you might need to also exclude
it if you go that route

[carlosvaldivia, 2012-04-04 21:54:08]

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1590: #if defined(OS_ANDROID)
On 2012/04/04 21:41:04, Yaron wrote:
> I didn't mean to get rid of just the TODO... You should also get rid of the
> #ifdef 

Done.

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/common/loggi...
File chrome/common/logging_chrome_uitest.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/19001/chrome/common/loggi...
chrome/common/logging_chrome_uitest.cc:69: #if (defined(OS_LINUX) ||
defined(OS_POSIX)) \
On 2012/04/04 21:43:58, Yaron wrote:
> This should either be OS_LINUX || OS_ANDROID or it should be OS_POSIX.
> 
> One difference is that OS_POSIX includes Mac, so you might need to also
exclude
> it if you go that route

Done.

[Yaron, 2012-04-04 23:31:03]

lg, thanks.

Mark, can you take a look?

[Mark Mentovai, 2012-04-05 14:28:02]

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
File base/android/build_info.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
2012

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:10: #include "base/android/jni_android.h"
Sort. android < logging.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:14: #include "jni/build_info_jni.h"
Why is this in a separate section?

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:41: jobject app_context = GetApplicationContext();
You don’t need to call this ’til you need it, which looks like it happens at
about line 68.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:47: const char* device = strdup(device_str.c_str());
#include <string.h> to be able to call strdup.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:87: LOG(INFO) << "BuildInfo instance initialized
with"
This doesn’t seem necessary.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:9: #include <string>
Separate C system headers from C++ system headers with a blank line.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:14: 
Get rid of this extra blank line and the one at line 56.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:20: // Static factory method for getting the singleton
BuildInfo instance.
Observe the proper ordering within a class, per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
.

Your static function should follow the destructor.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:21: static const BuildInfo* const GetInstance();
Say somewhere that this leaks all of its data and explain why it’s OK.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:21: static const BuildInfo* const GetInstance();
Consider using a Singleton (base/memory/singleton.h) instead of implementing
this custom stuff.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:23: ~BuildInfo() {};
The semicolon; is unnecessary.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:25: // We use const char* instead of std::strings
because we need to have these
Don”t write “we” in comments.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:29: const char* device;
Data members should always be private.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:38: static BuildInfo* instance_;
private + static + used directly only in one file? Sounds like you should take
it out of the class altogether and just put it in the .cc file that uses it.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:48: 
DISALLOW_COPY_AND_ASSIGN?

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/java/o...
File base/android/java/org/chromium/base/BuildInfo.java (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/java/o...
base/android/java/org/chromium/base/BuildInfo.java:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/java/o...
base/android/java/org/chromium/base/BuildInfo.java:5: package org.chromium.base;
Other than that, I haven’t reviewed this file carefully.

https://chromiumcodereview.appspot.com/9838033/diff/33001/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/breakpad/breakpad.g...
breakpad/breakpad.gyp:228: ['posix_breakpad==1', {
This isn’t a correct name. I would leave it at linux_breakpad.

If you’ve already started using posix_breakpad elsewhere, you should correct
that usage now.

https://chromiumcodereview.appspot.com/9838033/diff/33001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/build/common.gypi#n...
build/common.gypi:616: 'posix_breakpad%': 0,
No.

https://chromiumcodereview.appspot.com/9838033/diff/33001/chrome/app/breakpad...
File chrome/app/breakpad_posix.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/chrome/app/breakpad...
chrome/app/breakpad_posix.h:5: #ifndef CHROME_APP_BREAKPAD_POSIX_H_
No. Move these files back to their old names as well.

https://chromiumcodereview.appspot.com/9838033/diff/33001/chrome/app/breakpad...
chrome/app/breakpad_posix.h:30: uint64_t pid;                    // PID where
applicable.
This addition is of course fine.

[carlosvaldivia, 2012-04-05 18:35:56]

Point of contention:

I don't think we can easily rename crash_handler_host_posix and breakpad_posix
back to *_linux with gyp related problems. Thoughts? What is the cost of the
rename.

Todo:

1) Rename posix_breakpad back to linux_breakpad
2) Make BuildInfo conform to singleton.h

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
File base/android/build_info.cc (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:10: #include "base/android/jni_android.h"
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Sort. android < logging.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:14: #include "jni/build_info_jni.h"
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Why is this in a separate section?

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:47: const char* device = strdup(device_str.c_str());
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> #include <string.h> to be able to call strdup.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.cc:87: LOG(INFO) << "BuildInfo instance initialized
with"
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> This doesn’t seem necessary.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:9: #include <string>
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Separate C system headers from C++ system headers with a blank line.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:14: 
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Get rid of this extra blank line and the one at line 56.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:20: // Static factory method for getting the singleton
BuildInfo instance.
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Observe the proper ordering within a class, per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> .
> 
> Your static function should follow the destructor.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:21: static const BuildInfo* const GetInstance();
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Say somewhere that this leaks all of its data and explain why it’s OK.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:21: static const BuildInfo* const GetInstance();
I'll get to that along with the linux/posix_breakpad rename after I respond to
these comments.

On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Consider using a Singleton (base/memory/singleton.h) instead of implementing
> this custom stuff.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:23: ~BuildInfo() {};
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> The semicolon; is unnecessary.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:25: // We use const char* instead of std::strings
because we need to have these
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Don”t write “we” in comments.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:29: const char* device;
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Data members should always be private.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:38: static BuildInfo* instance_;
It is a jni wrapper. There is java code that uses it as well.

On 2012/04/05 14:28:03, Mark Mentovai wrote:
> private + static + used directly only in one file? Sounds like you should take
> it out of the class altogether and just put it in the .cc file that uses it.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
base/android/build_info.h:48: 
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/java/o...
File base/android/java/org/chromium/base/BuildInfo.java (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/java/o...
base/android/java/org/chromium/base/BuildInfo.java:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/java/o...
base/android/java/org/chromium/base/BuildInfo.java:5: package org.chromium.base;
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> Other than that, I haven’t reviewed this file carefully.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/33001/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/breakpad/breakpad.g...
breakpad/breakpad.gyp:228: ['posix_breakpad==1', {
Ok I'll get to this.

Out of curiosity why is renaming linuix_breakpad to posix_breakpad bad?

On 2012/04/05 14:28:03, Mark Mentovai wrote:
> This isn’t a correct name. I would leave it at linux_breakpad.
> 
> If you’ve already started using posix_breakpad elsewhere, you should correct
> that usage now.

https://chromiumcodereview.appspot.com/9838033/diff/33001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/build/common.gypi#n...
build/common.gypi:616: 'posix_breakpad%': 0,
I'll get to it.

On 2012/04/05 14:28:03, Mark Mentovai wrote:
> No.

https://chromiumcodereview.appspot.com/9838033/diff/33001/chrome/app/breakpad...
File chrome/app/breakpad_posix.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/33001/chrome/app/breakpad...
chrome/app/breakpad_posix.h:5: #ifndef CHROME_APP_BREAKPAD_POSIX_H_
So what is the cost? Given our compilation filters (the android build excludes
_linux files from the android compilation, I was forced into this change. I
remember trying to include the relevant
breakpad_psoix/crash_handler_host_linux.*  files explicitly to our gyp files,
but they were still getting excluded by the broader rule.

On 2012/04/05 14:28:03, Mark Mentovai wrote:
> No. Move these files back to their old names as well.

https://chromiumcodereview.appspot.com/9838033/diff/33001/chrome/app/breakpad...
chrome/app/breakpad_posix.h:30: uint64_t pid;                    // PID where
applicable.
On 2012/04/05 14:28:03, Mark Mentovai wrote:
> This addition is of course fine.

Done.

[Mark Mentovai, 2012-04-05 18:45:19]

Breakpad has a different interface for each platform. There is no such thing as
POSIX Breakpad. There are some POSIX-like systems that Breakpad supports that
are nothing like Linux. Take the Mac, for example.

Android uses the Linux breakpad implementation because, for the most part, at
the level Breakpad operates at, it’s Linux.

If you can’t make this work while leaving the name “linux_breakpad”, you can
pick a new name, but that name can’t be “posix_breakpad”.
linux_or_android_breakpad would suffice, I guess. It’s most preferable, in my
opinion, to just leave it at linux_breakpad and add a .gyp rule to bring
linux_breakpad files back in on Android when Breakpad is enabled there.

[carlosvaldivia, 2012-04-05 20:38:43]

How about a new suffix "lindroid" or "linuxish".

Ideally I'd like to just stick with _linux, but pattern lists defined in
filename_rules.gypi don't seem to get overridden when I define a new
pattern_list in chrome_browser.gypi to include crash_host_handler_linux or
breakpad_linux.

On 2012/04/05 18:45:19, Mark Mentovai wrote:
> Breakpad has a different interface for each platform. There is no such thing
as
> POSIX Breakpad. There are some POSIX-like systems that Breakpad supports that
> are nothing like Linux. Take the Mac, for example.
> 
> Android uses the Linux breakpad implementation because, for the most part, at
> the level Breakpad operates at, it’s Linux.
> 
> If you can’t make this work while leaving the name “linux_breakpad”, you can
> pick a new name, but that name can’t be “posix_breakpad”.
> linux_or_android_breakpad would suffice, I guess. It’s most preferable, in my
> opinion, to just leave it at linux_breakpad and add a .gyp rule to bring
> linux_breakpad files back in on Android when Breakpad is enabled there.

[Mark Mentovai, 2012-04-05 20:59:31]

That’d be fine. Just get it off of posix. (We really really don’t want to build
these files on all posix platforms.)

The pattern include/exclude rules are a little hard to supplement. You might
have luck convincing GYP to put them in the right spot by sticking them inside a
target_conditions block. The condition can be a tautology like '1'. The point is
you’d be forcing GYP to delay merging the includes until after the excludes have
already been established, since those include/excludes are processed
sequentially.

[carlosvaldivia, 2012-04-06 02:58:10]

Hey Mark.

Renaming has been done. Also build info is now using singleton.h.

I haven't renamed crash handler hosts to "Linixish" because "Linuxish" makes me
want to vomit. Sadly a better name escapes me.

On 2012/04/05 20:59:31, Mark Mentovai wrote:
> That’d be fine. Just get it off of posix. (We really really don’t want to
build
> these files on all posix platforms.)
> 
> The pattern include/exclude rules are a little hard to supplement. You might
> have luck convincing GYP to put them in the right spot by sticking them inside
a
> target_conditions block. The condition can be a tautology like '1'. The point
is
> you’d be forcing GYP to delay merging the includes until after the excludes
have
> already been established, since those include/excludes are processed
> sequentially.

[Mark Mentovai, 2012-04-06 18:56:12]

LGTM with these changes.

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
base/android/build_info.h:26: // question isn't actually freed until shutdown.
This is ok because there
I would just let it leak, and use LeakySingletonTraits in the other file.
There’s no point in running code at exit. We generally avoid code running at
exit on Chrome.

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
base/android/build_info.h:34: const char* device() {
These can be const methods, like

  const char* device() const {

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
base/android/build_info.h:87: 
Remove one of these blank lines.

[carlosvaldivia, 2012-04-06 19:55:23]

Thanks reviewing!

Going to do one more sync and test pass before I commit.

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
File base/android/build_info.h (right):

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
base/android/build_info.h:26: // question isn't actually freed until shutdown.
This is ok because there
On 2012/04/06 18:56:12, Mark Mentovai wrote:
> I would just let it leak, and use LeakySingletonTraits in the other file.
> There’s no point in running code at exit. We generally avoid code running at
> exit on Chrome.

Done.

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
base/android/build_info.h:34: const char* device() {
On 2012/04/06 18:56:12, Mark Mentovai wrote:
> These can be const methods, like
> 
>   const char* device() const {

Done.

https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_...
base/android/build_info.h:87: 
On 2012/04/06 18:56:12, Mark Mentovai wrote:
> Remove one of these blank lines.

Done.

[commit-bot: I haz the power, 2012-04-06 20:02:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/carlosvaldivia@google.com/9838033/44002

[commit-bot: I haz the power, 2012-04-06 20:02:22]

Can't apply patch for file chrome/app/breakpad_linuxish.cc.
While running patch -p1 --forward --force;
patching file chrome/app/breakpad_linuxish.cc
Hunk #2 succeeded at 42 (offset 1 line).
Hunk #3 succeeded at 127 (offset 4 lines).
Hunk #4 succeeded at 341 (offset 4 lines).
Hunk #5 succeeded at 358 (offset 4 lines).
Hunk #6 succeeded at 379 (offset 4 lines).
Hunk #7 succeeded at 394 (offset 4 lines).
Hunk #8 succeeded at 418 (offset 4 lines).
Hunk #9 succeeded at 426 (offset 4 lines).
Hunk #10 succeeded at 525 (offset 14 lines).
Hunk #11 succeeded at 554 (offset 14 lines).
Hunk #12 succeeded at 723 (offset 25 lines).
Hunk #13 succeeded at 833 (offset 25 lines).
Hunk #14 succeeded at 863 (offset 25 lines).
Hunk #15 FAILED at 914.
Hunk #16 succeeded at 952 (offset 26 lines).
Hunk #17 succeeded at 960 (offset 26 lines).
Hunk #18 succeeded at 977 (offset 26 lines).
Hunk #19 succeeded at 1002 (offset 26 lines).
Hunk #20 succeeded at 1013 (offset 26 lines).
Hunk #21 succeeded at 1090 (offset 28 lines).
Hunk #22 succeeded at 1098 (offset 28 lines).
Hunk #23 succeeded at 1115 (offset 28 lines).
Hunk #24 succeeded at 1134 (offset 28 lines).
1 out of 24 hunks FAILED -- saving rejects to file
chrome/app/breakpad_linuxish.cc.rej

[commit-bot: I haz the power, 2012-04-06 20:08:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/carlosvaldivia@google.com/9838033/36009

[commit-bot: I haz the power, 2012-04-06 20:09:06]

Presubmit check for 9838033-36009 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

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/gtk

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Peter Kasting, 2012-04-06 20:17:35]

LGTM

[commit-bot: I haz the power, 2012-04-06 20:21:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/carlosvaldivia@google.com/9838033/36009

[commit-bot: I haz the power, 2012-04-06 20:33:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/carlosvaldivia@google.com/9838033/37080

[commit-bot: I haz the power, 2012-04-06 21:33:57]

Try job failure for 9838033-37080 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[carlosvaldivia, 2012-04-06 21:48:21]

Hey Mark,

Turns out I need to add a dependency rule to chrome/apps/DEPS.  Basically
the OS_ANDROID version needs to get third_party/lss/linux_syscall_support.h.

I was wondering if you could weigh in before I try again.

Thanks,
Carlos

On Fri, Apr 6, 2012 at 11:56 AM, <mark@chromium.org> wrote:

> LGTM with these changes.
>
>
> https://chromiumcodereview.**appspot.com/9838033/diff/**
>
43001/base/android/build_info.**h<https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_info.h>
> File base/android/build_info.h (right):
>
> https://chromiumcodereview.**appspot.com/9838033/diff/**
>
43001/base/android/build_info.**h#newcode26<https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_info.h#newcode26>
> base/android/build_info.h:26: // question isn't actually freed until
> shutdown. This is ok because there
> I would just let it leak, and use LeakySingletonTraits in the other
> file. There’s no point in running code at exit. We generally avoid code
> running at exit on Chrome.
>
> https://chromiumcodereview.**appspot.com/9838033/diff/**
>
43001/base/android/build_info.**h#newcode34<https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_info.h#newcode34>
> base/android/build_info.h:34: const char* device() {
> These can be const methods, like
>
>  const char* device() const {
>
> https://chromiumcodereview.**appspot.com/9838033/diff/**
>
43001/base/android/build_info.**h#newcode87<https://chromiumcodereview.appspot.com/9838033/diff/43001/base/android/build_info.h#newcode87>
> base/android/build_info.h:87:
> Remove one of these blank lines.
>
>
https://chromiumcodereview.**appspot.com/9838033/<https://chromiumcodereview....
>

[Mark Mentovai, 2012-04-09 15:34:17]

LGTM

[commit-bot: I haz the power, 2012-04-09 17:47:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/carlosvaldivia@google.com/9838033/53001

[commit-bot: I haz the power, 2012-04-09 19:27:24]

Change committed as 131404

[Lei Zhang, 2012-04-09 21:14:18]

re: patch set 18 - per the "Re-using CLs when re-landing a reverted change"
thread on chromium-dev, recycling a CL in Reitveld was discouraged. I would
upload a new CL with patch set 17, and then upload patchset 18 to show the fix.