Avoid a string copy in URLRequestResourceBundleJob::GetData().

Avoid a string copy in URLRequestResourceBundleJob::GetData().

This adds a URLRequestSimpleJob::GetStringPieceData method with which a subclass
can provide its own storage for the result string, instead of having to copy the
whole thing into the URLRequestSimpleJob.

I also made base::IsStringUTF8 take a StringPiece to avoid a copy there.

BUG=422489
Committed: https://crrev.com/3edf2fec6af00f9a3928edf5b67e77464c47819a
Cr-Commit-Position: refs/heads/master@{#304445}

[Jeffrey Yasskin, 2014-11-14 20:45:10]

jyasskin@chromium.org changed reviewers:
+ danakj@chromium.org, mmenke@chromium.org, rdevlin.cronin@chromium.org

[Jeffrey Yasskin, 2014-11-14 20:45:10]

jyasskin@chromium.org changed required reviewers:
+ danakj@chromium.org, mmenke@chromium.org

[Jeffrey Yasskin, 2014-11-14 20:45:11]

Dana for base/strings.
Devlin for extensions.
Matt for net: This is just the quickest of several options. It has the downside
that we can't detect a subclass that fails to override either GetData or
GetStringPieceData at compile time. I could also extract a base class (by what
name?). I don't immediately see how an extra constructor would help, but I'm
happy to do that too if you explain.

Thanks.

[danakj, 2014-11-14 20:50:06]

base LGTM

[Devlin, 2014-11-14 22:05:32]

extensions lgtm, assuming the net/ change works.

[mmenke, 2014-11-14 22:46:40]

https://codereview.chromium.org/725443003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.h (right):

https://codereview.chromium.org/725443003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:51: const CompletionCallback&
callback);
Could be instead take a base::RefCountedMemory?  See
ResourceBundle::LoadDataResourceBytes[ForScale]?

That may allow other cases of dynamically created data to avoid a copy, too.

[mmenke, 2014-11-14 22:47:11]

On 2014/11/14 22:46:40, mmenke wrote:
>
https://codereview.chromium.org/725443003/diff/20001/net/url_request/url_requ...
> File net/url_request/url_request_simple_job.h (right):
> 
>
https://codereview.chromium.org/725443003/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_simple_job.h:51: const CompletionCallback&
> callback);
> Could be instead take a base::RefCountedMemory?  See
> ResourceBundle::LoadDataResourceBytes[ForScale]?
> 
> That may allow other cases of dynamically created data to avoid a copy, too.

Erm...cases of non-immortal data, rather.

[Jeffrey Yasskin, 2014-11-15 02:34:20]

https://codereview.chromium.org/725443003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.h (right):

https://codereview.chromium.org/725443003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:51: const CompletionCallback&
callback);
On 2014/11/14 22:46:40, mmenke wrote:
> Could be instead take a base::RefCountedMemory?  See
> ResourceBundle::LoadDataResourceBytes[ForScale]?
> 
> That may allow other cases of dynamically created data to avoid a copy, too.

Ah, that's definitely better; thanks! I'm inclined to switch the rest of the
implementations of GetData() over to RefCountedMemory too, but to do that in a
separate patch after we've seen that this patch fixes the problem.

[mmenke, 2014-11-17 15:54:10]

https://codereview.chromium.org/725443003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/chrome_url_request_util.cc (right):

https://codereview.chromium.org/725443003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_url_request_util.cc:80:
rb.GetRawDataResource(resource_id_);
Can't you just replace this with rb.LoadDataResourceBytes(resource_id_); which
already has code to create the RefCountedStaticMemory?

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
net/url_request/url_request_simple_job.cc:73:
scoped_refptr<base::RefCountedString> str_data(new base::RefCountedString());
Do we get anything from using a temporary here?

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.h (right):

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:37: // Subclasses must override the way
response data is determined.
Should mention subclasses must override only one or the other.

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:51: // Similar to GetData(), except
*data can share ownership of the bytes instead
nit:  |*data| or *|data|.

[Jeffrey Yasskin, 2014-11-17 17:16:23]

Thanks!

https://codereview.chromium.org/725443003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/chrome_url_request_util.cc (right):

https://codereview.chromium.org/725443003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_url_request_util.cc:80:
rb.GetRawDataResource(resource_id_);
On 2014/11/17 15:54:10, mmenke wrote:
> Can't you just replace this with rb.LoadDataResourceBytes(resource_id_); which
> already has code to create the RefCountedStaticMemory?

This winds up forcing a couple extra virtual calls, and some extra code to
create the StringPiece out of the RefCountedMemory, but done.

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
net/url_request/url_request_simple_job.cc:73:
scoped_refptr<base::RefCountedString> str_data(new base::RefCountedString());
On 2014/11/17 15:54:10, mmenke wrote:
> Do we get anything from using a temporary here?

Yeah, I have to use the subclass's member functions to fill in the string in or
after the GetData() call. The temporary lets me delay the upcast long enough to
do that. I'd make the temporary a std::string, but RefCountedString() doesn't
have a constructor taking a string, so I'd still need the temporary
scoped_refptr<RefCountedString>.

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.h (right):

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:37: // Subclasses must override the way
response data is determined.
On 2014/11/17 15:54:10, mmenke wrote:
> Should mention subclasses must override only one or the other.

Done.

https://codereview.chromium.org/725443003/diff/40001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:51: // Similar to GetData(), except
*data can share ownership of the bytes instead
On 2014/11/17 15:54:10, mmenke wrote:
> nit:  |*data| or *|data|.

Done.

[mmenke, 2014-11-17 17:32:25]

LGTM

https://codereview.chromium.org/725443003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.h (right):

https://codereview.chromium.org/725443003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:51: const CompletionCallback& callback)
const;
nit:  Should probably be a blank line here.

https://codereview.chromium.org/725443003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:59: protected:
while you're here, mind removing this redundant label?

[Jeffrey Yasskin, 2014-11-17 17:40:08]

https://codereview.chromium.org/725443003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_simple_job.h (right):

https://codereview.chromium.org/725443003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:51: const CompletionCallback& callback)
const;
On 2014/11/17 17:32:25, mmenke wrote:
> nit:  Should probably be a blank line here.

Done.

https://codereview.chromium.org/725443003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job.h:59: protected:
On 2014/11/17 17:32:25, mmenke wrote:
> while you're here, mind removing this redundant label?

Done.

[Jeffrey Yasskin, 2014-11-17 17:40:12]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-11-17 17:41:01]

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

[commit-bot: I haz the power, 2014-11-17 18:44:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-17 18:45:07]

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