Fixed playing video in component extensions.

net/url_request/url_request_simple_job_unittest.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reservedelegate_.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "net/base/request_priority.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_job_factory.h"
+#include "net/url_request/url_request_job_factory_impl.h"
+#include "net/url_request/url_request_simple_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+const char kTestData[] = "Huge data array";
+const size_t kRangeFirstPosition = 5;
+const size_t kRangeLastPosition = 8;
+
+class MockSimpleJob : public net::URLRequestSimpleJob {

[mmenke, 2014/01/07 18:51:39]

nit:  "net::" prefix not needed anywhere in this file.

[kirr, 2014/01/09 07:33:21]

Done.

+ public:
+  MockSimpleJob(net::URLRequest* request,
+                    net::NetworkDelegate* network_delegate)

[mmenke, 2014/01/07 18:51:39]

nit:  4 less indent.

[kirr, 2014/01/09 07:33:21]

Done.

+      : net::URLRequestSimpleJob(request, network_delegate) {
+  }
+
+ protected:
+  virtual int GetData(std::string* mime_type,
+                      std::string* charset,
+                      std::string* data,
+                      const net::CompletionCallback& callback) const OVERRIDE {
+    mime_type->assign("text/plain");
+    charset->assign("US-ASCII");
+    data->assign(kTestData);
+    return net::OK;
+  }
+
+ private:
+  virtual ~MockSimpleJob() {}
+
+  std::string data_;
+  DISALLOW_COPY_AND_ASSIGN(MockSimpleJob);

[mmenke, 2014/01/07 18:51:39]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN.

[kirr, 2014/01/09 07:33:21]

Done.

+};
+
+class URLRequestSimpleJobTest : public ::testing::Test {
+ public:
+  class SimpleJobProtocolHandler :
+      public net::URLRequestJobFactory::ProtocolHandler {

[mmenke, 2014/01/07 18:51:39]

optional:  Suggest moving this class outside of the test fixture, just to make
the indentation easier to follow.

[kirr, 2014/01/09 07:33:21]

Done.

+   public:
+    virtual net::URLRequestJob* MaybeCreateJob(
+        net::URLRequest* request,
+        net::NetworkDelegate* network_delegate) const OVERRIDE {
+      return new MockSimpleJob(request, network_delegate);
+    }
+  };
+
+  URLRequestSimpleJobTest() : context_(true) {
+    CHECK(kRangeFirstPosition > 0 &&
+        kRangeFirstPosition < kRangeLastPosition &&
+        kRangeLastPosition < strlen(kTestData));

[mmenke, 2014/01/07 18:51:39]

Instead of a CHECK, you could put things like the following at the top of the
file:

COMPILE_ASSERT(kRangeFirstPosition > 0, RangeStarTooLow);

(I believe you'd need to use something like ARRAYSIZE(kTestData) - 1 for the
last one).

I'm not sure these are really necessary, though.

[kirr, 2014/01/09 07:33:21]

Done.

+
+    job_factory_.SetProtocolHandler("data", new SimpleJobProtocolHandler());
+    context_.set_job_factory(&job_factory_);
+    context_.Init();
+
+    request_.reset(new URLRequest(
+        GURL("data:test"), DEFAULT_PRIORITY, &delegate_, &context_));
+  }
+
+  void StartRequest(const net::HttpRequestHeaders* headers) {
+    if (headers)
+      request_->SetExtraRequestHeaders(*headers);
+    request_->Start();
+
+    EXPECT_TRUE(request_->is_pending());
+    base::RunLoop().Run();
+    EXPECT_FALSE(request_->is_pending());
+  }
+
+ protected:
+  URLRequestJobFactoryImpl job_factory_;
+  TestURLRequestContext context_;
+  TestDelegate delegate_;
+  scoped_ptr<URLRequest> request_;
+
+};
+
+}  // namespace
+
+TEST_F(URLRequestSimpleJobTest, SimpleRequest) {
+  StartRequest(NULL);
+  ASSERT_TRUE(request_->status().is_success());
+  EXPECT_EQ(kTestData, delegate_.data_received());
+}
+
+TEST_F(URLRequestSimpleJobTest, RangeRequest) {
+  const std::string data_part = std::string(kTestData + kRangeFirstPosition,

[mmenke, 2014/01/07 18:51:39]

nit:  This should use kConstantNamingScheme.  I also think we could do a bit
better on the name.  Maybe kExpectedBody?

[kirr, 2014/01/09 07:33:21]

Done.

+      kTestData + kRangeLastPosition + 1);

[mmenke, 2014/01/07 18:51:39]

nit:  This should line up with the "kTestData" in the above line.  If there's
not enough space, you can move the "kTestData + kRangeFirstPosition" down to the
next line, and optionally move the "std::string(" down as well.

[kirr, 2014/01/09 07:33:21]

Done.

+  HttpRequestHeaders headers;
+  headers.SetHeader(
+      HttpRequestHeaders::kRange,
+      net::HttpByteRange::Bounded(kRangeFirstPosition, kRangeLastPosition)
+          .GetHeaderValue());
+
+  StartRequest(&headers);
+
+  ASSERT_TRUE(request_->status().is_success());
+  EXPECT_EQ(data_part, delegate_.data_received());
+}
+
+TEST_F(URLRequestSimpleJobTest, MultipleRangeRequest) {
+  HttpRequestHeaders headers;
+  size_t middle_pos = (kRangeFirstPosition + kRangeLastPosition)/2;
+  std::string range = base::StringPrintf("bytes=%zu-%zu,%zu-%zu",
+                                         kRangeFirstPosition,
+                                         middle_pos,
+                                         middle_pos + 1,
+                                         kRangeLastPosition);
+  headers.SetHeader(HttpRequestHeaders::kRange, range);
+
+  StartRequest(&headers);
+
+  EXPECT_TRUE(delegate_.request_failed());
+  EXPECT_EQ(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE, request_->status().error());
+}
+
+TEST_F(URLRequestSimpleJobTest, InvalidRangeRequest) {
+  HttpRequestHeaders headers;
+  std::string range = base::StringPrintf(
+      "bytes=%zu-%zu", kRangeLastPosition, kRangeFirstPosition);
+  headers.SetHeader(HttpRequestHeaders::kRange, range);
+
+  StartRequest(&headers);
+
+  ASSERT_TRUE(request_->status().is_success());
+  EXPECT_EQ(kTestData, delegate_.data_received());

[mmenke, 2014/01/07 18:51:39]

Wonder if we should be returning an error or not in this case.  I don't feel
strongly about it, but it seems kinda weird that we're completely refusing to
handle multiple ranges, but happy to ignore bad ranges.

[kirr, 2014/01/09 07:33:21]

As I understand we ignore invalid ranges just like http server does. So it's ok.
I return error in case of multiple ranges, because all job classes do the same.
I agree that it looks strange.

Maybe we should ignore multiple ranges (or support it where possible).
But I think it would be better to do in another cl, if it is needed.

On 2014/01/07 18:51:39, mmenke wrote:

[mmenke, 2014/01/09 15:49:23]

It just seems a little weird to me that if we see request for "bytes=0-49,goat"
we just ignore the header, while if we see "bytes=0-49,70-79", then we return an
error.  But reading over the spec, you're right - it specifically says servers
must ignore range headers with invalid data.

On 2014/01/09 07:33:21, kirr wrote:

+}
+
+}  // namespace net