Offload disk accesses to WorkerPool in ExtensionProtocolHandler

Offload disk accesses to WorkerPool in ExtensionProtocolHandler
and URLRequestResourceBundleJob.

To achieve that URLRequestSimpleJob::GetData() signature changed
to allow async operation finishing. All places implementing GetData()
are updated.


Patch from Pavel Ivanov <paivanof@gmail.com>;

BUG=59849, 90207
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147869

[pivanof, 2012-07-09 04:02:50]

This patch is only compile-tested because I couldn't find tests where changed
code paths in extension_protocols.cc are actually executed. Could you please
tell me are there tests for these?

Also Lint errors shown here seem to be invalid. Should I do something to fix
them?

[wtc, 2012-07-11 01:08:46]

Patch set 1 LGTM.

I did a cursory review of chrome/browser/extensions/extension_protocols.cc.

https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
File net/url_request/url_request_simple_job.cc (right):

https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
net/url_request/url_request_simple_job.cc:72: // what should the error code be?

Nit: add
    DCHECK_EQ(result, ERR_FAILED);

https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
File net/url_request/url_request_simple_job.h (right):

https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
net/url_request/url_request_simple_job.h:40: // called.

Nit: remove net:: in this comment block because this class
is inside the net namespace.

should be called => will be called
should never be called => will not be called

[pivanof, 2012-07-11 03:17:35]

I've updated patch but after second thought...

On 2012/07/11 01:08:46, wtc wrote:
https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
> net/url_request/url_request_simple_job.h:40: // called.
> 
> should be called => will be called
> should never be called => will not be called

This comment is placed on a pure virtual method GetData(). So it cannot
guarantee the exact behavior of all inherited classes, but it can say what
URLRequestSimpleJob expects those implementations to do. Thus maybe language
should be more suggesting than stating the fact?
But of course everything depends on what's the intended audience reading the
comment is. :) So I'll be okay with "will" too.

[wtc, 2012-07-11 18:48:49]

Patch set 2 LGTM.

https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
File net/url_request/url_request_simple_job.h (right):

https://chromiumcodereview.appspot.com/10696135/diff/1/net/url_request/url_re...
net/url_request/url_request_simple_job.h:40: // called.

I see.  Then the use of "should" is appropriate in this comment.
Sorry about my confusion.

Nit: the second sentence might be clearer in a bullet point
form:

  The return value should be
  * OK if data is obtained;
  * ERR_IO_PENDING if processing in another thread is needed to finish obtaining
data; and
  * ERR_FAILED to indicate an error.

[pivanof, 2012-07-12 01:10:27]

I've updated the patch.

Do I understand correctly that before committing we need to have a more detailed
review of chrome/browser/extensions/extension_protocols.cc? Did I add to
reviewers correct people for this job?

[wtc, 2012-07-12 01:34:11]

Sorry I wasn't clear.  I am only qualified to review
net/*.  I reviewed everything except
chrome/browser/extensions/extension_protocols.cc.

You should wait for the qualified reviewers for files
outside net/.

[pivanof, 2012-07-12 11:45:11]

On 2012/07/12 01:34:11, wtc wrote:
> Sorry I wasn't clear.  I am only qualified to review
> net/*.  I reviewed everything except
> chrome/browser/extensions/extension_protocols.cc.

mpcomplete, could you look at that?

> You should wait for the qualified reviewers for files
> outside net/.

Could you tell me did I add correct reviewers for that or should I add somebody
else?

[Matt Perry, 2012-07-12 18:49:55]

extension stuff LGTM. Thanks for taking this on. BTW, URLRequestFileJob has more
disk access on the wrong thread that could use cleaning up.

As for testing, any sort of test which loads a chrome-extension:// URL will test
extension_protocols. We have tons of tests which implicitly do this. I think
you're covered.

[pivanof, 2012-07-12 19:09:05]

On 2012/07/12 18:49:55, Matt Perry wrote:
> extension stuff LGTM. Thanks for taking this on. BTW, URLRequestFileJob has
more
> disk access on the wrong thread that could use cleaning up.

I think I've got that covered too:
https://chromiumcodereview.appspot.com/10695110/.

[Matt Perry, 2012-07-12 19:10:30]

On 2012/07/12 19:09:05, pivanof wrote:
> On 2012/07/12 18:49:55, Matt Perry wrote:
> > extension stuff LGTM. Thanks for taking this on. BTW, URLRequestFileJob has
> more
> > disk access on the wrong thread that could use cleaning up.
> 
> I think I've got that covered too:
> https://chromiumcodereview.appspot.com/10695110/.

Awesome!

[wtc, 2012-07-13 00:06:34]

On 2012/07/12 11:45:11, pivanof wrote:
> 
> Could you tell me did I add correct reviewers for that or should I add
somebody
> else?

Certainly.  I believe you have added the right reviewers.
What you need to do is to tell them which files they need to
review.  For example, you can say:

sky,mpcomplete: please review chrome/browser/extensions/extension_protocols.cc 

brettw: please review content/* and webkit/*

Also, michaeln and jamesr would be good reviewers for
content/browser/tcmalloc_internals_request_job.* and
webkit/{appcache,blob} based on SVN history of
content/browser/tcmalloc_internals_request_job.* and the
finer-grained OWNERS files in webkit/{appcache,blob}.  They
would be more appropriate if brettw is not responsive or if
you are making nontrivial changes to those files.

[wtc, 2012-07-13 00:06:36]

On 2012/07/12 11:45:11, pivanof wrote:
> 
> Could you tell me did I add correct reviewers for that or should I add
somebody
> else?

Certainly.  I believe you have added the right reviewers.
What you need to do is to tell them which files they need to
review.  For example, you can say:

sky,mpcomplete: please review chrome/browser/extensions/extension_protocols.cc 

brettw: please review content/* and webkit/*

Also, michaeln and jamesr would be good reviewers for
content/browser/tcmalloc_internals_request_job.* and
webkit/{appcache,blob} based on SVN history of
content/browser/tcmalloc_internals_request_job.* and the
finer-grained OWNERS files in webkit/{appcache,blob}.  They
would be more appropriate if brettw is not responsive or if
you are making nontrivial changes to those files.

[pivanof, 2012-07-13 01:49:48]

Thank you wtc, I guess I still need your lgtm on net/* files from patch set 3.
And as I understand jamesr won't be enough here as he is not among OWNERS of
content/. So

darin: could you please review content/browser/tcmalloc_internals_request_job.*
here?

michaeln: could you please review webkit/* files here?

[darin (slow to review), 2012-07-13 04:26:50]

https://chromiumcodereview.appspot.com/10696135/diff/12002/net/url_request/ur...
File net/url_request/url_request_simple_job.h (right):

https://chromiumcodereview.appspot.com/10696135/diff/12002/net/url_request/ur...
net/url_request/url_request_simple_job.h:37: //  - ERR_FAILED to indicate an
error;
questions:

- it is not OK to report other failures?

- it seems like another thread does not need to necessarily be involved here. 
one could also implement GetData using
MessageLoop::current()->PostDelayedTask(...), right?  that would also be valid. 
i think the key aspect here is that GetData may complete asynchronously.

https://chromiumcodereview.appspot.com/10696135/diff/12002/webkit/blob/view_b...
File webkit/blob/view_blob_internals_job.cc (right):

https://chromiumcodereview.appspot.com/10696135/diff/12002/webkit/blob/view_b...
webkit/blob/view_blob_internals_job.cc:147: std::string* mime_type,
nit: indentation should be 4 spaces

[pivanof, 2012-07-13 13:01:07]

On 2012/07/13 04:26:50, darin wrote:
>
https://chromiumcodereview.appspot.com/10696135/diff/12002/net/url_request/ur...
> File net/url_request/url_request_simple_job.h (right):
> 
>
https://chromiumcodereview.appspot.com/10696135/diff/12002/net/url_request/ur...
> net/url_request/url_request_simple_job.h:37: //  - ERR_FAILED to indicate an
> error;
> questions:
> 
> - it is not OK to report other failures?

Good point. It was done similar to what was there (only one value false in case
of error). But now I see it could be better.

> - it seems like another thread does not need to necessarily be involved here. 
> one could also implement GetData using
> MessageLoop::current()->PostDelayedTask(...), right?  that would also be
valid. 
> i think the key aspect here is that GetData may complete asynchronously.

Yes, you are right. I've updated the comment.

>
https://chromiumcodereview.appspot.com/10696135/diff/12002/webkit/blob/view_b...
> File webkit/blob/view_blob_internals_job.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10696135/diff/12002/webkit/blob/view_b...
> webkit/blob/view_blob_internals_job.cc:147: std::string* mime_type,
> nit: indentation should be 4 spaces

For some reason I thought it should be 4 spaces from function body. I've updated
this too.


wtc: it turns out this patch depends on change in net/base/mime_util.cc that
I've included in https://chromiumcodereview.appspot.com/10695110/. So either
this patch should be committed after that (which should be committed after yet
unreviewed https://chromiumcodereview.appspot.com/10701050/) or change in
net/base/mime_util.cc should be included in this patch. Could you please suggest
which is better?

[michaeln, 2012-07-13 19:32:21]

/webkit lgtm

[wtc, 2012-07-13 22:55:14]

Patch set 4 LGTM.

I think https://chromiumcodereview.appspot.com/10695110/
should be kept separate.  Thanks.

https://chromiumcodereview.appspot.com/10696135/diff/9007/chrome/browser/exte...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/10696135/diff/9007/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:130: int result = *read_result?
net::OK: net::ERR_INVALID_URL;

The use of net::ERR_INVALID_URL here was not obvious.  (I
know it was meant to match the error code used in the original
code.)  I would think that the most common reason for
net::GetMimeTypeFromFile to fail is ERR_FILE_NOT_FOUND.
I guess ERR_INVALID_URL can be interpreted to mean the file
URL is invalid?

https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
File net/url_request/url_request_simple_job.h (right):

https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
net/url_request/url_request_simple_job.h:42: //    error code while finishing
request.

Remove "This code will be used as error code while finishing request."
because it seems to be stating the obvious.

[pivanof, 2012-07-13 23:32:31]

On 2012/07/13 22:55:14, wtc wrote:
> Patch set 4 LGTM.
> 
> I think https://chromiumcodereview.appspot.com/10695110/
> should be kept separate.  Thanks.

Separate meaning changes to net/base/mime_util.cc should be included here too?
 
>
https://chromiumcodereview.appspot.com/10696135/diff/9007/chrome/browser/exte...
> File chrome/browser/extensions/extension_protocols.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10696135/diff/9007/chrome/browser/exte...
> chrome/browser/extensions/extension_protocols.cc:130: int result =
*read_result?
> net::OK: net::ERR_INVALID_URL;
> 
> The use of net::ERR_INVALID_URL here was not obvious.  (I
> know it was meant to match the error code used in the original
> code.)  I would think that the most common reason for
> net::GetMimeTypeFromFile to fail is ERR_FILE_NOT_FOUND.
> I guess ERR_INVALID_URL can be interpreted to mean the file
> URL is invalid?

Actually no, my selection of ERR_INVALID_URL wasn't meant to match original
error code. I wanted to select something that is the most appropriate here. And
my thoughts were as follows: GetMimeTypeFromFile uses only file extension to
understand which mime type is that (it doesn't looks into file contents, in fact
as I understand it doesn't do any disk accesses at all except reading registry
on Windows). So if GetMimeTypeFromFile returned false it means it didn't
understand the extension it was given, which means filename points to file that
cannot be processed by browser, which means filename is an invalid URL.

>
https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
> File net/url_request/url_request_simple_job.h (right):
> 
>
https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
> net/url_request/url_request_simple_job.h:42: //    error code while finishing
> request.
> 
> Remove "This code will be used as error code while finishing request."
> because it seems to be stating the obvious.

I'd disagree. It doesn't look obvious to me that error code from GetData()
should translate directly to error code for the request. Especially when before
this patch GetData() just returned false and error code for request was an
implementation detail.

[wtc, 2012-07-14 00:08:55]

Thank you for the clarifications.

If all you need from https://chromiumcodereview.appspot.com/10695110/ is
https://chromiumcodereview.appspot.com/10695110/diff/1004/net/base/mime_util.cc,
you can include the changes to net/base/mime_util.cc in
this CL.  I thought you were asking about combining the
entire https://chromiumcodereview.appspot.com/10695110/
with this CL.

https://chromiumcodereview.appspot.com/10696135/diff/9007/chrome/browser/exte...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/10696135/diff/9007/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:130: int result = *read_result?
net::OK: net::ERR_INVALID_URL;

Thank you for the explanation.  I didn't think about this
hard enough.  I saw net::GetMimeTypeFromFile takes a 'filename'
argument and just assumed the most common reason of failure is
"file not found".

I agree that ERR_INVALID_URL is appropriate here.

https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
File net/url_request/url_request_simple_job.h (right):

https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
net/url_request/url_request_simple_job.h:42: //    error code while finishing
request.

Thank you for the clarification.  I didn't realize that by
"request" you're referring to the URLRequestSimpleJob rather
than the GetData call.  I suggest rewriting this sentence as
follows:

  This code will be used as the error code in the
  URLRequestStatus for the request.

or

  This code will be used as the error code in the
  URLRequestStatus when the URLRequest is finished.

Note: you tend to omit unimportant words such as "a", "the",
"this" in your comments.  Sometimes this makes the comments
a little hard to read.  However, in this case I misinterpreted
"request" as "call" or "operation".

[pivanof, 2012-07-14 03:31:47]

On 2012/07/14 00:08:55, wtc wrote:
>
https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
> File net/url_request/url_request_simple_job.h (right):
> 
>
https://chromiumcodereview.appspot.com/10696135/diff/9007/net/url_request/url...
> net/url_request/url_request_simple_job.h:42: //    error code while finishing
> request.
> 
> Thank you for the clarification.  I didn't realize that by
> "request" you're referring to the URLRequestSimpleJob rather
> than the GetData call.  I suggest rewriting this sentence as
> follows:
> 
>   This code will be used as the error code in the
>   URLRequestStatus for the request.
> 
> or
> 
>   This code will be used as the error code in the
>   URLRequestStatus when the URLRequest is finished.

I've updated patch putting the second of suggested comments and adding
mime_util.cc.

[sky, 2012-07-16 14:55:05]

I removed myself as a reviewer from this cl since I'm not an owner of any of the
files you're changing.

[wtc, 2012-07-16 23:37:35]

Patch set 5 LGTM.

[pivanof, 2012-07-16 23:48:20]

darin: Your lgtm for content/* is needed. And could you also commit the CL?

[sky, 2012-07-20 15:30:55]

LGTM

[pivanof, 2012-07-20 16:04:16]

wtc, mpcomplete, michaeln, sky: Could any of you please commit the CL?

[willchan no longer on Chromium, 2012-07-20 16:14:08]

Do you get a commit checkbox that you can check? That should submit it to
the commit queue if you've got all the approvals.

[commit-bot: I haz the power, 2012-07-20 16:15:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10696135/15003

[pivanof, 2012-07-20 16:19:32]

On 2012/07/20 16:14:08, willchan wrote:
> Do you get a commit checkbox that you can check? That should submit it to
> the commit queue if you've got all the approvals.

Hm... I didn't know that. Thanks. I guess docs at
http://dev.chromium.org/developers/contributing-code should be changed to
reflect this.

[commit-bot: I haz the power, 2012-07-20 16:29:08]

Try job failure for 10696135-15003 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[pivanof, 2012-07-21 01:53:03]

sky: I've rebased and had to add more files from content/browser/ into CL (they
were added recently). Please take a look.

[sky, 2012-07-23 15:38:01]

LGTM

[commit-bot: I haz the power, 2012-07-23 15:38:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10696135/36007

[commit-bot: I haz the power, 2012-07-23 17:15:15]

Change committed as 147869