Don't show a placeholder if the response contains the full image.

net/http/http_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // The rules for parsing content-types were borrowed from Firefox:
 // http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsURLHelper.cpp#834
 
 #include "net/http/http_util.h"
 
 #include <algorithm>
@@ skipping 247 matching lines @@
 
     // Do a final check on the HttpByteRange object.
     if (!range.IsValid())
       return false;
     ranges->push_back(range);
   }
   return !ranges->empty();
 }
 
 // static
+// From RFC 2616 14.16:
+// content-range-spec =
+//     bytes-unit SP byte-range-resp-spec "/" ( instance-length | "*" )
+// byte-range-resp-spec = (first-byte-pos "-" last-byte-pos) | "*"
+// instance-length = 1*DIGIT
+// bytes-unit = "bytes"
+bool HttpUtil::ParseContentRangeHeader(base::StringPiece content_range_spec,
+                                       int64_t* first_byte_position,
+                                       int64_t* last_byte_position,
+                                       int64_t* instance_length) {
+  *first_byte_position = *last_byte_position = *instance_length = -1;
+  content_range_spec = TrimLWS(content_range_spec);
+
+  size_t space_position = content_range_spec.find(' ');
+  if (space_position == base::StringPiece::npos)
+    return false;
+
+  // Invalid header if it doesn't contain "bytes-unit".
+  if (!base::LowerCaseEqualsASCII(
+          TrimLWS(content_range_spec.substr(0, space_position)), "bytes")) {
+    return false;
+  }
+
+  size_t slash_position = content_range_spec.find('/', space_position + 1);
+  if (slash_position == base::StringPiece::npos)
+    return false;
+
+  // Obtain the part behind the space and before slash.
+  base::StringPiece byte_range_resp_spec = TrimLWS(content_range_spec.substr(
+      space_position + 1, slash_position - (space_position + 1)));
+
+  if (byte_range_resp_spec != "*") {
+    size_t minus_position = byte_range_resp_spec.find('-');
+    if (minus_position == base::StringPiece::npos)
+      return false;
+
+    // Obtain first-byte-pos and last-byte-pos.
+    if (!base::StringToInt64(
+            TrimLWS(byte_range_resp_spec.substr(0, minus_position)),
+            first_byte_position) ||
+        !base::StringToInt64(
+            TrimLWS(byte_range_resp_spec.substr(minus_position + 1)),
+            last_byte_position)) {
+      *first_byte_position = *last_byte_position = -1;

[mmenke, 2016/12/02 19:05:08]

This API seems really bad.  It sets these values to something other than -1 on
some errors, but not on others.  Can we change the API to only set values to
something other than -1 when it returns true?

It looks like nothing depends on the current weirdness, with the possible
exception of chrome/browser/extensions/blob_reader.cc, so should get an owners
review from that for the API change, just in case.

Would also need to update docs.

[sclittle, 2016/12/02 22:19:22]

In what way are you suggesting changing the API? For example, consider
"Content-Range: bytes 0-99/*", which is a valid Content-Range according to spec
(unless I'm misreading it), such as for a resource where the full length isn't
known, but the server returned the first 100 bytes anyways.

Currently, this method would set the outputs to 0, 99, and -1 respectively, and
return false.

Are you suggesting that it should instead return true in that case, or that it
should set all the outputs to -1 and return false?

If we're changing it, I'd lean towards returning true in that case, since it
*is* a valid Content-Range. blob_reader.cc wouldn't need to be changed in that
case, and the ImageResource stuff in this file would be fine with that too.

[mmenke, 2016/12/02 22:31:56]

We return false if it's not a valid response to a 206 request, and nowhere do we
seem to handle the case where it's not a valid 206 response (And the API
description here is unclear).

"bytes 5-0" is an invalid byte range, and when we see it, we return
false/5/0/-1.  "bytes 1-2/*" is valid, but not for a 206 response, but when we
see it, we return false/1/2/-1....So if someone is using this for something
other than a 206, they basically have to do their own validation of the results
of this method, which is weird and bizarre, and no one is doing.

I'd like to get this API to a reasonable state before making more code depend on
it / making it easier for code to use.

[sclittle, 2016/12/03 00:43:13]

From what I can see, it looks like all the callers of this function only seem to
care about the Content-Range header in the context of 206 responses.

Do you mind if I land this CL keeping the existing behavior, with a TODO on
myself to clean it up? I'll send a follow up CL that changes the behavior and
API of ParseContentRanges.

[mmenke, 2016/12/03 00:46:21]

I'm fine with splitting it into multiple CLs, but I'd rather have webkit start
depending on this code only after we've cleaned up the API (i.e., just land the
net/ part of this CL, then clean up the API, then land the WebKit change.  Yes,
I'm paranoid).

[sclittle, 2016/12/03 00:56:34]

Woah, that is paranoid. Ok, I guess I'll do that then.

+      return false;
+    }
+    if (*first_byte_position < 0 || *last_byte_position < 0 ||
+        *first_byte_position > *last_byte_position)
+      return false;
+  }
+
+  // Parse the instance-length part.
+  base::StringPiece instance_length_spec =
+      TrimLWS(content_range_spec.substr(slash_position + 1));
+
+  if (base::StartsWith(instance_length_spec, "*",
+                       base::CompareCase::SENSITIVE)) {
+    return false;
+  } else if (!base::StringToInt64(instance_length_spec, instance_length)) {
+    *instance_length = -1;
+    return false;

[mmenke, 2016/12/02 22:38:47]

Actually, we don't provide enough data to validate it.  We return the same thing
for:

"1-2/*" as we do for "1-2/foo", which seems bad.

If we needed to handle the * case, I'd be fine with two methods, one that wraps
the other (One allows * and returns true if present, the other just calls into
it, and returns false and all -1's if the instance length is -1), but I'm not
sure we need both.  Do we have anything that cares about the asterisk?

[sclittle, 2016/12/03 00:43:13]

Looking at the call sites, I don't see any reader that cares about non-206
responses.

+  }
+
+  // We have all the values; let's verify that they make sense for a 206
+  // response.
+  if (*first_byte_position < 0 || *last_byte_position < 0 ||
+      *instance_length < 0 || *instance_length - 1 < *last_byte_position)
+    return false;
+
+  return true;
+}
+
+// static
 bool HttpUtil::ParseRetryAfterHeader(const std::string& retry_after_string,
                                      base::Time now,
                                      base::TimeDelta* retry_after) {
   int seconds;
   base::Time time;
   base::TimeDelta interval;
 
   if (base::StringToInt(retry_after_string, &seconds)) {
     interval = base::TimeDelta::FromSeconds(seconds);
   } else if (base::Time::FromUTCString(retry_after_string.c_str(), &time)) {
@@ skipping 795 matching lines @@
   return true;
 }
 
 bool HttpUtil::NameValuePairsIterator::IsQuote(char c) const {
   if (strict_quotes_)
     return c == '"';
   return HttpUtil::IsQuote(c);
 }
 
 }  // namespace net