Upstream android file related code.

Upstream android file related code.

Implemented file related features

BUG=None
TEST=None

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

[michaelbai, 2011-06-17 00:17:29]

Please help to review it.

[joth, 2011-06-17 11:25:54]

Couple comments on making this better match chromium style.

http://codereview.chromium.org/7184032/diff/1/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7184032/diff/1/base/platform_file.h#newcode26
base/platform_file.h:26: extern "C" char* mkdtemp(char* path);
could we just put these declarations in the .cc that need it
(platform_file_posix.cc, file_util_posix.cc respectively)?

No need to publish them to the users of this cross platform wrapper.

If we need to share them across .cc files, we might make an internal .h to host
them.

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc
File base/platform_file_android.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc#n...
base/platform_file_android.cc:8: #include <utime.h>
nit: \n between sections

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:115: DCHECK(mapped_file_ == -1);
DCHECK_EQ(-1, mapped_file_)

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:118: mapped_file_ =
ashmem_create_region(name.data(), size);
name.c_str(), to ensure it is null terminated

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:250: const uint32 ashmem_bytes =
ashmem_get_size_region(mapped_file_);
ashmem_get_size_region appears to return 'int', probably best to use that here
too

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:259: // in TransportDIB::Map().
can we fix this? It seems to be abusing both Map(0) and created_size()
see also bug http://crbug.com/60821 - that seems to suggest TransportDIB should
just keep track itself of the size it requested
just calling ashmem_get_size_region in transport_dib_android doesn't sound
awful, but if it is, how about we could always add a dedicated OS_ANDROID only
function onto SharedMemory to do what we need.
SharedMemory::GetAshmenSizeRegion()

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:303: static inline int lockf(int fd, int cmd, off_t
ignored_len) {
nit: use anon namespace rather than static (unless we want to ensure a build
break should it get added to android in future?)

also why here? Maybe better up top of file (with any other related platform
functions)

http://codereview.chromium.org/7184032/diff/1/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/sys_info_posix.cc#newcode10
base/sys_info_posix.cc:10: #if defined(OS_ANDROID)
this is unreliable as we may not have included base/basictypes yet.

The preferred format is to put conditional dependencies in their own section
after all other #includes i.e. just before namespace base {

[joth, 2011-06-17 11:38:28]

couple more things i noticed
- put BUG=None, TEST=None in the CL description
- check_deps will break when this lands: 
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/32565/st...
will need to add a global(?) rule to permit including third_party/ashmem  -
although perhaps that would be better as third_party/android/ashmem 

Thanks!

[brettw, 2011-06-17 16:29:14]

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc#newcode38
base/file_util_posix.cc:38: #if defined(OS_ANDROID)
Can you put this below all other includes with a blank line above it? This is
our new style for #ifdefed includes.

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc
File base/platform_file_android.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc#n...
base/platform_file_android.cc:20: }
I'd probably format this like a namespace and write:
}  // extern "C"

[darin (slow to review), 2011-06-17 16:29:29]

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc#newcode39
base/file_util_posix.cc:39: #include "base/platform_file.h"
nit: this include looks unnecessary.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:114: #if defined(OS_ANDROID)
it seems like almost all of the code in this file is #ifdef'd differently for
android.  maybe there is a better way to factor things so that the code is less
chopped up?  maybe shared_memroy_android.cc would make sense?

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:120: LOG(ERROR) << "Shared memory creation failed";
nit: do you really need these log statements to be compiled into release mode? 
wouldn't DLOG suffice?

[michaelbai, 2011-06-17 22:41:29]

Thanks very much for your review. Addressed all comments, please check again.

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc#newcode38
base/file_util_posix.cc:38: #if defined(OS_ANDROID)
On 2011/06/17 16:29:14, brettw wrote:
> Can you put this below all other includes with a blank line above it? This is
> our new style for #ifdefed includes.

Done.

http://codereview.chromium.org/7184032/diff/1/base/file_util_posix.cc#newcode39
base/file_util_posix.cc:39: #include "base/platform_file.h"
On 2011/06/17 16:29:29, darin wrote:
> nit: this include looks unnecessary.

Removed, but still need 'base/platform_file_android.h'

http://codereview.chromium.org/7184032/diff/1/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7184032/diff/1/base/platform_file.h#newcode26
base/platform_file.h:26: extern "C" char* mkdtemp(char* path);
On 2011/06/17 11:25:54, joth wrote:
> could we just put these declarations in the .cc that need it
> (platform_file_posix.cc, file_util_posix.cc respectively)?
> 
> No need to publish them to the users of this cross platform wrapper.
> 
> If we need to share them across .cc files, we might make an internal .h to
host
> them.

Done.

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc
File base/platform_file_android.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc#n...
base/platform_file_android.cc:8: #include <utime.h>
On 2011/06/17 11:25:54, joth wrote:
> nit: \n between sections

Done.

http://codereview.chromium.org/7184032/diff/1/base/platform_file_android.cc#n...
base/platform_file_android.cc:20: }
On 2011/06/17 16:29:14, brettw wrote:
> I'd probably format this like a namespace and write:
> }  // extern "C"

Done.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:114: #if defined(OS_ANDROID)
On 2011/06/17 16:29:29, darin wrote:
> it seems like almost all of the code in this file is #ifdef'd differently for
> android.  maybe there is a better way to factor things so that the code is
less
> chopped up?  maybe shared_memroy_android.cc would make sense?

I moved fully android specific methods into shared_memory_android.c, but can not
completely get ride of android specific code from this file because android
still need share some posix code.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:115: DCHECK(mapped_file_ == -1);
On 2011/06/17 11:25:54, joth wrote:
> DCHECK_EQ(-1, mapped_file_)

Done.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:118: mapped_file_ =
ashmem_create_region(name.data(), size);
On 2011/06/17 11:25:54, joth wrote:
> name.c_str(), to ensure it is null terminated

Done, thanks

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:120: LOG(ERROR) << "Shared memory creation failed";
On 2011/06/17 16:29:29, darin wrote:
> nit: do you really need these log statements to be compiled into release mode?

> wouldn't DLOG suffice?

Done.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:250: const uint32 ashmem_bytes =
ashmem_get_size_region(mapped_file_);
On 2011/06/17 11:25:54, joth wrote:
> ashmem_get_size_region appears to return 'int', probably best to use that here
> too

Done.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:259: // in TransportDIB::Map().
On 2011/06/17 11:25:54, joth wrote:
> can we fix this? It seems to be abusing both Map(0) and created_size()
> see also bug http://crbug.com/60821 - that seems to suggest TransportDIB
should
> just keep track itself of the size it requested
> just calling ashmem_get_size_region in transport_dib_android doesn't sound
> awful, but if it is, how about we could always add a dedicated OS_ANDROID only
> function onto SharedMemory to do what we need.
> SharedMemory::GetAshmenSizeRegion()

Method added, I will have another CL to handle the reset of change.

http://codereview.chromium.org/7184032/diff/1/base/shared_memory_posix.cc#new...
base/shared_memory_posix.cc:303: static inline int lockf(int fd, int cmd, off_t
ignored_len) {
On 2011/06/17 11:25:54, joth wrote:
> nit: use anon namespace rather than static (unless we want to ensure a build
> break should it get added to android in future?)
> 
> also why here? Maybe better up top of file (with any other related platform
> functions)

Done.

http://codereview.chromium.org/7184032/diff/1/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/7184032/diff/1/base/sys_info_posix.cc#newcode10
base/sys_info_posix.cc:10: #if defined(OS_ANDROID)
On 2011/06/17 11:25:54, joth wrote:
> this is unreliable as we may not have included base/basictypes yet.
> 
> The preferred format is to put conditional dependencies in their own section
> after all other #includes i.e. just before namespace base { 

Done.

[joth, 2011-06-19 12:48:05]

Thanks! I just have a couple nits, and a question about the naming of
platform_file_android.h it would be good for Brett or Darin to chime in on.

http://codereview.chromium.org/7184032/diff/2013/base/DEPS
File base/DEPS (right):

http://codereview.chromium.org/7184032/diff/2013/base/DEPS#newcode2
base/DEPS:2: "+third_party/ashmem",
hopefully check_deps? doesn't complain about the missing directory. Not sure if
we should comment why this is referring to a non-existant dir?

http://codereview.chromium.org/7184032/diff/2013/base/platform_file_android.h
File base/platform_file_android.h (right):

http://codereview.chromium.org/7184032/diff/2013/base/platform_file_android.h...
base/platform_file_android.h:6: #define BASE_PLATFORM_FILE_ANDROID_H_
(Looking for a base OWNER opinion on this; I'm guessing)

I'm not sure about the file name for this cc/h pair.
platform_file.h already exists and has a different purpose: it's a
cross-platform wrapper that defines PlatformFile typedef and various methods
that operate on it.

In here we are declaring posix/linux methods that are missing from sys/time.h
and stdlib.h

Investigating a bit more, based on compat_execinfo.h I think the consistent
style would be to split this into a pair of files called 
base/compat_sys_time.h and base/compat_stdlib.h  and use these in place of their
name-sakes?

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory.h#newcode188
base/shared_memory.h:188: // Returns the size of shared memory.
// Returns the size of shared memory or a negative value on error.
(I think. AFAICT ashmem_get_size_region is implemented as an ioctl call, which
will return -1 on error and futz errno)

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:192: 
nit: remove \n

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:225: DCHECK_GE((uint32)ashmem_bytes, bytes);
nit: static_cast<uint32>(ashmem_bytes) although this check will be ineffective
if GetAshmenSizeRegion returns -1
-> should check for negative return value and return false(?)
-> could the GetAshmenSizeRegion() call not go inside the if (bytes == 0) below?

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:234: #endif
seems unfortunate to commit this hack when the code depending on it is can be
fixed before being committed itself, but there we go.

[brettw, 2011-06-20 03:50:40]

LGTM, just some minor things (don't need to wait for my review again).

http://codereview.chromium.org/7184032/diff/2013/base/DEPS
File base/DEPS (right):

http://codereview.chromium.org/7184032/diff/2013/base/DEPS#newcode2
base/DEPS:2: "+third_party/ashmem",
This is fine.

http://codereview.chromium.org/7184032/diff/2013/base/platform_file_android.h
File base/platform_file_android.h (right):

http://codereview.chromium.org/7184032/diff/2013/base/platform_file_android.h...
base/platform_file_android.h:6: #define BASE_PLATFORM_FILE_ANDROID_H_
How about file_util_android.h/cc? I'd rather not have two separate headers, each
with just one function. file_util is kind of "misc stuff" and I think it's fine
if these go in there if you agree.

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_android.cc
File base/shared_memory_android.cc (right):

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_android.c...
base/shared_memory_android.cc:56: }  // namespace base
Nit: extra blank line before this.

[darin (slow to review), 2011-06-20 18:44:01]

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc
File base/base_paths_android.cc (right):

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc#n...
base/base_paths_android.cc:10: #include "base/android_os.h"
this CL does not have this android_os.h file, and i don't see that in the tree. 
is it in a different CL that i can see?  it might help to be able to reference
it when reviewing this code.

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc#n...
base/base_paths_android.cc:32: case base::DIR_MODULE: {
this is supposed to be directory of the currently executing code.  are you sure
this GetLibDirectory does that?  judging by the name, i'd guess not.  maybe a
TODO comment is at least warranted if you don't care about the distinction in
practice?

by the way, it can make it difficult for other developers to make use of
DIR_MODULE in new ways if it is not properly implemented.

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc#n...
base/base_paths_android.cc:40: // to the device via android_run_test.sh.
android_run_test.sh is also not part of this CL.  be careful about adding
comments that refer to files.  they can become easily stale.

http://codereview.chromium.org/7184032/diff/2025/base/file_util_android.h
File base/file_util_android.h (right):

http://codereview.chromium.org/7184032/diff/2025/base/file_util_android.h#new...
base/file_util_android.h:6: #define BASE_FILE_UTIL_ANDROID_H_
it really seems wrong to call this file_util* since it is not an extension of
file_util.h.  it is something completely different.

it feels more like you might want a name like bsd_compat_android.h since both
futimes and mkdtemp are BSD functions that Linux happens to also support.

http://codereview.chromium.org/7184032/diff/2025/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7184032/diff/2025/base/file_util_posix.cc#newc...
base/file_util_posix.cc:859: #endif  // !defined(OS_ANDROID)
no need for the comment here.  the #if block is so very small, and you aren't
consistently using comments like this elsewhere.

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory.h#newcode189
base/shared_memory.h:189: int GetAshmenSizeRegion();
Is "Asmen" a common word on Android?  It is good to use function names
consisting of words and common acronyms.  OK, I see... the underlying API is
prefixed by ashmen_.

Can you tell me why you need this Android-specific public method?  Are you sure
there isn't a portable method to be defined here?

Note: The comment on created_size().  It says that the caller should know the
size.

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:33: // The lockf() function is not available on
Android; we translate to flock().
hmm... will this be needed elsewhere?  i was about to suggest that you could put
this into the bsd_compat_android.h file, but lockf is POSIX not BSD ;-)

maybe bsd_compat_android.h could be more generically named os_compat_android.h? 
anyways, it seems like you might end up with a collection of compat bits like
this, and so having a common place for them might be nice.

[michaelbai, 2011-06-20 22:50:36]

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory.h#newcode188
base/shared_memory.h:188: // Returns the size of shared memory.
On 2011/06/19 12:48:05, joth wrote:
> // Returns the size of shared memory or a negative value on error.
> (I think. AFAICT ashmem_get_size_region is implemented as an ioctl call, which
> will return -1 on error and futz errno)

Done.

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_android.cc
File base/shared_memory_android.cc (right):

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_android.c...
base/shared_memory_android.cc:56: }  // namespace base
On 2011/06/20 03:50:40, brettw wrote:
> Nit: extra blank line before this.

Done.

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:192: 
On 2011/06/19 12:48:05, joth wrote:
> nit: remove \n

Done.

http://codereview.chromium.org/7184032/diff/2013/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:225: DCHECK_GE((uint32)ashmem_bytes, bytes);
On 2011/06/19 12:48:05, joth wrote:
> nit: static_cast<uint32>(ashmem_bytes) although this check will be ineffective
> if GetAshmenSizeRegion returns -1
> -> should check for negative return value and return false(?)
> -> could the GetAshmenSizeRegion() call not go inside the if (bytes == 0)
below?
> 
> 

Done.

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc
File base/base_paths_android.cc (right):

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc#n...
base/base_paths_android.cc:10: #include "base/android_os.h"
On 2011/06/20 18:44:01, darin wrote:
> this CL does not have this android_os.h file, and i don't see that in the
tree. 
> is it in a different CL that i can see?  it might help to be able to reference
> it when reviewing this code.

I will provide it in next CL.

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc#n...
base/base_paths_android.cc:32: case base::DIR_MODULE: {
On 2011/06/20 18:44:01, darin wrote:
> this is supposed to be directory of the currently executing code.  are you
sure
> this GetLibDirectory does that?  judging by the name, i'd guess not.  maybe a
> TODO comment is at least warranted if you don't care about the distinction in
> practice?
> 
> by the way, it can make it difficult for other developers to make use of
> DIR_MODULE in new ways if it is not properly implemented.

As we discussed, implemented FILE_MODULE and used default implementation for
DIR_MODULE

http://codereview.chromium.org/7184032/diff/2025/base/base_paths_android.cc#n...
base/base_paths_android.cc:40: // to the device via android_run_test.sh.
On 2011/06/20 18:44:01, darin wrote:
> android_run_test.sh is also not part of this CL.  be careful about adding
> comments that refer to files.  they can become easily stale.

Done.

http://codereview.chromium.org/7184032/diff/2025/base/file_util_android.h
File base/file_util_android.h (right):

http://codereview.chromium.org/7184032/diff/2025/base/file_util_android.h#new...
base/file_util_android.h:6: #define BASE_FILE_UTIL_ANDROID_H_
On 2011/06/20 18:44:01, darin wrote:
> it really seems wrong to call this file_util* since it is not an extension of
> file_util.h.  it is something completely different.
> 
> it feels more like you might want a name like bsd_compat_android.h since both
> futimes and mkdtemp are BSD functions that Linux happens to also support.

Rename to os_compat_android.*

http://codereview.chromium.org/7184032/diff/2025/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7184032/diff/2025/base/file_util_posix.cc#newc...
base/file_util_posix.cc:859: #endif  // !defined(OS_ANDROID)
On 2011/06/20 18:44:01, darin wrote:
> no need for the comment here.  the #if block is so very small, and you aren't
> consistently using comments like this elsewhere.

Done.

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory.h#newcode189
base/shared_memory.h:189: int GetAshmenSizeRegion();
On 2011/06/20 18:44:01, darin wrote:
> Is "Asmen" a common word on Android?  It is good to use function names
> consisting of words and common acronyms.  OK, I see... the underlying API is
> prefixed by ashmen_.
> 
> Can you tell me why you need this Android-specific public method?  Are you
sure
> there isn't a portable method to be defined here?
> 
> Note: The comment on created_size().  It says that the caller should know the
> size.

I think created_size() is just for one who created the shmem. This method is for
the one who open it. Joth point out http://crbug.com/60821 in previous comment

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/2025/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:33: // The lockf() function is not available on
Android; we translate to flock().
On 2011/06/20 18:44:01, darin wrote:
> hmm... will this be needed elsewhere?  i was about to suggest that you could
put
> this into the bsd_compat_android.h file, but lockf is POSIX not BSD ;-)
> 
> maybe bsd_compat_android.h could be more generically named
os_compat_android.h? 
> anyways, it seems like you might end up with a collection of compat bits like
> this, and so having a common place for them might be nice.

Added os_compat_andorid.*

[darin (slow to review), 2011-06-21 18:44:02]

http://codereview.chromium.org/7184032/diff/6003/base/base_paths_android.cc
File base/base_paths_android.cc (right):

http://codereview.chromium.org/7184032/diff/6003/base/base_paths_android.cc#n...
base/base_paths_android.cc:32: *result = aos->GetLibDirectory();
GetLibDirectory sounds like it returns the path to a directory, but FILE_MODULE
is supposed to return the path to a file.

http://codereview.chromium.org/7184032/diff/6003/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/7184032/diff/6003/base/shared_memory.h#newcode189
base/shared_memory.h:189: int GetAshmenSizeRegion();
I am still wondering why you need this method.  If the goal of
http://code.google.com/p/chromium/issues/detail?id=60821 is to hide knowledge of
the size, then the addition of this method here seems to be going against that
goal.

http://codereview.chromium.org/7184032/diff/6003/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/6003/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:36: }  // namespace.
nit: no trailing comment

[michaelbai, 2011-06-21 20:17:31]

Joth has replied your question about shmem.
Please see my answer below.

http://codereview.chromium.org/7184032/diff/6003/base/base_paths_android.cc
File base/base_paths_android.cc (right):

http://codereview.chromium.org/7184032/diff/6003/base/base_paths_android.cc#n...
base/base_paths_android.cc:32: *result = aos->GetLibDirectory();
On 2011/06/21 18:44:02, darin wrote:
> GetLibDirectory sounds like it returns the path to a directory, but
FILE_MODULE
> is supposed to return the path to a file.

OK, then, it seemed this change is not correct, since we don't a method to
return the file name. I will find out dladdr if it is feasible, I will send
another CL

http://codereview.chromium.org/7184032/diff/6003/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/6003/base/shared_memory_posix.cc#...
base/shared_memory_posix.cc:36: }  // namespace.
On 2011/06/21 18:44:02, darin wrote:
> nit: no trailing comment

Done.

[darin (slow to review), 2011-06-21 22:15:06]

LGTM w/ one nit:

http://codereview.chromium.org/7184032/diff/12015/base/shared_memory_posix.cc
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/7184032/diff/12015/base/shared_memory_posix.cc...
base/shared_memory_posix.cc:224: DCHECK_GE((uint32)ashmem_bytes, bytes);
nit: use a C++ style cast here.  (sorry, didn't notice before!)