Add |SetImageDownloadLimit| to ImageFetcher to limit downloaded bytes

components/image_fetcher/core/image_data_fetcher.cc

Index: components/image_fetcher/core/image_data_fetcher.cc
diff --git a/components/image_fetcher/core/image_data_fetcher.cc b/components/image_fetcher/core/image_data_fetcher.cc
index 0ef812f6465ffe2436e7aed35d73fa781d14c0ab..e01f3fa6998c882c1a5085b4718063e652444ca2 100644
--- a/components/image_fetcher/core/image_data_fetcher.cc
+++ b/components/image_fetcher/core/image_data_fetcher.cc
@@ -36,7 +36,8 @@ ImageDataFetcher::ImageDataFetcher(
     net::URLRequestContextGetter* url_request_context_getter)
     : url_request_context_getter_(url_request_context_getter),
       data_use_service_name_(DataUseUserData::IMAGE_FETCHER_UNTAGGED),
-      next_url_fetcher_id_(0) {}
+      next_url_fetcher_id_(0),
+      max_download_bytes_(-1) {}  // No download limitation.
 
 ImageDataFetcher::~ImageDataFetcher() {}
 
@@ -100,4 +101,27 @@ void ImageDataFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   pending_requests_.erase(request_iter);
 }
 
+void ImageDataFetcher::OnURLFetchDownloadProgress(
+    const net::URLFetcher* source,
+    int64_t current,
+    int64_t total,
+    int64_t current_network_bytes) {
+  if (max_download_bytes_ < 0) {
+    return;
+  }
+  if (total <= max_download_bytes_ && current <= max_download_bytes_) {
+    return;
+  }
+  auto request_iter = pending_requests_.find(source);

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

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:42]

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

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

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.

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

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.

+  DCHECK(request_iter != pending_requests_.end());
+
+  LOG(WARNING) << "Image data exceededs download size limit.";

[Marc Treib, 2017/03/27 13:16:42]

s/exceededs/exceeded/
Also DLOG?

[fhorschig, 2017/03/27 14:33:42]

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

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

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

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

This is still open

[fhorschig, 2017/03/28 08:28:53]

Change uploaded.

+  RequestMetadata metadata;
+  metadata.http_response_code = net::URLFetcher::RESPONSE_CODE_INVALID;

[Marc Treib, 2017/03/27 13:16:42]

Hm. I wonder if we should somehow signal to the client what exactly happened?
WDYT?

[fhorschig, 2017/03/27 14:33:42]

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.

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

Okay, fair enough.
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.

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

Did you file a bug for this?

[fhorschig, 2017/03/28 08:28:53]

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.

+
+  request_iter->second->callback.Run(std::string(), metadata);
+  // Abort the finished request.

[Marc Treib, 2017/03/27 13:16:42]

nit: not actually finished

[fhorschig, 2017/03/27 14:33:42]

Gone.

+  pending_requests_.erase(request_iter);
+}
+
 }  // namespace image_fetcher