Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

By calling SetImageDownloadLimit, the ImageFetcher will cancel requests
that exceed the given byte size. This affects currently running and
upcoming requests.
The limit can be disabled by calling it with negative limits.

By default, there is no limit set. Therefore, all existing users of
image_data_fetchers are not affected.

TBR'ing for ownership of two tests. The added functions are never used and exist just to implement the interface so the compiler is happy.
TBR=sky@chromium.org
BUG=702601
Review-Url: https://codereview.chromium.org/2781473003
Cr-Commit-Position: refs/heads/master@{#460059}
Committed: https://chromium.googlesource.com/chromium/src/+/281080243642ac41b1004f9d8c6363db6081bbfa

[fhorschig, 2017-03-27 12:55:37]

Patchset #1 (id:1) has been deleted

[fhorschig, 2017-03-27 12:57:24]

fhorschig@chromium.org changed reviewers:
+ treib@chromium.org

[fhorschig, 2017-03-27 12:57:24]

fhorschig@chromium.org changed reviewers:
+ treib@chromium.org

[fhorschig, 2017-03-27 12:57:25]

Hi Marc,
would you please have a look?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:115: auto request_iter =
pending_requests_.find(source);
I am aware that I duplicate 4 operations here from OnURLFetchComplete
(find, check, call & delete) but having a private helper receiving an
iterator is somehow even uglier. Also, we would need two helpers or
we would move the DCHECK down. WDYT?

[fhorschig, 2017-03-27 12:57:25]

Hi Marc,
would you please have a look?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:115: auto request_iter =
pending_requests_.find(source);
I am aware that I duplicate 4 operations here from OnURLFetchComplete
(find, check, call & delete) but having a private helper receiving an
iterator is somehow even uglier. Also, we would need two helpers or
we would move the DCHECK down. WDYT?

[Marc Treib, 2017-03-27 13:16:43]

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:115: auto request_iter =
pending_requests_.find(source);
On 2017/03/27 12:57:25, fhorschig wrote:
> I am aware that I duplicate 4 operations here from OnURLFetchComplete
> (find, check, call & delete) but having a private helper receiving an
> iterator is somehow even uglier. Also, we would need two helpers or
> we would move the DCHECK down. WDYT?

We could have a "FinishRequest(source, result, metadata)" or something like
that?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:118: LOG(WARNING) << "Image
data exceededs download size limit.";
s/exceededs/exceeded/
Also DLOG?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.http_response_code = net::URLFetcher::RESPONSE_CODE_INVALID;
Hm. I wonder if we should somehow signal to the client what exactly happened?
WDYT?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:123: // Abort the finished
request.
nit: not actually finished

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.h (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:45: // Sets an upper limit
for image downloads that is by default disabled.
nit: Remove "that is by default disabled", IMO that doesn't belong on the
setter.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:46: // Setting
|max_download_bytes| to a negative value will disable the limit.
Hm. Make it a base::Optional<int64_t> instead? IMO that's clearer than the
negative values hack.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:49: max_download_bytes_ =
max_download_bytes < 0 ? 0 : max_download_bytes;
Move implementation to .cc file?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:89: // Running requests
exceeding this limit will be canceled immediately. By
Upper limit for the number of bytes to download per image.
(The "running requests..." part is really secondary)

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:217: // code that
would be possible for a completed fetch.
s/possible/impossible/ ?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:221: .Times(1);
nit: ".Times(1)" is a noop

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:231:
test_url_fetcher->SetResponseString(kURLResponseData);
nit: add a comment saying that this isn't actually used here?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:241: 
Maybe first call OnURLFetchDownloadProgress with a value below the limit, and
make sure the fetcher is still there?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:242: // Call the
URLFetcher delegate to continue the test.
nit: IMO this comment isn't really helpful.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:248: // The fetcher
has to be deleted for the request to be canceled.
"The URL fetch should have been canceled" ?

[fhorschig, 2017-03-27 14:33:43]

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:115: auto request_iter =
pending_requests_.find(source);
On 2017/03/27 13:16:42, Marc Treib wrote:
> On 2017/03/27 12:57:25, fhorschig wrote:
> > I am aware that I duplicate 4 operations here from OnURLFetchComplete
> > (find, check, call & delete) but having a private helper receiving an
> > iterator is somehow even uglier. Also, we would need two helpers or
> > we would move the DCHECK down. WDYT?
> 
> We could have a "FinishRequest(source, result, metadata)" or something like
> that?

Hmm, for the sake of the argument, I introduced that function.
But this forces me to duplicate the find and the DCHECK.
Probably this is an appropriate solution as the find within the DCHECK would
not even exist in a Release build. So yeah, that should work.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:118: LOG(WARNING) << "Image
data exceededs download size limit.";
On 2017/03/27 13:16:42, Marc Treib wrote:
> s/exceededs/exceeded/
> Also DLOG?

Done.
LOG was intentional, check below whether you agree.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.http_response_code = net::URLFetcher::RESPONSE_CODE_INVALID;
On 2017/03/27 13:16:42, Marc Treib wrote:
> Hm. I wonder if we should somehow signal to the client what exactly happened?
> WDYT?

An empty image in combination with the warning seemed appropriate.
(Thus, no DLOG but LOG.)

I am not a big fan of the current error handling but the behavior matches
exactly what a user can currently expect:
 // In case of an error an empty string is passed to the callback.
and
 // If an error prevented a http response,
 // |request_metadata.response_code| will be RESPONSE_CODE_INVALID.

If you like, I can file a bug but introducing a new way to handle errors
should not be part of the CL. 
In fact, there is no reaction to any error.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:123: // Abort the finished
request.
On 2017/03/27 13:16:42, Marc Treib wrote:
> nit: not actually finished

Gone.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.h (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:45: // Sets an upper limit
for image downloads that is by default disabled.
On 2017/03/27 13:16:42, Marc Treib wrote:
> nit: Remove "that is by default disabled", IMO that doesn't belong on the
> setter.

Done.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:46: // Setting
|max_download_bytes| to a negative value will disable the limit.
On 2017/03/27 13:16:42, Marc Treib wrote:
> Hm. Make it a base::Optional<int64_t> instead? IMO that's clearer than the
> negative values hack.

Done.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:49: max_download_bytes_ =
max_download_bytes < 0 ? 0 : max_download_bytes;
On 2017/03/27 13:16:42, Marc Treib wrote:
> Move implementation to .cc file?

Done.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:89: // Running requests
exceeding this limit will be canceled immediately. By
On 2017/03/27 13:16:42, Marc Treib wrote:
> Upper limit for the number of bytes to download per image.
> (The "running requests..." part is really secondary)

Done.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:217: // code that
would be possible for a completed fetch.
On 2017/03/27 13:16:43, Marc Treib wrote:
> s/possible/impossible/ ?

Absolutely.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:221: .Times(1);
On 2017/03/27 13:16:42, Marc Treib wrote:
> nit: ".Times(1)" is a noop

Done.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:231:
test_url_fetcher->SetResponseString(kURLResponseData);
On 2017/03/27 13:16:42, Marc Treib wrote:
> nit: add a comment saying that this isn't actually used here?

Do you mean this particular line? Because nothing I set here is
used. (Adjusted the comment above a little)

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:241: 
On 2017/03/27 13:16:42, Marc Treib wrote:
> Maybe first call OnURLFetchDownloadProgress with a value below the limit, and
> make sure the fetcher is still there?

Good idea. I introduced some edge cases.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:242: // Call the
URLFetcher delegate to continue the test.
On 2017/03/27 13:16:43, Marc Treib wrote:
> nit: IMO this comment isn't really helpful.

Gone.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:248: // The fetcher
has to be deleted for the request to be canceled.
On 2017/03/27 13:16:42, Marc Treib wrote:
> "The URL fetch should have been canceled" ?

Done.

[Marc Treib, 2017-03-27 14:49:18]

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:115: auto request_iter =
pending_requests_.find(source);
On 2017/03/27 14:33:43, fhorschig wrote:
> On 2017/03/27 13:16:42, Marc Treib wrote:
> > On 2017/03/27 12:57:25, fhorschig wrote:
> > > I am aware that I duplicate 4 operations here from OnURLFetchComplete
> > > (find, check, call & delete) but having a private helper receiving an
> > > iterator is somehow even uglier. Also, we would need two helpers or
> > > we would move the DCHECK down. WDYT?
> > 
> > We could have a "FinishRequest(source, result, metadata)" or something like
> > that?
> 
> Hmm, for the sake of the argument, I introduced that function.
> But this forces me to duplicate the find and the DCHECK.
> Probably this is an appropriate solution as the find within the DCHECK would
> not even exist in a Release build. So yeah, that should work.

Sure, that's fine. Though duplicating the DCHECKs isn't even strictly necessary,
is it? I think it'd be fine to only DCHECK in FinishRequest, since that's the
only place that touches pending_requests_ now.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:118: LOG(WARNING) << "Image
data exceededs download size limit.";
On 2017/03/27 14:33:42, fhorschig wrote:
> On 2017/03/27 13:16:42, Marc Treib wrote:
> > s/exceededs/exceeded/
> > Also DLOG?
> 
> Done.
> LOG was intentional, check below whether you agree.

Non-"D" LOG is rarely useful IMO. Any particular use case you had in mind?

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.http_response_code = net::URLFetcher::RESPONSE_CODE_INVALID;
On 2017/03/27 14:33:42, fhorschig wrote:
> On 2017/03/27 13:16:42, Marc Treib wrote:
> > Hm. I wonder if we should somehow signal to the client what exactly
happened?
> > WDYT?
> 
> An empty image in combination with the warning seemed appropriate.
> (Thus, no DLOG but LOG.)
> 
> I am not a big fan of the current error handling but the behavior matches
> exactly what a user can currently expect:
>  // In case of an error an empty string is passed to the callback.
> and
>  // If an error prevented a http response,
>  // |request_metadata.response_code| will be RESPONSE_CODE_INVALID.

Okay, fair enough.

> If you like, I can file a bug but introducing a new way to handle errors
> should not be part of the CL. 
> In fact, there is no reaction to any error.

I'm worried about the following situation: The Doodle image is 1.1 MB, and we
set a limit of 1 MB. So we download the first MB, then abort. Once the next NTP
is opened, we download the first MB again, etc. (Assuming the
partially-downloaded file won't get cached.)
In that case, it'd be better if we had just downloaded the whole image once.

If we told the client "the file was too large", then it could decide not to
retry. But maybe it should remember other errors anyway and also not retry...
hm.

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.from_http_cache = false;
This is the default anyway, no?

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.h (right):

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:47: // Already running
downloads are immediately affected.
nit: They're affected as soon as we get the next Progress callback, right? If
so, I'd remove the "immediately", that sounds like it'd happen "inline". (Sorry
for not noticing this earlier...)

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:48: void
SetImageDownloadLimit(const base::Optional<int64_t>& max_download_bytes);
I think it'd be fine to pass this by value - it's essentially an int plus a
bool. WDYT?

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:209: int64_t
kMaxDownloadBytes = 1024 * 1024;
const

[fhorschig, 2017-03-27 16:33:34]

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.from_http_cache = false;
On 2017/03/27 14:49:18, Marc Treib wrote:
> This is the default anyway, no?

Gone. (Should be)

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.h (right):

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:47: // Already running
downloads are immediately affected.
On 2017/03/27 14:49:18, Marc Treib wrote:
> nit: They're affected as soon as we get the next Progress callback, right? If
> so, I'd remove the "immediately", that sounds like it'd happen "inline".
(Sorry
> for not noticing this earlier...)

Gone.

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.h:48: void
SetImageDownloadLimit(const base::Optional<int64_t>& max_download_bytes);
On 2017/03/27 14:49:18, Marc Treib wrote:
> I think it'd be fine to pass this by value - it's essentially an int plus a
> bool. WDYT?

Hmpf, and I spent all those hours replacing that. Removed the const &.

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/2781473003/diff/40001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher_unittest.cc:209: int64_t
kMaxDownloadBytes = 1024 * 1024;
On 2017/03/27 14:49:18, Marc Treib wrote:
> const

Done.

[fhorschig, 2017-03-27 16:35:48]

The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 16:36:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-03-27 16:40:12]

LGTM with a last nit

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:118: LOG(WARNING) << "Image
data exceededs download size limit.";
On 2017/03/27 14:49:18, Marc Treib wrote:
> On 2017/03/27 14:33:42, fhorschig wrote:
> > On 2017/03/27 13:16:42, Marc Treib wrote:
> > > s/exceededs/exceeded/
> > > Also DLOG?
> > 
> > Done.
> > LOG was intentional, check below whether you agree.
> 
> Non-"D" LOG is rarely useful IMO. Any particular use case you had in mind?

This is still open

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.http_response_code = net::URLFetcher::RESPONSE_CODE_INVALID;
On 2017/03/27 14:49:18, Marc Treib wrote:
> On 2017/03/27 14:33:42, fhorschig wrote:
> > On 2017/03/27 13:16:42, Marc Treib wrote:
> > > Hm. I wonder if we should somehow signal to the client what exactly
> happened?
> > > WDYT?
> > 
> > An empty image in combination with the warning seemed appropriate.
> > (Thus, no DLOG but LOG.)
> > 
> > I am not a big fan of the current error handling but the behavior matches
> > exactly what a user can currently expect:
> >  // In case of an error an empty string is passed to the callback.
> > and
> >  // If an error prevented a http response,
> >  // |request_metadata.response_code| will be RESPONSE_CODE_INVALID.
> 
> Okay, fair enough.
> 
> > If you like, I can file a bug but introducing a new way to handle errors
> > should not be part of the CL. 
> > In fact, there is no reaction to any error.
> 
> I'm worried about the following situation: The Doodle image is 1.1 MB, and we
> set a limit of 1 MB. So we download the first MB, then abort. Once the next
NTP
> is opened, we download the first MB again, etc. (Assuming the
> partially-downloaded file won't get cached.)
> In that case, it'd be better if we had just downloaded the whole image once.
> 
> If we told the client "the file was too large", then it could decide not to
> retry. But maybe it should remember other errors anyway and also not retry...
> hm.

Did you file a bug for this?

[commit-bot: I haz the power, 2017-03-27 17:05:54]

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

[commit-bot: I haz the power, 2017-03-27 17:05:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fhorschig, 2017-03-28 08:28:54]

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
File components/image_fetcher/core/image_data_fetcher.cc (right):

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:118: LOG(WARNING) << "Image
data exceededs download size limit.";
On 2017/03/27 16:40:12, Marc Treib wrote:
> On 2017/03/27 14:49:18, Marc Treib wrote:
> > On 2017/03/27 14:33:42, fhorschig wrote:
> > > On 2017/03/27 13:16:42, Marc Treib wrote:
> > > > s/exceededs/exceeded/
> > > > Also DLOG?
> > > 
> > > Done.
> > > LOG was intentional, check below whether you agree.
> > 
> > Non-"D" LOG is rarely useful IMO. Any particular use case you had in mind?
> 
> This is still open

Change uploaded.

https://codereview.chromium.org/2781473003/diff/20001/components/image_fetche...
components/image_fetcher/core/image_data_fetcher.cc:120:
metadata.http_response_code = net::URLFetcher::RESPONSE_CODE_INVALID;
On 2017/03/27 16:40:12, Marc Treib wrote:
> On 2017/03/27 14:49:18, Marc Treib wrote:
> > On 2017/03/27 14:33:42, fhorschig wrote:
> > > On 2017/03/27 13:16:42, Marc Treib wrote:
> > > > Hm. I wonder if we should somehow signal to the client what exactly
> > happened?
> > > > WDYT?
> > > 
> > > An empty image in combination with the warning seemed appropriate.
> > > (Thus, no DLOG but LOG.)
> > > 
> > > I am not a big fan of the current error handling but the behavior matches
> > > exactly what a user can currently expect:
> > >  // In case of an error an empty string is passed to the callback.
> > > and
> > >  // If an error prevented a http response,
> > >  // |request_metadata.response_code| will be RESPONSE_CODE_INVALID.
> > 
> > Okay, fair enough.
> > 
> > > If you like, I can file a bug but introducing a new way to handle errors
> > > should not be part of the CL. 
> > > In fact, there is no reaction to any error.
> > 
> > I'm worried about the following situation: The Doodle image is 1.1 MB, and
we
> > set a limit of 1 MB. So we download the first MB, then abort. Once the next
> NTP
> > is opened, we download the first MB again, etc. (Assuming the
> > partially-downloaded file won't get cached.)
> > In that case, it'd be better if we had just downloaded the whole image once.
> > 
> > If we told the client "the file was too large", then it could decide not to
> > retry. But maybe it should remember other errors anyway and also not
retry...
> > hm.
> 
> Did you file a bug for this?
There is a bug now:
https://crbug.com/705888

I am not exactly sure of where it belongs since this is not NTP-exlusive
anymore.

[Marc Treib, 2017-03-28 08:48:14]

Thanks! Non-conditional lgtm now :)

[fhorschig, 2017-03-28 08:53:21]

The CQ bit was checked by fhorschig@chromium.org

[commit-bot: I haz the power, 2017-03-28 08:53:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 09:00:18]

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

[commit-bot: I haz the power, 2017-03-28 09:00:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marc Treib, 2017-03-28 10:07:59]

On 2017/03/28 09:00:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

IMO those changes are fine to TBR

[fhorschig, 2017-03-28 10:50:09]

Description was changed from

==========
Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

By calling SetImageDownloadLimit, the ImageFetcher will cancel requests
that exceed the given byte size. This affects currently running and
upcoming requests.
The limit can be disabled by calling it with negative limits.

By default, there is no limit set. Therefore, all existing users of
image_data_fetchers are not affected.

BUG=702601
==========

to

==========
Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

By calling SetImageDownloadLimit, the ImageFetcher will cancel requests
that exceed the given byte size. This affects currently running and
upcoming requests.
The limit can be disabled by calling it with negative limits.

By default, there is no limit set. Therefore, all existing users of
image_data_fetchers are not affected.

TBR'ing for ownership of two tests. The added functions are never used and exist
just to implement the interface so the compiler is happy.
TBR=sky@chromium.org
BUG=702601
==========

[fhorschig, 2017-03-28 10:51:21]

On 2017/03/28 10:07:59, Marc Treib wrote:
> On 2017/03/28 09:00:19, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> IMO those changes are fine to TBR

Thanks! I added sky@ to TBR and CC.

[fhorschig, 2017-03-28 10:51:37]

The CQ bit was checked by fhorschig@chromium.org

[commit-bot: I haz the power, 2017-03-28 10:51:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 10:55:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490698297068090,
"parent_rev": "6c71e781ce832aeeb16ccdfa80858cd4560ae128", "commit_rev":
"281080243642ac41b1004f9d8c6363db6081bbfa"}

[commit-bot: I haz the power, 2017-03-28 10:56:07]

Description was changed from

==========
Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

By calling SetImageDownloadLimit, the ImageFetcher will cancel requests
that exceed the given byte size. This affects currently running and
upcoming requests.
The limit can be disabled by calling it with negative limits.

By default, there is no limit set. Therefore, all existing users of
image_data_fetchers are not affected.

TBR'ing for ownership of two tests. The added functions are never used and exist
just to implement the interface so the compiler is happy.
TBR=sky@chromium.org
BUG=702601
==========

to

==========
Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

By calling SetImageDownloadLimit, the ImageFetcher will cancel requests
that exceed the given byte size. This affects currently running and
upcoming requests.
The limit can be disabled by calling it with negative limits.

By default, there is no limit set. Therefore, all existing users of
image_data_fetchers are not affected.

TBR'ing for ownership of two tests. The added functions are never used and exist
just to implement the interface so the compiler is happy.
TBR=sky@chromium.org
BUG=702601

Review-Url: https://codereview.chromium.org/2781473003
Cr-Commit-Position: refs/heads/master@{#460059}
Committed:
https://chromium.googlesource.com/chromium/src/+/281080243642ac41b1004f9d8c63...
==========

[commit-bot: I haz the power, 2017-03-28 10:56:08]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/281080243642ac41b1004f9d8c63...