Upstream android debug and log related files

Upstream android debug and log related files


BUG=
TEST=

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

[brettw, 2011-06-24 22:23:57]

Just some (hopefully easy) questions.

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc
File base/debug/debugger.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc#newcode16
base/debug/debugger.cc:16: #if defined(OS_ANDROID)
It's not clear why we need a log statement for Android and not for other
platforms. Is there something special about Android that makes this more
necessary?

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc
File base/debug/debugger_posix.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc#ne...
base/debug/debugger_posix.cc:184: #define DEBUG_BREAK() asm("bkpt 0")
Sorry I don't have much background in this. Why doesn't this generic ARM code
work on Android?

http://codereview.chromium.org/7238012/diff/1/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/7238012/diff/1/base/logging.h#newcode707
base/logging.h:707: // TODO(port): fix once "enough" works
Do you think "enough" works now? I can imagine this was necessary early, but it
seems it should be pretty good now.

http://codereview.chromium.org/7238012/diff/1/base/port.h
File base/port.h (right):

http://codereview.chromium.org/7238012/diff/1/base/port.h#newcode56
base/port.h:56: #include <ctype.h>
What's this for?

[michaelbai, 2011-06-24 23:30:44]

One issue left. let me know if you have further comments for others

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc
File base/debug/debugger.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc#newcode16
base/debug/debugger.cc:16: #if defined(OS_ANDROID)
On 2011/06/24 22:23:57, brettw wrote:
> It's not clear why we need a log statement for Android and not for other
> platforms. Is there something special about Android that makes this more
> necessary?


The pid from which we know which process to attach to are not output by android,
so we have to print it out explicitly.

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc
File base/debug/debugger_posix.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc#ne...
base/debug/debugger_posix.cc:184: #define DEBUG_BREAK() asm("bkpt 0")
On 2011/06/24 22:23:57, brettw wrote:
> Sorry I don't have much background in this. Why doesn't this generic ARM code
> work on Android?

For this issue, I have to get it back to you later.

http://codereview.chromium.org/7238012/diff/1/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/7238012/diff/1/base/logging.h#newcode707
base/logging.h:707: // TODO(port): fix once "enough" works
On 2011/06/24 22:23:57, brettw wrote:
> Do you think "enough" works now? I can imagine this was necessary early, but
it
> seems it should be pretty good now.

We haven't ready yet :(

http://codereview.chromium.org/7238012/diff/1/base/port.h
File base/port.h (right):

http://codereview.chromium.org/7238012/diff/1/base/port.h#newcode56
base/port.h:56: #include <ctype.h>
On 2011/06/24 22:23:57, brettw wrote:
> What's this for?

It is for tolower isspace etc. I can not figure out why other platform doesn't
need it. It might related Android platform. thought?

[brettw, 2011-06-25 00:07:19]

http://codereview.chromium.org/7238012/diff/1/base/port.h
File base/port.h (right):

http://codereview.chromium.org/7238012/diff/1/base/port.h#newcode56
base/port.h:56: #include <ctype.h>
Seems like we should be adding the ctype includes to the files that don't
compile. Anybody that uses tolower/isspace without including it is wrong.

[John Grabowski, 2011-06-28 17:27:23]

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc
File base/debug/debugger_posix.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc#ne...
base/debug/debugger_posix.cc:184: #define DEBUG_BREAK() asm("bkpt 0")
On 2011/06/24 22:23:57, brettw wrote:
> Sorry I don't have much background in this. Why doesn't this generic ARM code
> work on Android?

In general, Android is Java based.  Android has a "helpful" process called
debuggerd to catch native signals on the general assumption that they are fatal
errors.  Although symptoms change from build to build, in general we've had
great difficulty continuing in a debugger once we stop from SIGINT triggered by
native code.  So we don't.

[michaelbai, 2011-06-28 18:02:10]

http://codereview.chromium.org/7238012/diff/1/base/port.h
File base/port.h (right):

http://codereview.chromium.org/7238012/diff/1/base/port.h#newcode56
base/port.h:56: #include <ctype.h>
On 2011/06/25 00:07:19, brettw wrote:
> Seems like we should be adding the ctype includes to the files that don't
> compile. Anybody that uses tolower/isspace without including it is wrong.

It seemed that I have to change too many files and addd #if defined(OS_ANDROID),
If the port.h is not the right place, How about move it to basictypes.h?

[brettw, 2011-06-29 16:38:36]

http://codereview.chromium.org/7238012/diff/1/base/port.h
File base/port.h (right):

http://codereview.chromium.org/7238012/diff/1/base/port.h#newcode56
base/port.h:56: #include <ctype.h>
I really don't think ctype.h should be in any common file, and we should fix the
callers to include it properly. Although it may be a number of files, I doubt
doing that patch is very difficult.

[brettw, 2011-06-29 16:42:28]

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc
File base/debug/debugger.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc#newcode16
base/debug/debugger.cc:16: #if defined(OS_ANDROID)
Can we make the comment say what you just said then?

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc#newcode19
base/debug/debugger.cc:19: LOG(INFO) << "DebugUtil::WaitForDebugger(pid=" <<
(int)getpid() << ")";
We're not supposed to use C-style casts. I think you can just use static_cast
here.

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc
File base/debug/debugger_posix.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc#ne...
base/debug/debugger_posix.cc:184: #define DEBUG_BREAK() asm("bkpt 0")
Can a comment be added to this effect?

[michaelbai, 2011-06-29 17:18:46]

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc
File base/debug/debugger.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc#newcode16
base/debug/debugger.cc:16: #if defined(OS_ANDROID)
On 2011/06/24 23:30:44, michaelbai wrote:
> On 2011/06/24 22:23:57, brettw wrote:
> > It's not clear why we need a log statement for Android and not for other
> > platforms. Is there something special about Android that makes this more
> > necessary?
> 
> 
> The pid from which we know which process to attach to are not output by
android,
> so we have to print it out explicitly. 

Done.

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger.cc#newcode19
base/debug/debugger.cc:19: LOG(INFO) << "DebugUtil::WaitForDebugger(pid=" <<
(int)getpid() << ")";
On 2011/06/29 16:42:28, brettw wrote:
> We're not supposed to use C-style casts. I think you can just use static_cast
> here.

Done.

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc
File base/debug/debugger_posix.cc (right):

http://codereview.chromium.org/7238012/diff/1/base/debug/debugger_posix.cc#ne...
base/debug/debugger_posix.cc:184: #define DEBUG_BREAK() asm("bkpt 0")
On 2011/06/29 16:42:28, brettw wrote:
> Can a comment be added to this effect?

Done.

[michaelbai, 2011-07-06 18:05:52]

Removed the port.h file from this patch

http://codereview.chromium.org/7238012/diff/1/base/port.h
File base/port.h (right):

http://codereview.chromium.org/7238012/diff/1/base/port.h#newcode56
base/port.h:56: #include <ctype.h>
On 2011/06/29 16:38:36, brettw wrote:
> I really don't think ctype.h should be in any common file, and we should fix
the
> callers to include it properly. Although it may be a number of files, I doubt
> doing that patch is very difficult.

Done.

[brettw, 2011-07-07 16:55:53]

LGTM

http://codereview.chromium.org/7238012/diff/15001/base/string_number_conversi...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/7238012/diff/15001/base/string_number_conversi...
base/string_number_conversions.cc:17: #if defined(OS_ANDROID)
I wouldn't put these ctype includes in an android guard. It seems they "should"
be needed on other platforms. Plus, since it will always compile, we can make
the includes look nicer by removing the ifdefs. So just put this in the
"standard C includes" section above. Same for other places you added this.

[michaelbai, 2011-07-07 18:28:47]

http://codereview.chromium.org/7238012/diff/15001/base/string_number_conversi...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/7238012/diff/15001/base/string_number_conversi...
base/string_number_conversions.cc:17: #if defined(OS_ANDROID)
On 2011/07/07 16:55:53, brettw wrote:
> I wouldn't put these ctype includes in an android guard. It seems they
"should"
> be needed on other platforms. Plus, since it will always compile, we can make
> the includes look nicer by removing the ifdefs. So just put this in the
> "standard C includes" section above. Same for other places you added this.

Done.