Support content scheme uri for Chrome on Android

Support content scheme uri for Chrome on Android

Android uses content scheme to store files and ensure permission checks.
For example, the downloaded files are stored as content://downloads/all_downloads/123.
However, chrome currently cannot handle url requests for content uri.
As a result, chrome can save html pages to sdcard, but cannot open it.
This change adds the content scheme support for chrome on android.

BUG=433011
Committed: https://crrev.com/120a15519703dfe8601596f1f436a322ea0a2aff
Cr-Commit-Position: refs/heads/master@{#305772}

[qinmin, 2014-11-18 23:53:57]

qinmin@chromium.org changed reviewers:
+ asanka@chromium.org, nyquist@chromium.org, thakis@chromium.org

[qinmin, 2014-11-18 23:53:58]

PTAL

thakis for base/ and chrome/ OWNER
asanka for net/ OWNER
nyquist for base/android OWNER

[qinmin, 2014-11-18 23:54:33]

qinmin@chromium.org changed reviewers:
+ palmer@chromium.org

[Nico, 2014-11-18 23:56:35]

It looks wrong to me that base knows about content:// stuff. Shouldn't the
content uri stuff be in, well, content/?

[Nico, 2014-11-19 00:01:50]

On 2014/11/18 23:56:35, Nico wrote:
> It looks wrong to me that base knows about content:// stuff. Shouldn't the
> content uri stuff be in, well, content/?

I have been informed off-thread that content:// urls have nothing whatsoever to
do with code in content/. Ignore my remark :-)

[nyquist, 2014-11-19 00:02:30]

content:// in Android sadly is not the same as //content in Chrome (even though
//chrome contains code for chrome://).

content:// URIs in Android is a way for indirectly reading content, for example
files from disk. See
http://developer.android.com/guide/topics/providers/content-provider-basics.html
for a basic introduction.

https://codereview.chromium.org/739033003/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/739033003/diff/1/base/base.gyp#newcode438
base/base.gyp:438: 'android/content_uri_utils_unittest.cc',
I think you need to update //base/BUILD.gn as well. It also has a base_unittests
target.

[Nico, 2014-11-19 00:06:03]

thakis@chromium.org changed reviewers:
+ noms@chromium.org

[Nico, 2014-11-19 00:06:03]

base/ lgtm (with the gn change nyquist asked for)

+noms for the chrome/browser/profiles change.

https://codereview.chromium.org/739033003/diff/1/url/url_constants.h
File url/url_constants.h (right):

https://codereview.chromium.org/739033003/diff/1/url/url_constants.h#newcode16
url/url_constants.h:16: URL_EXPORT extern const char kContentScheme[];
If this is some android concept unrelated to content/, maybe this should have a
comment saying so

[nyquist, 2014-11-19 00:06:25]

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:85: * Retrieve the
mime type for the content URI.
Nit: MIME here and below?

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:88: * @param
uriString the content URI to open.
It feels a bit off that we say 'open' here. Maybe 'inspect' or 'look up'? Or
"the content URI to get the MIME type for"?

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:89: * @returns mime
type or an empty string if the input params are invalid.
This leads to believe that the method can never return null, which is wrong. It
still returns null if the URL is invalid (I am assuming they also mean
non-existing?).

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:96: return
resolver.getType(uri);
Do we want to ensure that this never returns null?

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:115: } catch
(java.lang.SecurityException e) {
Do you really need the 'java.lang.' prefix here?

[palmer, 2014-11-19 00:23:54]

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
File base/android/content_uri_utils.cc (right):

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
base/android/content_uri_utils.cc:38: std::string GetContentUriMimeType(const
FilePath& content_uri) {
The name "...uri" seems to conflict with the type FilePath. I'd expect a thing
named "...uri" to be a GURL or other URL/URI type. Is |content_uri| the path
component of a URL?

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
File base/android/content_uri_utils.h (right):

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
base/android/content_uri_utils.h:26: // Gets mime type from a content uri.
Returns an empty string if the uri is
Nit: Uppercase for acronyms ("MIME", "URI", et c).

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:89: * @returns mime
type or an empty string if the input params are invalid.
FWIW, The Java Way would be to throw, not return, to signal error.

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:115: } catch
(java.lang.SecurityException e) {
Nope.

https://codereview.chromium.org/739033003/diff/1/net/url_request/file_protoco...
File net/url_request/file_protocol_handler.cc (right):

https://codereview.chromium.org/739033003/diff/1/net/url_request/file_protoco...
net/url_request/file_protocol_handler.cc:29: request, network_delegate,
base::FilePath(request->url().spec()),
Will content://data/data/org.chromium.blah/foo/Profiles/Cookies work? I'm
concerned that content:// might be a way to bypass any safety checks applied to
file: URLs.

[qinmin, 2014-11-19 02:13:08]

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
File base/android/content_uri_utils.cc (right):

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
base/android/content_uri_utils.cc:38: std::string GetContentUriMimeType(const
FilePath& content_uri) {
On 2014/11/19 00:23:54, Chromium Palmer wrote:
> The name "...uri" seems to conflict with the type FilePath. I'd expect a thing
> named "...uri" to be a GURL or other URL/URI type. Is |content_uri| the path
> component of a URL?

There are some historic reasons. The previous patches about content URI are file
upload cases. We treat content URIs as a file, reading the content and get the
data. So content URI is currently only used in file related classes,  such as
FileStreamContext and FileUtil, e.g. Additionally, there are operations like
Open() on content URI to get file desciptors. That makes it more like a FilePath
rather than a GURL. So we just treat the "content://" scheme the same as a drive
letter on Windows.

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
File base/android/content_uri_utils.h (right):

https://codereview.chromium.org/739033003/diff/1/base/android/content_uri_uti...
base/android/content_uri_utils.h:26: // Gets mime type from a content uri.
Returns an empty string if the uri is
On 2014/11/19 00:23:54, Chromium Palmer wrote:
> Nit: Uppercase for acronyms ("MIME", "URI", et c).

Done.

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/ContentUriUtils.java (right):

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:85: * Retrieve the
mime type for the content URI.
On 2014/11/19 00:06:25, nyquist wrote:
> Nit: MIME here and below?

Done.

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:88: * @param
uriString the content URI to open.
On 2014/11/19 00:06:25, nyquist wrote:
> It feels a bit off that we say 'open' here. Maybe 'inspect' or 'look up'? Or
> "the content URI to get the MIME type for"?

Done.

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:89: * @returns mime
type or an empty string if the input params are invalid.
On 2014/11/19 00:06:25, nyquist wrote:
> This leads to believe that the method can never return null, which is wrong.
It
> still returns null if the URL is invalid (I am assuming they also mean
> non-existing?).

Ok, changed the description to just return NULL on error and invalid case.

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:89: * @returns mime
type or an empty string if the input params are invalid.
On 2014/11/19 00:23:54, Chromium Palmer wrote:
> FWIW, The Java Way would be to throw, not return, to signal error.

This method is called by native, so either the java side or the native side has
to handle the exception.
Since contentResolver silently returns a NULL if uri is invalid, we will just
return null for empty input params.

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:96: return
resolver.getType(uri);
On 2014/11/19 00:06:25, nyquist wrote:
> Do we want to ensure that this never returns null?

returned null for the empty inputs case

https://codereview.chromium.org/739033003/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/ContentUriUtils.java:115: } catch
(java.lang.SecurityException e) {
On 2014/11/19 00:06:25, nyquist wrote:
> Do you really need the 'java.lang.' prefix here?

removed

https://codereview.chromium.org/739033003/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/739033003/diff/1/base/base.gyp#newcode438
base/base.gyp:438: 'android/content_uri_utils_unittest.cc',
On 2014/11/19 00:02:30, nyquist wrote:
> I think you need to update //base/BUILD.gn as well. It also has a
base_unittests
> target.

Done.

https://codereview.chromium.org/739033003/diff/1/net/url_request/file_protoco...
File net/url_request/file_protocol_handler.cc (right):

https://codereview.chromium.org/739033003/diff/1/net/url_request/file_protoco...
net/url_request/file_protocol_handler.cc:29: request, network_delegate,
base::FilePath(request->url().spec()),
On 2014/11/19 00:23:54, Chromium Palmer wrote:
> Will content://data/data/org.chromium.blah/foo/Profiles/Cookies work? I'm
> concerned that content:// might be a way to bypass any safety checks applied
to
> file: URLs.

no, android ContentResolver has it's own permission controls and it won't allow
chrome to access the data.

https://codereview.chromium.org/739033003/diff/1/url/url_constants.h
File url/url_constants.h (right):

https://codereview.chromium.org/739033003/diff/1/url/url_constants.h#newcode16
url/url_constants.h:16: URL_EXPORT extern const char kContentScheme[];
On 2014/11/19 00:06:03, Nico wrote:
> If this is some android concept unrelated to content/, maybe this should have
a
> comment saying so

Done.

[qinmin, 2014-11-20 17:42:01]

qinmin@chromium.org changed reviewers:
+ brettw@chromium.org

[qinmin, 2014-11-20 17:42:02]

+brettw for url/ OWNER

@nyquist, @asanka, @palmer, would you please take another look?

[brettw, 2014-11-20 18:50:37]

url lgtm

[palmer, 2014-11-20 19:04:47]

LGTM

[nyquist, 2014-11-20 20:01:11]

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

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:33: public Uri
getContentUriFromFile(Context context, File file);
This is implicitly public.

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:78: if (pfd ==
null) {
Optional: This bugs me; feel free to change this to return pfd != null. You
could at that point also inline pfd.

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:89: * @returns MIME
type or null if the input params are empty or invalid.
This should be @return (throughout this file)

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:95: if (resolver ==
null || uri == null) return null;
Uri.parse(...) never returns null (specifically mentioned in the API).

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:113: } catch
(java.io.FileNotFoundException e) {
could you change this one to import FileNotFoundException as well?

[qinmin, 2014-11-20 23:07:19]

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

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:33: public Uri
getContentUriFromFile(Context context, File file);
On 2014/11/20 20:01:11, nyquist wrote:
> This is implicitly public.

ok, removed the public keyword.

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:78: if (pfd ==
null) {
On 2014/11/20 20:01:11, nyquist wrote:
> Optional: This bugs me; feel free to change this to return pfd != null. You
> could at that point also inline pfd.

Done.

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:89: * @returns MIME
type or null if the input params are empty or invalid.
On 2014/11/20 20:01:11, nyquist wrote:
> This should be @return (throughout this file)

Done.

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:95: if (resolver ==
null || uri == null) return null;
On 2014/11/20 20:01:11, nyquist wrote:
> Uri.parse(...) never returns null (specifically mentioned in the API).

Done.

https://codereview.chromium.org/739033003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/ContentUriUtils.java:113: } catch
(java.io.FileNotFoundException e) {
On 2014/11/20 20:01:11, nyquist wrote:
> could you change this one to import FileNotFoundException as well?

Done.

[nyquist, 2014-11-20 23:15:48]

thanks! lgtm

[noms (inactive), 2014-11-21 00:38:08]

noms@chromium.org changed reviewers:
+ mmenke@chromium.org

[noms (inactive), 2014-11-21 00:38:08]

mmenke@ is a better reviewer for ProfileIOData. Sorry!

[asanka, 2014-11-21 02:22:02]

mmenke: I see you are on the hook as well. Mind looking at the //net code?

My thoughts are that we shouldn't overload the FileProtocolHandler in this
manner. We can come up with ways to share code, but I think there should be a
ContentProtocolHandler and a URLRequestContentJob that encapsulate the
assumptions that are being made about content:// URIs. Also, given the
specifics, it seems that such protocol handler shouldn't be in //net either. In
general, FooProtocolHandler shouldn't need to check whether the URL in the
request is some other protocol.

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_file_job.cc:221: if (file_path.IsContentUri()) {
If this extra logic is required for a FilePath that contains a content URI, then
I think this logic should go in GetMimeTypeFromFile() instead of here.

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_file_job_unittest.cc (right):

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_file_job_unittest.cc:204:
ASSERT_LT(static_cast<unsigned int>(range->end), content.length());
Why not test the behavior when a range request extends past the end of the
content length?

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_file_job_unittest.cc:234: 
extra whitespace.

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_file_job_unittest.cc:313: RunRequestInternal(path,
NULL);
This doesn't run the test with a content:// URI, does it?

https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_file_job_unittest.cc:315:
EXPECT_EQ(delegate_.bytes_received(), file_size);
Assuming the test will still look like this, the received bytes can also be
verified by comparing with the contents of the file.

https://codereview.chromium.org/739033003/diff/40001/net/url_request/file_pro...
File net/url_request/file_protocol_handler.cc (right):

https://codereview.chromium.org/739033003/diff/40001/net/url_request/file_pro...
net/url_request/file_protocol_handler.cc:29: request, network_delegate,
base::FilePath(request->url().spec()),
Is this correct? No unescaping?

[mmenke, 2014-11-21 15:37:14]

asanka:  Don't think this requires any //net changes.

Android WebView already has support for content URLs, so we should be sharing
code, rather than having two completely different implementations.

On 2014/11/21 02:22:02, asanka wrote:
> mmenke: I see you are on the hook as well. Mind looking at the //net code?
> 
> My thoughts are that we shouldn't overload the FileProtocolHandler in this
> manner. We can come up with ways to share code, but I think there should be a
> ContentProtocolHandler and a URLRequestContentJob that encapsulate the
> assumptions that are being made about content:// URIs. Also, given the
> specifics, it seems that such protocol handler shouldn't be in //net either.
In
> general, FooProtocolHandler shouldn't need to check whether the URL in the
> request is some other protocol.
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> File net/url_request/url_request_file_job.cc (right):
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_file_job.cc:221: if (file_path.IsContentUri()) {
> If this extra logic is required for a FilePath that contains a content URI,
then
> I think this logic should go in GetMimeTypeFromFile() instead of here.
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> File net/url_request/url_request_file_job_unittest.cc (right):
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_file_job_unittest.cc:204:
> ASSERT_LT(static_cast<unsigned int>(range->end), content.length());
> Why not test the behavior when a range request extends past the end of the
> content length?
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_file_job_unittest.cc:234: 
> extra whitespace.
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_file_job_unittest.cc:313: RunRequestInternal(path,
> NULL);
> This doesn't run the test with a content:// URI, does it?
> 
>
https://codereview.chromium.org/739033003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_file_job_unittest.cc:315:
> EXPECT_EQ(delegate_.bytes_received(), file_size);
> Assuming the test will still look like this, the received bytes can also be
> verified by comparing with the contents of the file.
> 
>
https://codereview.chromium.org/739033003/diff/40001/net/url_request/file_pro...
> File net/url_request/file_protocol_handler.cc (right):
> 
>
https://codereview.chromium.org/739033003/diff/40001/net/url_request/file_pro...
> net/url_request/file_protocol_handler.cc:29: request, network_delegate,
> base::FilePath(request->url().spec()),
> Is this correct? No unescaping?

[qinmin, 2014-11-21 16:27:42]

On 2014/11/21 15:37:14, mmenke wrote:
> asanka:  Don't think this requires any //net changes.
> 
> Android WebView already has support for content URLs, so we should be sharing
> code, rather than having two completely different implementations.
> 

I don't think webview really did a good job on supporting content URLs.
Otherwise, we won't have crbug.com/425365 (the Android htmlViewer app uses
webview)
I think having a separate ContentProtocolHandler and a URLRequestContentJob
makes sense, I will do that in the next path

[mmenke, 2014-11-21 16:37:30]

On 2014/11/21 16:27:42, qinmin wrote:
> On 2014/11/21 15:37:14, mmenke wrote:
> > asanka:  Don't think this requires any //net changes.
> > 
> > Android WebView already has support for content URLs, so we should be
sharing
> > code, rather than having two completely different implementations.
> > 
> 
> I don't think webview really did a good job on supporting content URLs.
> Otherwise, we won't have crbug.com/425365 (the Android htmlViewer app uses
> webview)
> I think having a separate ContentProtocolHandler and a URLRequestContentJob
> makes sense, I will do that in the next path

I'm fine with handling the content scheme in some new manner, though if that's
the way to go (And I completely defer to you on that), we should plan to fairly
promptly move WebView over to the same method.  Makes no sense to maintain two
different ways to do it when it's not needed.

[qinmin, 2014-11-25 19:45:49]

qinmin@chromium.org changed reviewers:
+ sievers@chromium.org
- mmenke@chromium.org, noms@chromium.org

[qinmin, 2014-11-25 19:45:50]

+sievers for all the content/ review

-mmenke, -noms

[no sievers, 2014-11-25 20:50:19]

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/content_protocol_handler_impl.h (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/content_protocol_handler_impl.h:11: #include
"content/public/browser/android/content_protocol_handler.h"
Is this file missing in the patch?

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/content_protocol_handler_impl.h:32: virtual
~ContentProtocolHandlerImpl();
nit: ~ContentProtocolHandlerImpl() override; (or final even)

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:42:
content_task_runner_->PostTaskAndReply(
nit: Why do you want DidFetchMetaInfo to run as a separate task?

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:61: DCHECK_NE(dest_size, 0);
DCHECK_GT?

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:84:
DCHECK_GE(remaining_bytes_, 0);
I don't see this being implemented in FileStream. Would you mind adding a
|io_pending_| flag and DCHECKing and resetting that in DidRead()?

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:214: buf = NULL;
Either remove this or put a comment why this needs to be done before calling
Notify*().

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
why not 'if (result >= 0)'?

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:222:
NotifyReadComplete(result);
Does the order of NotifyDone and NotifyReadComplete matter?

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job_unittest.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job_unittest.cc:102: Range() {
nit: might as well remove this and explicitly run the test with Range(0, 0)
below

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job_unittest.cc:168:
std::min(range->end, (int) file_size - 1) - range->start + 1;
nit: bad-style cast, you can just do file_size - 1ll

https://codereview.chromium.org/739033003/diff/60001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/739033003/diff/60001/content/content_browser....
content/content_browser.gypi:351: 'browser/android/ui_resource_provider_impl.h',
Already listed in line 348

[qinmin, 2014-11-25 23:36:10]

Patchset #5 (id:80001) has been deleted

[qinmin, 2014-11-25 23:37:02]

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/content_protocol_handler_impl.h (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/content_protocol_handler_impl.h:11: #include
"content/public/browser/android/content_protocol_handler.h"
On 2014/11/25 20:50:18, sievers wrote:
> Is this file missing in the patch?

Sorry, forget to upload it, uploaded now

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/content_protocol_handler_impl.h:32: virtual
~ContentProtocolHandlerImpl();
On 2014/11/25 20:50:18, sievers wrote:
> nit: ~ContentProtocolHandlerImpl() override; (or final even)

Done.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:42:
content_task_runner_->PostTaskAndReply(
On 2014/11/25 20:50:18, sievers wrote:
> nit: Why do you want DidFetchMetaInfo to run as a separate task?

FetchMetaInfo() runs on the content_task_runner and DidFetchMetaInfo() runs on
the io thread. I think it is ok to combine DidFetchMetaInfo with some of the
other functions, or post a task back from the content_task_runner when
FetchMetaInfo completes, but here i just want to keep the code similar to that
of the FileRequestContentJob, so we can do some refactoring later.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:61: DCHECK_NE(dest_size, 0);
On 2014/11/25 20:50:18, sievers wrote:
> DCHECK_GT?

Done.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:84:
DCHECK_GE(remaining_bytes_, 0);
On 2014/11/25 20:50:18, sievers wrote:
> I don't see this being implemented in FileStream. Would you mind adding a
> |io_pending_| flag and DCHECKing and resetting that in DidRead()?

Done.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:214: buf = NULL;
On 2014/11/25 20:50:18, sievers wrote:
> Either remove this or put a comment why this needs to be done before calling
> Notify*().

removed.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
On 2014/11/25 20:50:18, sievers wrote:
> why not 'if (result >= 0)'?

all errors have negative value, and non-failure case the return value is 0.
added a DCHECK.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:222:
NotifyReadComplete(result);
On 2014/11/25 20:50:18, sievers wrote:
> Does the order of NotifyDone and NotifyReadComplete matter?

I think the ordering shouldn't matter as NotifyDone actually post a task. I
simply kept the ordering from URLRequestFileJob.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job_unittest.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job_unittest.cc:102: Range() {
On 2014/11/25 20:50:18, sievers wrote:
> nit: might as well remove this and explicitly run the test with Range(0, 0)
> below

Done.

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job_unittest.cc:168:
std::min(range->end, (int) file_size - 1) - range->start + 1;
On 2014/11/25 20:50:18, sievers wrote:
> nit: bad-style cast, you can just do file_size - 1ll

range->end is an int, so need to cast file_size or (file_size-1) into an int.
Anyway, move file_size - 1 into a bracket, it doesn't matter whether we use 1ll
or 1 in this case.

https://codereview.chromium.org/739033003/diff/60001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/739033003/diff/60001/content/content_browser....
content/content_browser.gypi:351: 'browser/android/ui_resource_provider_impl.h',
On 2014/11/25 20:50:18, sievers wrote:
> Already listed in line 348

Done.

[no sievers, 2014-11-25 23:51:52]

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
On 2014/11/25 23:37:02, qinmin wrote:
> On 2014/11/25 20:50:18, sievers wrote:
> > why not 'if (result >= 0)'?
> 
> all errors have negative value, and non-failure case the return value is 0.
> added a DCHECK.

But above you are handling |result > 0|.

https://codereview.chromium.org/739033003/diff/100001/content/browser/android...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/100001/content/browser/android...
content/browser/android/url_request_content_job.cc:85: io_pending_ = true;
Don't you have to set this below only if |rv == net::ERR_IO_PENDING)|?

Doesn't the DCHECK below in DidRead() fail now?

[qinmin, 2014-11-26 00:56:47]

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
On 2014/11/25 23:51:52, sievers wrote:
> On 2014/11/25 23:37:02, qinmin wrote:
> > On 2014/11/25 20:50:18, sievers wrote:
> > > why not 'if (result >= 0)'?
> > 
> > all errors have negative value, and non-failure case the return value is 0.
> > added a DCHECK.
> 
> But above you are handling |result > 0|.

ah... if resule>=0, the return value is the size read by file_stream. So in this
case, it means read hasn't completed yet, and we shouldn't call NotifyDone. If
result == 0, it means read has completed, and we can call NotifyDone. If result
< 0, an error is encounterde..

https://codereview.chromium.org/739033003/diff/100001/content/browser/android...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/100001/content/browser/android...
content/browser/android/url_request_content_job.cc:85: io_pending_ = true;
On 2014/11/25 23:51:52, sievers wrote:
> Don't you have to set this below only if |rv == net::ERR_IO_PENDING)|?
> 
> Doesn't the DCHECK below in DidRead() fail now?

oops, fixed.

[no sievers, 2014-11-26 01:09:49]

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
On 2014/11/26 00:56:46, qinmin wrote:
> On 2014/11/25 23:51:52, sievers wrote:
> > On 2014/11/25 23:37:02, qinmin wrote:
> > > On 2014/11/25 20:50:18, sievers wrote:
> > > > why not 'if (result >= 0)'?
> > > 
> > > all errors have negative value, and non-failure case the return value is
0.
> > > added a DCHECK.
> > 
> > But above you are handling |result > 0|.
> 
> ah... if resule>=0, the return value is the size read by file_stream. So in
this
> case, it means read hasn't completed yet, and we shouldn't call NotifyDone. If
> result == 0, it means read has completed, and we can call NotifyDone. If
result
> < 0, an error is encounterde..

So DidRead() gets called twice, once with result == bytes_read and then with
result == 0?
I only see it post the completion callback once from
FileStream::Context::Read().

[qinmin, 2014-11-26 01:38:05]

Patchset #6 (id:80002) has been deleted

[qinmin, 2014-11-26 01:38:42]

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
On 2014/11/26 01:09:49, sievers wrote:
> On 2014/11/26 00:56:46, qinmin wrote:
> > On 2014/11/25 23:51:52, sievers wrote:
> > > On 2014/11/25 23:37:02, qinmin wrote:
> > > > On 2014/11/25 20:50:18, sievers wrote:
> > > > > why not 'if (result >= 0)'?
> > > > 
> > > > all errors have negative value, and non-failure case the return value is
> 0.
> > > > added a DCHECK.
> > > 
> > > But above you are handling |result > 0|.
> > 
> > ah... if resule>=0, the return value is the size read by file_stream. So in
> this
> > case, it means read hasn't completed yet, and we shouldn't call NotifyDone.
If
> > result == 0, it means read has completed, and we can call NotifyDone. If
> result
> > < 0, an error is encounterde..
> 
> So DidRead() gets called twice, once with result == bytes_read and then with
> result == 0?
> I only see it post the completion callback once from
> FileStream::Context::Read().

The actual implementation should be in file_stream_context_posix.cc, if you
check FileStream::Context::ReadFileImpl(), you can see that the returned result
is defined in unistd.h, 0 for EOF, bytes read for successful case, and -1 for
error.

[no sievers, 2014-11-26 01:46:08]

lgtm

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
File content/browser/android/url_request_content_job.cc (right):

https://codereview.chromium.org/739033003/diff/60001/content/browser/android/...
content/browser/android/url_request_content_job.cc:216: if (result == 0) {
On 2014/11/26 01:38:42, qinmin wrote:
> On 2014/11/26 01:09:49, sievers wrote:
> > On 2014/11/26 00:56:46, qinmin wrote:
> > > On 2014/11/25 23:51:52, sievers wrote:
> > > > On 2014/11/25 23:37:02, qinmin wrote:
> > > > > On 2014/11/25 20:50:18, sievers wrote:
> > > > > > why not 'if (result >= 0)'?
> > > > > 
> > > > > all errors have negative value, and non-failure case the return value
is
> > 0.
> > > > > added a DCHECK.
> > > > 
> > > > But above you are handling |result > 0|.
> > > 
> > > ah... if resule>=0, the return value is the size read by file_stream. So
in
> > this
> > > case, it means read hasn't completed yet, and we shouldn't call
NotifyDone.
> If
> > > result == 0, it means read has completed, and we can call NotifyDone. If
> > result
> > > < 0, an error is encounterde..
> > 
> > So DidRead() gets called twice, once with result == bytes_read and then with
> > result == 0?
> > I only see it post the completion callback once from
> > FileStream::Context::Read().
> 
> The actual implementation should be in file_stream_context_posix.cc, if you
> check FileStream::Context::ReadFileImpl(), you can see that the returned
result
> is defined in unistd.h, 0 for EOF, bytes read for successful case, and -1 for
> error.

Ok as long as it works fine and we end up calling NotifyDone() if the size of
the read request is exactly equal to the remaining bytes in the file.

[qinmin, 2014-11-26 01:50:35]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-11-26 01:51:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/739033003/130001

[commit-bot: I haz the power, 2014-11-26 03:02:24]

Committed patchset #6 (id:130001)

[commit-bot: I haz the power, 2014-11-26 03:03:47]

Patchset 6 (id:??) landed as
https://crrev.com/120a15519703dfe8601596f1f436a322ea0a2aff
Cr-Commit-Position: refs/heads/master@{#305772}