Fix chrome upload with content uri

Fix chrome upload with content uri

For android, the upload file dialog returns files with content uri scheme(content://).
This CL makes it possible for upload to work with this new file type.
It fixes both the form and fileapi based uploads.

The CL follows the same code path used by regular file upload and the content url is encompassed by a FilePath object.

R=jar@chromium.org, joth@chromium.org, kinuko@chromium.org, mmenke@chromium.org, tsepez@chromium.org
TBR=yfriedman
BUG=278640
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235752

[qinmin, 2013-10-28 23:32:38]

+kinuko & ericu to review all the filepai/upload related functionality
+asanka for OWNER of net/
+jam for OWNER of content/
+joth for java files

This CL is pretty big due to 2 different upload code path, please let me know
wheter you want the CLs to be splitted into smaller pieces if needed

[joth, 2013-10-29 01:10:06]

On 2013/10/28 23:32:38, qinmin wrote:
> +kinuko & ericu to review all the filepai/upload related functionality
> +asanka for OWNER of net/
> +jam for OWNER of content/
> +joth for java files
> 
> This CL is pretty big due to 2 different upload code path, please let me know
> wheter you want the CLs to be splitted into smaller pieces if needed

I think the BUG= link is wrong?
Also would be good to have a brief overview of how this patch fixes the problem
in the description. (file descriptors are now...? passed to webkit? held in a
cache? etc) just to understand the overall design.

[joth, 2013-10-29 01:30:25]

high level comment - this patch connects a large number of OS_ANDROID specific
paths through numerous layers - I wonder how many layers could avoid android
specific knowledge if we changed the existing FilePath case to deal in a more
general GURL as the source, with file:// and content:// being specific examples
of how it is used.


I scanned through the java code quickly, couple small comments.

Cheers

https://codereview.chromium.org/46303005/diff/100004/net/android/java/src/org...
File net/android/java/src/org/chromium/net/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/100004/net/android/java/src/org...
net/android/java/src/org/chromium/net/ContentUriUtils.java:79: Log.e(TAG,
"Cannot find content uri: " + uriString + ", " + e.toString());
nit: pass 'e' as the third param.

not sure if file not found deserves 'error' level logcat - can it easily happen 
in normal usage?

https://codereview.chromium.org/46303005/diff/100004/net/base/upload_content_...
File net/base/upload_content_url_element_reader_android.cc (right):

https://codereview.chromium.org/46303005/diff/100004/net/base/upload_content_...
net/base/upload_content_url_element_reader_android.cc:20: // This function is
used to implement Init().
the duplication with UploadFileElementReader does not look ideal. Could we
factor out a shared class that just uses FD, and have the FilePath and
"content://" classes be factory-like in function, creating the inner
UploadFileElementReader passing it the appropriate FD?

[qinmin, 2013-10-29 02:45:56]

updated the BUG id, and modified the CL description to reflect the basic work
flow

https://codereview.chromium.org/46303005/diff/100004/net/android/java/src/org...
File net/android/java/src/org/chromium/net/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/100004/net/android/java/src/org...
net/android/java/src/org/chromium/net/ContentUriUtils.java:79: Log.e(TAG,
"Cannot find content uri: " + uriString + ", " + e.toString());
Done. I am not sure how we can get exception on android, maybe rename the file
while uploading is in progress? Changed it to Log.w though

On 2013/10/29 01:30:25, joth wrote:
> nit: pass 'e' as the third param.
> 
> not sure if file not found deserves 'error' level logcat - can it easily
happen 
> in normal usage?

https://codereview.chromium.org/46303005/diff/100004/net/base/upload_content_...
File net/base/upload_content_url_element_reader_android.cc (right):

https://codereview.chromium.org/46303005/diff/100004/net/base/upload_content_...
net/base/upload_content_url_element_reader_android.cc:20: // This function is
used to implement Init().
ok, will do.

On 2013/10/29 01:30:25, joth wrote:
> the duplication with UploadFileElementReader does not look ideal. Could we
> factor out a shared class that just uses FD, and have the FilePath and
> "content://" classes be factory-like in function, creating the inner
> UploadFileElementReader passing it the appropriate FD?

[kinuko, 2013-10-29 02:52:30]

Starting with cosmetic comments + some high-level comments.

https://codereview.chromium.org/46303005/diff/100004/content/browser/child_pr...
File content/browser/child_process_security_policy_impl.h (right):

https://codereview.chromium.org/46303005/diff/100004/content/browser/child_pr...
content/browser/child_process_security_policy_impl.h:257: // bit-set of
base::PlatformFileFlags.
nit: we no longer use base::PlatformFileFlags for these methods but use internal
ChildProcessSecurityPermissions values. Can we fix these comments while you're
there?

https://codereview.chromium.org/46303005/diff/100004/content/browser/loader/u...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/46303005/diff/100004/content/browser/loader/u...
content/browser/loader/upload_data_stream_builder.cc:80: task_runner,
nit: usually we indent not from ':' but from where class name / var name starts
in the previous line

SubClass(foo, bar)
    : BaseClass(
          foo,
          bar)

https://codereview.chromium.org/46303005/diff/100004/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/46303005/diff/100004/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1102:
GetProcess()->GetID(), url);
We receive this as file path and then at some point convert it to GURL if the
path can be interpret as content: URL.

What resources do content: URL refer? Can we always assume it points to a local
file resource? It looks if we delay this conversion point later we can remove
more OS_ANDROID code (e.g. we can change some of local file streams to
understand content: URL if it makes sense). Should we do so to limit the os
specific code or not?

https://codereview.chromium.org/46303005/diff/100004/content/common/resource_...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/46303005/diff/100004/content/common/resource_...
content/common/resource_messages.cc:138: expected_modification_time);
nit: indent is off

[asanka, 2013-10-29 14:08:16]

Redirecting to mmenke for net/

[mmenke, 2013-10-29 14:46:46]

On 2013/10/29 14:08:16, asanka wrote:
> Redirecting to mmenke for net/

Could I get some context here?  "content://" is an Android thing, not a content/
thing?

Warning:  May be a while before I can find time to do the necessary reading to
sign off on this.  Also, it looks like we have no tests for the new code in net/
(Or anywhere else, but I'm specifically concerned about the net case).

[qinmin, 2013-10-29 15:40:46]

On 2013/10/29 14:46:46, mmenke wrote:
> On 2013/10/29 14:08:16, asanka wrote:
> > Redirecting to mmenke for net/
> 
> Could I get some context here?  "content://" is an Android thing, not a
content/
> thing?
> 

Yes, this is an android thing. For android, the file upload dialog may return
files starts with "content://". For example, If selected an image from gallery,
it may return content://media/external/images/media/1.  The content resolver is
responsible for handling the content uri and guarantee the app has permission to
access it. For chrome, most upload requires an actual FilePath(not filesystem
url) object. The FilePath object does not handle content:// scheme correctly, so
I am adding another type here to handle this.

> Warning:  May be a while before I can find time to do the necessary reading to
> sign off on this.  Also, it looks like we have no tests for the new code in
net/
> (Or anywhere else, but I'm specifically concerned about the net case).

[qinmin, 2013-10-29 19:14:20]

https://codereview.chromium.org/46303005/diff/100004/content/browser/child_pr...
File content/browser/child_process_security_policy_impl.h (right):

https://codereview.chromium.org/46303005/diff/100004/content/browser/child_pr...
content/browser/child_process_security_policy_impl.h:257: // bit-set of
base::PlatformFileFlags.
On 2013/10/29 02:52:31, kinuko wrote:
> nit: we no longer use base::PlatformFileFlags for these methods but use
internal
> ChildProcessSecurityPermissions values. Can we fix these comments while you're
> there?

Done.

https://codereview.chromium.org/46303005/diff/100004/content/browser/loader/u...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/46303005/diff/100004/content/browser/loader/u...
content/browser/loader/upload_data_stream_builder.cc:80: task_runner,
On 2013/10/29 02:52:31, kinuko wrote:
> nit: usually we indent not from ':' but from where class name / var name
starts
> in the previous line
> 
> SubClass(foo, bar)
>     : BaseClass(
>           foo,
>           bar)

Done.

https://codereview.chromium.org/46303005/diff/100004/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/46303005/diff/100004/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1102:
GetProcess()->GetID(), url);
For upload case, the content url should point to a local file.
Yes, I think we can delay some of the conversion until we reach the
reading/getFileInfo stage. Also, some of the FilePath member functions are not
applicable to content urls, so I need to bypass those function.

I will do that in the next PS.

On 2013/10/29 02:52:31, kinuko wrote:
> We receive this as file path and then at some point convert it to GURL if the
> path can be interpret as content: URL.
> 
> What resources do content: URL refer? Can we always assume it points to a
local
> file resource? It looks if we delay this conversion point later we can remove
> more OS_ANDROID code (e.g. we can change some of local file streams to
> understand content: URL if it makes sense). Should we do so to limit the os
> specific code or not?

https://codereview.chromium.org/46303005/diff/100004/content/common/resource_...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/46303005/diff/100004/content/common/resource_...
content/common/resource_messages.cc:138: expected_modification_time);
On 2013/10/29 02:52:31, kinuko wrote:
> nit: indent is off

Done.

[qinmin, 2013-10-31 02:02:22]

PTAL again.

As suggested by @kinuko, removed the CONTENT_URL_TYPE enum and using the
FILE_TYPE enum instead.
This simplifies the whole logic and reuses a lot of existing code. #line change
is cut to 1/3 of the first patchset.

[kinuko, 2013-10-31 06:10:50]

Thanks for trying to simplify this. I like this change much better than the
previous one, but you'll first need to have base/ and net/base/ part reviewed
(as usually we have stricter review policy in these directories).

https://codereview.chromium.org/46303005/diff/290001/base/files/file_path.h
File base/files/file_path.h (right):

https://codereview.chromium.org/46303005/diff/290001/base/files/file_path.h#n...
base/files/file_path.h:391: // Returns true if the path is a content url, which
starts with content://.
Would be nice to add a few more (generic) comment about content:// url on
android.

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
File net/base/upload_file_element_reader.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
net/base/upload_file_element_reader.cc:53: int64 length = std::max(0ll,
GetContentUrlLengthSync(path));
Can this part be factored out as a separate static method (e.g.
GetLengthFromPath(path, &length)) so that we don't need to have #if..else..endif
in the main code flow?

[joth, 2013-10-31 06:42:38]

Yes this feels a much more natural fit, thanks for seeing that change through.

Some questions / suggestions - main one is really for the base/ OWNERS to
decide: should more of the content:// special casing move into file_utils?
(if it did, I think a lot of AndroidProtocolHandler in android_webview could be
collapsed just to using normal FilePath flows too, eventually)

https://codereview.chromium.org/46303005/diff/290001/content/browser/fileapi/...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/46303005/diff/290001/content/browser/fileapi/...
content/browser/fileapi/fileapi_message_filter.cc:945: request_id, info,
platform_path));
ubernit: indent one char more. (align with open paren)

https://codereview.chromium.org/46303005/diff/290001/content/browser/renderer...
File content/browser/renderer_host/file_utilities_message_filter.cc (right):

https://codereview.chromium.org/46303005/diff/290001/content/browser/renderer...
content/browser/renderer_host/file_utilities_message_filter.cc:55: #if
defined(OS_ANDROID)
whats the reason to do this here, and not inside file_util::GetFileInfo() ?

https://codereview.chromium.org/46303005/diff/290001/content/browser/renderer...
content/browser/renderer_host/file_utilities_message_filter.cc:58: // to extract
information other than size.
nit: populate PlatformFileInfo fields other than size.

I think you can just mark is_directory & is_symbolic_link false anyway..

(could mention opening FD, using fstat and factoring out from GetFileInfo() is
probably the answer)

https://codereview.chromium.org/46303005/diff/290001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/290001/net/android/java/src/org...
net/android/java/src/org/chromium/net/ContentUriUtils.java:18: public abstract
class ContentUriUtils {
if this class is only used from native, no need to be public.

https://codereview.chromium.org/46303005/diff/290001/net/android/java/src/org...
net/android/java/src/org/chromium/net/ContentUriUtils.java:55: long size =
pfd.getStatSize();
oh right, if you're just opening the FD then calling fd.getStatSize() over here
on the java side, this file and it's native peer would be way simpler if you
implement this all in native...

file_util::GetFileInfo() would just call getParcelFileDescriptor() and use fstat
on the returned fd. You only need one method in here then. (I think)

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
File net/base/file_stream_context_android.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
net/base/file_stream_context_android.cc:27:
base::Bind(&Context::OpenContentUrlImpl, base::Unretained(this),
the unretained is slightly scary but I see file_stream_context.cc does it
everywhere so I assume it must be safe! :)

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
File net/base/upload_file_element_reader.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
net/base/upload_file_element_reader.cc:53: int64 length = std::max(0ll,
GetContentUrlLengthSync(path));
On 2013/10/31 06:10:51, kinuko wrote:
> Can this part be factored out as a separate static method (e.g.
> GetLengthFromPath(path, &length)) so that we don't need to have
#if..else..endif
> in the main code flow?

+1

Maybe even, bundling this logic into file_util::GetFileSize() would mean no
special content:// logic here at all. 


Was a design goal of this patch explicitly not to add generic content:// support
in file_util? else I could easily imagine having a generic
file_util::OpenFileDescriptor(FilePath) method and the content_uri_utils call
would just happen inside that if appropriate.

https://codereview.chromium.org/46303005/diff/290001/webkit/browser/blob/loca...
File webkit/browser/blob/local_file_stream_reader.cc (right):

https://codereview.chromium.org/46303005/diff/290001/webkit/browser/blob/loca...
webkit/browser/blob/local_file_stream_reader.cc:73: #endif
...again if file_util::GetFileInfo() knew how to deal with content:// path then
this edit would not be needed at all

[mmenke, 2013-10-31 15:07:06]

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
File net/base/file_stream_context_android.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
net/base/file_stream_context_android.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
file_stream_context.cc is small enough that I think it's reasonable to put this
stuff in it instead of in a separate file.  Makes it less likely someone will
miss this code when refactoring the class.

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
net/base/file_stream_context_android.cc:27:
base::Bind(&Context::OpenContentUrlImpl, base::Unretained(this),
On 2013/10/31 06:42:39, joth wrote:
> the unretained is slightly scary but I see file_stream_context.cc does it
> everywhere so I assume it must be safe! :)

The FileStreamContext basically owns itself, and will clean up properly when
deleted...But I agree, is rather weird and scary.

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
File net/base/upload_file_element_reader.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
net/base/upload_file_element_reader.cc:55: range_offset <
static_cast<uint64>(length)) {
This class is being modified to handle errors differently, see
https://codereview.chromium.org/39203004/diff/670001/net/base/upload_file_ele....
 If we keep the split logic here, whichever lands second will have to make this
return an error on the fail-to-get-length case.

I'm a fan of joth's idea to mover the logic into file_util, if we can, though.

[qinmin, 2013-11-05 01:41:29]

PTAL again. 
Moved most of the changes to base/, +thakis for OWNER

https://codereview.chromium.org/46303005/diff/290001/base/files/file_path.h
File base/files/file_path.h (right):

https://codereview.chromium.org/46303005/diff/290001/base/files/file_path.h#n...
base/files/file_path.h:391: // Returns true if the path is a content url, which
starts with content://.
On 2013/10/31 06:10:51, kinuko wrote:
> Would be nice to add a few more (generic) comment about content:// url on
> android.

Done.

https://codereview.chromium.org/46303005/diff/290001/content/browser/fileapi/...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/46303005/diff/290001/content/browser/fileapi/...
content/browser/fileapi/fileapi_message_filter.cc:945: request_id, info,
platform_path));
On 2013/10/31 06:42:39, joth wrote:
> ubernit: indent one char more. (align with open paren)

Done.

https://codereview.chromium.org/46303005/diff/290001/content/browser/renderer...
File content/browser/renderer_host/file_utilities_message_filter.cc (right):

https://codereview.chromium.org/46303005/diff/290001/content/browser/renderer...
content/browser/renderer_host/file_utilities_message_filter.cc:55: #if
defined(OS_ANDROID)
On 2013/10/31 06:42:39, joth wrote:
> whats the reason to do this here, and not inside file_util::GetFileInfo() ?

Done.

https://codereview.chromium.org/46303005/diff/290001/content/browser/renderer...
content/browser/renderer_host/file_utilities_message_filter.cc:58: // to extract
information other than size.
On 2013/10/31 06:42:39, joth wrote:
> nit: populate PlatformFileInfo fields other than size.
> 
> I think you can just mark is_directory & is_symbolic_link false anyway..
> 
> (could mention opening FD, using fstat and factoring out from GetFileInfo() is
> probably the answer)

Done.

https://codereview.chromium.org/46303005/diff/290001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/290001/net/android/java/src/org...
net/android/java/src/org/chromium/net/ContentUriUtils.java:18: public abstract
class ContentUriUtils {
On 2013/10/31 06:42:39, joth wrote:
> if this class is only used from native, no need to be public.

Done.

https://codereview.chromium.org/46303005/diff/290001/net/android/java/src/org...
net/android/java/src/org/chromium/net/ContentUriUtils.java:55: long size =
pfd.getStatSize();
Done. Changed the GetFileInfo to call openContentUriForRead() to get the fd,
then call fstat to get the FileInfo.

On 2013/10/31 06:42:39, joth wrote:
> oh right, if you're just opening the FD then calling fd.getStatSize() over
here
> on the java side, this file and it's native peer would be way simpler if you
> implement this all in native...
> 
> file_util::GetFileInfo() would just call getParcelFileDescriptor() and use
fstat
> on the returned fd. You only need one method in here then. (I think)

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
File net/base/file_stream_context_android.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/file_stream_con...
net/base/file_stream_context_android.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/10/31 15:07:06, mmenke wrote:
> file_stream_context.cc is small enough that I think it's reasonable to put
this
> stuff in it instead of in a separate file.  Makes it less likely someone will
> miss this code when refactoring the class.

Done.

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
File net/base/upload_file_element_reader.cc (right):

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
net/base/upload_file_element_reader.cc:53: int64 length = std::max(0ll,
GetContentUrlLengthSync(path));
Done. GetFileInfo() should work for content url now. GetFileSize() calls
GetFileInfo(), so it will work too.
On 2013/10/31 06:42:39, joth wrote:
> On 2013/10/31 06:10:51, kinuko wrote:
> > Can this part be factored out as a separate static method (e.g.
> > GetLengthFromPath(path, &length)) so that we don't need to have
> #if..else..endif
> > in the main code flow?
> 
> +1
> 
> Maybe even, bundling this logic into file_util::GetFileSize() would mean no
> special content:// logic here at all. 
> 
> 
> Was a design goal of this patch explicitly not to add generic content://
support
> in file_util? else I could easily imagine having a generic
> file_util::OpenFileDescriptor(FilePath) method and the content_uri_utils call
> would just happen inside that if appropriate.

https://codereview.chromium.org/46303005/diff/290001/net/base/upload_file_ele...
net/base/upload_file_element_reader.cc:55: range_offset <
static_cast<uint64>(length)) {
On 2013/10/31 15:07:06, mmenke wrote:
> This class is being modified to handle errors differently, see
>
https://codereview.chromium.org/39203004/diff/670001/net/base/upload_file_ele....
>  If we keep the split logic here, whichever lands second will have to make
this
> return an error on the fail-to-get-length case.
> 
> I'm a fan of joth's idea to mover the logic into file_util, if we can, though.

Done.

https://codereview.chromium.org/46303005/diff/290001/webkit/browser/blob/loca...
File webkit/browser/blob/local_file_stream_reader.cc (right):

https://codereview.chromium.org/46303005/diff/290001/webkit/browser/blob/loca...
webkit/browser/blob/local_file_stream_reader.cc:73: #endif
On 2013/10/31 06:42:39, joth wrote:
> ...again if file_util::GetFileInfo() knew how to deal with content:// path
then
> this edit would not be needed at all

Done.

[mmenke, 2013-11-05 02:33:18]

https://codereview.chromium.org/46303005/diff/560001/net/base/file_stream_con...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/46303005/diff/560001/net/base/file_stream_con...
net/base/file_stream_context.cc:76: #endif
Can't you just move all content URL code  down into OpenFileImpl?

[qinmin, 2013-11-05 03:38:40]

https://codereview.chromium.org/46303005/diff/560001/net/base/file_stream_con...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/46303005/diff/560001/net/base/file_stream_con...
net/base/file_stream_context.cc:76: #endif
On 2013/11/05 02:33:19, mmenke wrote:
> Can't you just move all content URL code  down into OpenFileImpl?

Done.

[mmenke, 2013-11-05 15:36:03]

My only remaining concern is unit tests - how easy is it to set up a content URL
for tests?

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
net/base/file_stream_context.cc:214: file = base::CreatePlatformFile(path,
open_flags, NULL, NULL);
nit:  Think this is easier to read if this block is indented.

[joth, 2013-11-06 00:27:07]

lgtm for the bits I'm OWNER of...

https://codereview.chromium.org/46303005/diff/560001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/560001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:72: Log.w(TAG,
"Cannot find content uri: " + uriString + ", " + e);
nit: pass 'e' as the third param to Log.w()

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc#...
base/file_util_posix.cc:594: if (CallStat(file_path.value().c_str(), &file_info)
!= 0)
It looks like CallStat() is already intended to have different implementation
per-platform up top of file.
Can't we push the ifdef logic into that?

[ericu, 2013-11-06 00:42:10]

I see no problems w.r.t. filesystem code.

Bikeshedding: Are these content URIs or content URLs?  You use both names...this
could be correct if you anticipate all local file references to be URLs, but the
utils might at some point add members that deal with non-URL URIs, but is that
the case?

[kinuko, 2013-11-06 01:42:05]

lgtm for fileapi related files. (Wow the CL's gotten much smaller now)

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc#...
base/file_util_posix.cc:321: }
nit: usually we use no { } for single-line block in chromium

[joth, 2013-11-06 01:49:17]

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
net/base/file_stream_context.cc:217: #endif  // defined(OS_ANDROID)
just saw in another review [1], you can avoid the second ifdef here using this
pattern

#if defined(...)
if (xyz) {
  foo();
} else
#endif
{
  bar();
}



[1] -
https://codereview.chromium.org/58203002/diff/120001/content/browser/child_pr...

[joth, 2013-11-06 19:34:24]

On 2013/11/06 01:49:17, joth wrote:
>
https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
> File net/base/file_stream_context.cc (right):
> 
>
https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
> net/base/file_stream_context.cc:217: #endif  // defined(OS_ANDROID)
> just saw in another review [1], you can avoid the second ifdef here using this
> pattern
> 
> #if defined(...)
> if (xyz) {
>   foo();
> } else
> #endif
> {
>   bar();
> }
> 
> 
> 
> [1] -
>
https://codereview.chromium.org/58203002/diff/120001/content/browser/child_pr...

OK forget that... jar@ just took issue with that very bit of code
https://codereview.chromium.org/58203002/diff/320001/content/browser/child_pr...
I don't feel strongly on it either way. (ifdefs are ugly and usage of them
deserves to look ugly...!)

[qinmin, 2013-11-07 01:10:58]

On 2013/11/06 00:42:10, ericu wrote:
> I see no problems w.r.t. filesystem code.
> 
> Bikeshedding: Are these content URIs or content URLs?  You use both
names...this
> could be correct if you anticipate all local file references to be URLs, but
the
> utils might at some point add members that deal with non-URL URIs, but is that
> the case?

Currently there is no plan to deal with non-URL URIs. However, I have changed
all the functions to use URI to be consistent.

[qinmin, 2013-11-07 01:13:34]

Added new unit tests for changes in file_path.cc and file_utils.cc. To make the
test to work, first insert an image file into MediaStore, and then we can
retrieve the content URI for testing.

https://codereview.chromium.org/46303005/diff/560001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/560001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:72: Log.w(TAG,
"Cannot find content uri: " + uriString + ", " + e);
On 2013/11/06 00:27:08, joth wrote:
> nit: pass 'e' as the third param to Log.w()

Done.

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc#...
base/file_util_posix.cc:321: }
On 2013/11/06 01:42:05, kinuko wrote:
> nit: usually we use no { } for single-line block in chromium

Done.

https://codereview.chromium.org/46303005/diff/610001/base/file_util_posix.cc#...
base/file_util_posix.cc:594: if (CallStat(file_path.value().c_str(), &file_info)
!= 0)
I think I would rather keep it separate. CallStat() will set errno, so we have
to do the same when fd < 0 and it looks ugly. If other platforms later decided
to add Fstat(), they can just reuse the FStat() function we added. Having
something like file_path.IsContentUrl() inside CallStat() makes it not reusable.

On 2013/11/06 00:27:08, joth wrote:
> It looks like CallStat() is already intended to have different implementation
> per-platform up top of file.
> Can't we push the ifdef logic into that?

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
net/base/file_stream_context.cc:214: file = base::CreatePlatformFile(path,
open_flags, NULL, NULL);
On 2013/11/05 15:36:04, mmenke wrote:
> nit:  Think this is easier to read if this block is indented.

Done.

https://codereview.chromium.org/46303005/diff/610001/net/base/file_stream_con...
net/base/file_stream_context.cc:217: #endif  // defined(OS_ANDROID)
On 2013/11/06 01:49:17, joth wrote:
> just saw in another review [1], you can avoid the second ifdef here using this
> pattern
> 
> #if defined(...)
> if (xyz) {
>   foo();
> } else
> #endif
> {
>   bar();
> }
> 
> 
> 
> [1] -
>
https://codereview.chromium.org/58203002/diff/120001/content/browser/child_pr...

Done.

[qinmin, 2013-11-07 01:15:32]

+jar@ for OWNER for base/, thakis@ is on vacation

[mmenke, 2013-11-07 16:31:54]

Just some test suggestions, and I'm happy.

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest....
base/file_util_unittest.cc:2322: #if defined(OS_ANDROID)
Test suggestion:  Test the case where we get an error when opening the file.

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest....
base/file_util_unittest.cc:2322: #if defined(OS_ANDROID)
Test suggestion:  I'd like a net test that uses a content URL successfully as
well (Don't think we really need the failure case tested in net/, though I
wouldn't object to it).

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest....
base/file_util_unittest.cc:2340: EXPECT_LT(0, size);
Suggest reading a non-zero size file and checking its contents.  Could
optionally do it in another test, just so we have the empty file case covered as
well.

[qinmin, 2013-11-08 23:03:02]

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest....
base/file_util_unittest.cc:2322: #if defined(OS_ANDROID)
On 2013/11/07 16:31:55, mmenke wrote:
> Test suggestion:  Test the case where we get an error when opening the file.

Done.

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest....
base/file_util_unittest.cc:2322: #if defined(OS_ANDROID)
added a test in net/ for file_stream.
On 2013/11/07 16:31:55, mmenke wrote:
> Test suggestion:  I'd like a net test that uses a content URL successfully as
> well (Don't think we really need the failure case tested in net/, though I
> wouldn't object to it).

https://codereview.chromium.org/46303005/diff/740021/base/file_util_unittest....
base/file_util_unittest.cc:2340: EXPECT_LT(0, size);
The android mediastore won't accept empty file. Added a comparison of the
original file size to the content uri size.

On 2013/11/07 16:31:55, mmenke wrote:
> Suggest reading a non-zero size file and checking its contents.  Could
> optionally do it in another test, just so we have the empty file case covered
as
> well.

[qinmin, 2013-11-08 23:04:00]

jar@, would you please review base/? as suggested by all the reviewers, most of
the changes have been moved into base/

jam@, would you please take a look at content/?


thanks

[jar (doing other things), 2013-11-11 00:34:47]

I'm not familiar with the security model, and promises around file upload, so
I'd rather have folks from the security team review this and agree that there
are no security ramifications.

https://codereview.chromium.org/46303005/diff/1010001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/1010001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:62: * @returns
ParcelFileDescriptor of the content URI, or NULL if the file does not exist.
nit: Does style guide for Java suggest 80 character limit?

[mmenke, 2013-11-11 16:21:14]

net/ LGTM

[qinmin, 2013-11-11 17:57:51]

+tsepez for security review.

tsepez@: hi, Tom, would you please do a security review for this CL? On android,
the upload file selection dialog may return content uri schemes, so this CL
ensures that we are able to read the content URI.

https://codereview.chromium.org/46303005/diff/1010001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/46303005/diff/1010001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ContentUriUtils.java:62: * @returns
ParcelFileDescriptor of the content URI, or NULL if the file does not exist.
Java coding style uses 100 char limit.

On 2013/11/11 00:34:48, jar wrote:
> nit: Does style guide for Java suggest 80 character limit?

[Tom Sepez, 2013-11-11 18:28:33]

LGTM with comments.

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path.cc...
base/files/file_path.cc:1285: return StartsWithASCII(path_, "content://",
false);
Are there other ways to "spell" content:// ? content%2a%2f%2f or such? 
nit: comment 3rd parameter, as in
return StartsWithASCII(path_, "content://", false /*case_sensitive*/);

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path_un...
File base/files/file_path_unittest.cc (right):

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path_un...
base/files/file_path_unittest.cc:1238: { FPL(".. "),                false },
nit: how about adding some near misses:
content:foo.bar
content:/foo.bar
content:/dir/foo.bar
content:///foo.bar
CoNTenT://foo.bar
content: //foo.bar

https://codereview.chromium.org/46303005/diff/1010001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.h (right):

https://codereview.chromium.org/46303005/diff/1010001/content/browser/child_p...
content/browser/child_process_security_policy_impl.h:182: //
ChildProcessSecurityPermissions.
Thanks for taking a stab at fixing this comment.  But
ChildProcessSecurityPermissions is declared in the .cc file, and shouldn't
appear here, since no public caller will be able to know about them. There's a
problem with the way this interface has evolved that is orthogonal to your CL. 
Please file a follow-up bug about this, assign it to tommycli@, and take this
out of your CL.  I'd like to see this fixed independently.

https://codereview.chromium.org/46303005/diff/1010001/content/browser/fileapi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/46303005/diff/1010001/content/browser/fileapi...
content/browser/fileapi/fileapi_message_filter.cc:66: child_id, path);
Whitespace only changes in this file?  You can probably drop it from your CL and
let the next CL that touches this deal with the cleanup.

[qinmin, 2013-11-11 20:49:52]

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path.cc...
base/files/file_path.cc:1285: return StartsWithASCII(path_, "content://",
false);
Done fixing the 3rd parameter comments.
The Java side file selection dialog will return content://, so we should not
take content%2a%2f%2f. Added a unit test for this.


On 2013/11/11 18:28:34, Tom Sepez wrote:
> Are there other ways to "spell" content:// ? content%2a%2f%2f or such? 
> nit: comment 3rd parameter, as in
> return StartsWithASCII(path_, "content://", false /*case_sensitive*/);

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path_un...
File base/files/file_path_unittest.cc (right):

https://codereview.chromium.org/46303005/diff/1010001/base/files/file_path_un...
base/files/file_path_unittest.cc:1238: { FPL(".. "),                false },
On 2013/11/11 18:28:34, Tom Sepez wrote:
> nit: how about adding some near misses:
> content:foo.bar
> content:/foo.bar
> content:/dir/foo.bar
> content:///foo.bar
> CoNTenT://foo.bar
> content: //foo.bar

Done.

https://codereview.chromium.org/46303005/diff/1010001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.h (right):

https://codereview.chromium.org/46303005/diff/1010001/content/browser/child_p...
content/browser/child_process_security_policy_impl.h:182: //
ChildProcessSecurityPermissions.
Done. crbug/317814 filed to track this.

On 2013/11/11 18:28:34, Tom Sepez wrote:
> Thanks for taking a stab at fixing this comment.  But
> ChildProcessSecurityPermissions is declared in the .cc file, and shouldn't
> appear here, since no public caller will be able to know about them. There's a
> problem with the way this interface has evolved that is orthogonal to your CL.

> Please file a follow-up bug about this, assign it to tommycli@, and take this
> out of your CL.  I'd like to see this fixed independently.

https://codereview.chromium.org/46303005/diff/1010001/content/browser/fileapi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/46303005/diff/1010001/content/browser/fileapi...
content/browser/fileapi/fileapi_message_filter.cc:66: child_id, path);
On 2013/11/11 18:28:34, Tom Sepez wrote:
> Whitespace only changes in this file?  You can probably drop it from your CL
and
> let the next CL that touches this deal with the cleanup.

Done.

[qinmin, 2013-11-12 17:39:06]

Hi, jar@, tsepez lgtm'ed the CL, would you please do a OWNERS review?  thanks

[jar (doing other things), 2013-11-13 01:03:27]

With the security reviewed... base LGTM (with a few lingering questions below).

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path.cc...
base/files/file_path.cc:1289: ((path_[len] >= L'A' && path_[len] <= L'Z') ||
Why do you care if the path (authority?) begins with a letter?  Shouldn't you be
as general as possible, and assume anything with a content:// scheme is
potentially a content URI?  (...and presumably gets more care in handling and
access?)

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path_un...
File base/files/file_path_unittest.cc (right):

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path_un...
base/files/file_path_unittest.cc:1237: { FPL("CoNTenT://foo.bar"),    true },
Since you wrote code to distinguish the presence of a first letter... you
probably should have the test:


{ FPL("content://3foo/bar"),    false},
{ FPL("content://_foo/bar"),    false},

As noted... I wasn't sure why you wanted to define it this way.... but that
seemed to be an intentional set of cases.

[qinmin, 2013-11-13 23:42:42]

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path.cc...
base/files/file_path.cc:1289: ((path_[len] >= L'A' && path_[len] <= L'Z') ||
On 2013/11/13 01:03:28, jar wrote:
> Why do you care if the path (authority?) begins with a letter?  Shouldn't you
be
> as general as possible, and assume anything with a content:// scheme is
> potentially a content URI?  (...and presumably gets more care in handling and
> access?)

Done, removed the path check.

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path_un...
File base/files/file_path_unittest.cc (right):

https://codereview.chromium.org/46303005/diff/1230001/base/files/file_path_un...
base/files/file_path_unittest.cc:1237: { FPL("CoNTenT://foo.bar"),    true },
added the above tests, since the first letter check is now removed, changed the
expected result from these tests.
The app will handle invalid content uris.

On 2013/11/13 01:03:28, jar wrote:
> Since you wrote code to distinguish the presence of a first letter... you
> probably should have the test:
> 
> 
> { FPL("content://3foo/bar"),    false},
> { FPL("content://_foo/bar"),    false},
> 
> As noted... I wasn't sure why you wanted to define it this way.... but that
> seemed to be an intentional set of cases.
>

[qinmin, 2013-11-13 23:44:31]

jam@, would you please take a look at
content/browser/child_process_security_policy_impl.cc? tsepez has done the
security review.

+yfriedman for ui/android OWNERs

[jam, 2013-11-14 18:15:25]

On 2013/11/13 23:44:31, qinmin wrote:
> jam@, would you please take a look at
> content/browser/child_process_security_policy_impl.cc? tsepez has done the
> security review.

tsepez is an owner for that file, removing myself
> 
> +yfriedman for ui/android OWNERs

[commit-bot: I haz the power, 2013-11-14 18:20:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-14 19:41:46]

Retried try job too often on linux_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-14 21:26:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-14 21:44:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-15 01:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-15 03:47:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-15 06:44:51]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 07:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-15 14:05:13]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-11-15 14:43:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-15 21:49:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/1560001

[commit-bot: I haz the power, 2013-11-15 21:49:50]

Failed to apply patch for
ui/android/java/src/org/chromium/ui/SelectFileDialog.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: ui/android/java/src/org/chromium/ui/SelectFileDialog.java
  |diff --git a/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
b/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
  |index
d313681e03e7d8feb81855e7f81fc8d9d5a31fb1..bb53c04f3780a8232dc69490a55a211e26ad998c
100644
  |--- a/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
  |+++ b/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       ui/android/java/src/org/chromium/ui/SelectFileDialog.java
Index: ui/android/java/src/org/chromium/ui/SelectFileDialog.java
diff --git a/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
b/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
index
d313681e03e7d8feb81855e7f81fc8d9d5a31fb1..bb53c04f3780a8232dc69490a55a211e26ad998c
100644
--- a/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
+++ b/ui/android/java/src/org/chromium/ui/SelectFileDialog.java
@@ -164,27 +164,11 @@ class SelectFileDialog implements
WindowAndroid.IntentCallback{
             return;
         }
 
-        Cursor cursor = null;
-        try {
-            // We get back a content:// URI from the system if the user picked
a file from the
-            // gallery. The ContentView has functionality that will convert
that content:// URI to
-            // a file path on disk that Chromium understands.
-            cursor = contentResolver.query(results.getData(),
-                    new String[] { MediaStore.MediaColumns.DATA }, null, null,
null);
-            if (cursor != null) {
-                if (cursor.getCount() == 1) {
-                    cursor.moveToFirst();
-                    String path = cursor.getString(0);
-                    if (path != null) {
-                        // Not all providers support the MediaStore.DATA
column. For example,
-                        // Gallery3D (com.android.gallery3d.provider) does not
support it for
-                        // Picasa Web Album images.
-                        nativeOnFileSelected(mNativeSelectFileDialog, path);
-                        return;
-                    }
-                }
-            }
-        } finally { if (cursor != null) { cursor.close(); } }
+        if (results.getScheme() != null
+                && results.getScheme().equals(ContentResolver.SCHEME_CONTENT))
{
+            nativeOnFileSelected(mNativeSelectFileDialog,
results.getData().toString());
+            return;
+        }
 
         onFileNotSelected();
         window.showError(R.string.opening_file_error);

[commit-bot: I haz the power, 2013-11-15 23:48:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/2120001

[commit-bot: I haz the power, 2013-11-16 00:15:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/2120001

[commit-bot: I haz the power, 2013-11-16 11:27:06]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-11-17 06:33:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/2120001

[commit-bot: I haz the power, 2013-11-17 12:02:01]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-11-17 22:39:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/46303005/2120001

[commit-bot: I haz the power, 2013-11-18 03:53:03]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[qinmin, 2013-11-18 17:21:33]

Committed patchset #10 manually as r235752 (presubmit successful).