libaddressinput - one less copy when downloading data

libaddressinput - one less copy when downloading data

BUG=337679
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248561

[Evan Stade, 2014-01-31 03:51:26]

For context, libaddressinput downloads one potentially large block of data,
which I'd like to avoid copying around too much.

[Ryan Sleevi, 2014-01-31 21:03:17]

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:163: scoped_refptr<IOBuffer> buffer
= new StringIOBuffer(fake_response_string_);
pedantic nit: buffer(new StringIOBuffer(...)) ?

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:166: CompletionCallback());
This doesn't fully match the URLFetcher / URLFetcherResponseWriter interface.

If you're going to allow callers to supply an arbitrary URLFRW, I feel like we
should at least try to not violate 'least surprise' here.

Namely,
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

Write() may not write all of the data from fake_response_string_, so we should
try to continue writing more.

[Evan Stade, 2014-01-31 22:24:57]

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:166: CompletionCallback());
On 2014/01/31 21:03:17, Ryan Sleevi wrote:
> This doesn't fully match the URLFetcher / URLFetcherResponseWriter interface.
> 
> If you're going to allow callers to supply an arbitrary URLFRW, I feel like we
> should at least try to not violate 'least surprise' here.
> 
> Namely,
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> 
> Write() may not write all of the data from fake_response_string_, so we should
> try to continue writing more.

well, it has several flaws:

a) SetResponseString must have been previously called (as opposed to being
called afterwards)
b) async Writers are not supported
c) writers that don't write all of buffer will fail (as you've noted)

Fixing all these things would require a bit of effort. Spending effort on code
to support tests which don't exist seems like a bit of a waste, I think. No one
bothered to implement SaveResponseWithWriter at all (until it was needed), now
it's partially implemented, which seems like an improvement.

[Ryan Sleevi, 2014-01-31 22:31:16]

On 2014/01/31 22:24:57, Evan Stade wrote:
>
https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
> File net/url_request/test_url_fetcher_factory.cc (right):
> 
>
https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
> net/url_request/test_url_fetcher_factory.cc:166: CompletionCallback());
> On 2014/01/31 21:03:17, Ryan Sleevi wrote:
> > This doesn't fully match the URLFetcher / URLFetcherResponseWriter
interface.
> > 
> > If you're going to allow callers to supply an arbitrary URLFRW, I feel like
we
> > should at least try to not violate 'least surprise' here.
> > 
> > Namely,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> > 
> > Write() may not write all of the data from fake_response_string_, so we
should
> > try to continue writing more.
> 
> well, it has several flaws:
> 
> a) SetResponseString must have been previously called (as opposed to being
> called afterwards)
> b) async Writers are not supported
> c) writers that don't write all of buffer will fail (as you've noted)
> 
> Fixing all these things would require a bit of effort. Spending effort on code
> to support tests which don't exist seems like a bit of a waste, I think. No
one
> bothered to implement SaveResponseWithWriter at all (until it was needed), now
> it's partially implemented, which seems like an improvement.

Sure. But as with all things in net/, once it looks like somethings there,
people will start to use it :)

I'd suggest that it's fine to not implement the other bits, but use some DCHECK
/ NOTIMPLEMENTED() guards so that if/when tests fail - or hopefully when someone
starts coding against it - they realize it's not finalized and come back and
touch it more. Just the "silent failure" now is fairly subtle.

[Evan Stade, 2014-01-31 22:56:46]

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:163: scoped_refptr<IOBuffer> buffer
= new StringIOBuffer(fake_response_string_);
On 2014/01/31 21:03:17, Ryan Sleevi wrote:
> pedantic nit: buffer(new StringIOBuffer(...)) ?

Done.

https://codereview.chromium.org/151133002/diff/20001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:166: CompletionCallback());
On 2014/01/31 22:24:57, Evan Stade wrote:
> On 2014/01/31 21:03:17, Ryan Sleevi wrote:
> > This doesn't fully match the URLFetcher / URLFetcherResponseWriter
interface.
> > 
> > If you're going to allow callers to supply an arbitrary URLFRW, I feel like
we
> > should at least try to not violate 'least surprise' here.
> > 
> > Namely,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> > 
> > Write() may not write all of the data from fake_response_string_, so we
should
> > try to continue writing more.
> 
> well, it has several flaws:
> 
> a) SetResponseString must have been previously called (as opposed to being
> called afterwards)
> b) async Writers are not supported
> c) writers that don't write all of buffer will fail (as you've noted)
> 
> Fixing all these things would require a bit of effort. Spending effort on code
> to support tests which don't exist seems like a bit of a waste, I think. No
one
> bothered to implement SaveResponseWithWriter at all (until it was needed), now
> it's partially implemented, which seems like an improvement.

Done.

[Ryan Sleevi, 2014-02-01 00:46:33]

net/ LGTM, although I'm a little surprised that you're optimizing down to the
string level. Perhaps it's not the best datatype?

[Evan Stade, 2014-02-01 00:48:11]

On 2014/02/01 00:46:33, Ryan Sleevi wrote:
> net/ LGTM, although I'm a little surprised that you're optimizing down to the
> string level. Perhaps it's not the best datatype?

Thanks. Some of the strings are large. libaddressinput could use a different
kind of buffer but I'm not sure what that would gain us (I'd still want to avoid
copying).

[Evan Stade, 2014-02-01 00:48:24]

+dbeam for libaddressinput

[Dan Beam, 2014-02-01 00:53:33]

small questions before lg

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:22: class
UnownedStringWriter : public net::URLFetcherResponseWriter {
nit: BufferedStringWriter maybe?

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:28:
data_->clear();
should we be calling the CompletionCallbacks from any of these methods?

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:43: private:
maybe add this somewhere:

  // Cleared on initialization, written to a chunk at a time.

[Evan Stade, 2014-02-01 01:02:15]

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:22: class
UnownedStringWriter : public net::URLFetcherResponseWriter {
On 2014/02/01 00:53:33, Dan Beam wrote:
> nit: BufferedStringWriter maybe?

I like unowned better, I'm not sure what "BufferedString" would mean.

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:28:
data_->clear();
On 2014/02/01 00:53:33, Dan Beam wrote:
> should we be calling the CompletionCallbacks from any of these methods?

no, we should not, because these operations are all synchronous.

https://codereview.chromium.org/151133002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:43: private:
On 2014/02/01 00:53:33, Dan Beam wrote:
> maybe add this somewhere:
> 
>   // Cleared on initialization, written to a chunk at a time.

I think that's easy enough to see in code; a comment has the drawback of being
likely to fork from the code. And it's not like I'm inviting other clients to
use this class (that's why it's in the .cc).

[Dan Beam, 2014-02-01 01:03:29]

lgtm

[Dan Beam, 2014-02-01 01:07:36]

lgtm

[Evan Stade, 2014-02-01 01:07:40]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-01 01:08:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/151133002/410002

[commit-bot: I haz the power, 2014-02-01 01:25:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 01:25:52]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-02-01 01:25:55]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 01:26:02]

CQ bit was unchecked on CL. Ignoring.

[Evan Stade, 2014-02-01 02:19:02]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/151133002/410002

[commit-bot: I haz the power, 2014-02-01 02:49:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:49:57]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-02-01 02:50:02]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 02:50:05]

CQ bit was unchecked on CL. Ignoring.

[Evan Stade, 2014-02-01 03:03:37]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-01 03:04:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/151133002/410002

[commit-bot: I haz the power, 2014-02-01 03:04:52]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 03:04:54]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 03:04:55]

CQ bit was unchecked on CL. Ignoring.

[Evan Stade, 2014-02-01 03:05:00]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-01 03:05:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/151133002/840001

[commit-bot: I haz the power, 2014-02-01 03:20:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 03:20:15]

Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests,
cc_unittests, check_deps, check_deps2git, chromedriver_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, ppapi_unittests, printing_unittests,
remoting_unittests, sandbox_linux_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-02-01 03:20:15]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 03:20:23]

CQ bit was unchecked on CL. Ignoring.

[Evan Stade, 2014-02-03 17:32:52]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-02-03 17:33:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/151133002/840001

[commit-bot: I haz the power, 2014-02-03 20:10:59]

Change committed as 248561

[commit-bot: I haz the power, 2014-02-03 20:11:07]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 20:11:08]

CQ bit was unchecked on CL. Ignoring.