[Net] Propagate priority changes from URLRequest to HttpTransaction

net/url_request/url_request_ftp_job_unittest.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.
 
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/run_loop.h"
+#include "googleurl/src/gurl.h"
+#include "net/http/http_transaction_unittest.h"
 #include "net/proxy/proxy_config_service.h"
 #include "net/socket/socket_test_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_ftp_job.h"

[mmenke, 2013/03/21 15:39:23]

nit:  This should go first, up top.

[akalin, 2013/03/21 23:43:48]

Done.

 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
 namespace {
 
 class SimpleProxyConfigService : public ProxyConfigService {
  public:
@@ skipping 21 matching lines @@
   void IncrementConfigId() {
     config_.set_id(config_.id() + 1);
     observer_->OnProxyConfigChanged(config_, ProxyConfigService::CONFIG_VALID);
   }
 
  private:
   ProxyConfig config_;
   Observer* observer_;
 };
 
+// Inherit from URLRequestFtpJob to expose the priority and some
+// other hidden functions.
+class TestURLRequestFtpJob : public URLRequestFtpJob {
+ public:
+  explicit TestURLRequestFtpJob(URLRequest* request)
+      : URLRequestFtpJob(request, NULL,
+                         request->context()->ftp_transaction_factory(),
+                         request->context()->ftp_auth_cache()) {}
+
+  using URLRequestFtpJob::SetPriority;
+  using URLRequestFtpJob::Start;
+  using URLRequestFtpJob::Kill;
+  using URLRequestFtpJob::priority;
+
+ protected:
+  virtual ~TestURLRequestFtpJob() {}
+};
+
+// Fixture for priority-related tests. Priority matters when there is
+// an HTTP proxy.
+class URLRequestFtpJobPriorityTest : public testing::Test {
+ protected:
+  URLRequestFtpJobPriorityTest()
+      : req_(GURL("ftp://ftp.example.com"), &delegate_, &context_, NULL) {
+    context_.set_proxy_service(
+        new ProxyService(new SimpleProxyConfigService, NULL, NULL));
+    context_.set_http_transaction_factory(&network_layer_);
+  }
+
+  MockNetworkLayer network_layer_;
+  TestURLRequestContext context_;
+  TestDelegate delegate_;
+  TestURLRequest req_;
+};
+
+// Make sure that SetPriority actually sets the URLRequestFtpJob's
+// priority, both before and after start.
+TEST_F(URLRequestFtpJobPriorityTest, SetPriorityBasic) {
+  scoped_refptr<TestURLRequestFtpJob> job(new TestURLRequestFtpJob(&req_));
+  EXPECT_EQ(DEFAULT_PRIORITY, job->priority());

[mmenke, 2013/03/21 15:39:23]

Since this should be different than the LOW below, think we should explicitly
set a priority here, rather than using default.

[akalin, 2013/03/21 23:43:48]

Hmm, I don't understand this comment. This is just verifying that a new FtpJob
starts off with DEFAULT_PRIORITY. Note that DEFAULT_PRIORITY == LOWEST, so the
SetPriority() call below does change the priority.

[mattmenke_gmail.com, 2013/03/21 23:48:36]

This test relies on DEFAULT_PRIORITY not being LOW.  If we wanted to have all
code that uses DEFAULT_PRIORITY to know that it is LOWEST, then I'd argue that
using a separate name for DEFAULT_PRIORITY serves no purpose.

As-is, it's not obvious, looking at this test, that SetPriority(LOW) is not the
same as SetPriority(DEFAULT_PRIORITY).

Please do change this.

[akalin, 2013/03/22 00:16:36]

I'm not sure what you want me to do. Should I add an EXPECT_NE(DEFAULT_PRIORITY,
LOW)? Add another call to SetPriority with HIGH?

+
+  job->SetPriority(LOW);
+  EXPECT_EQ(LOW, job->priority());
+
+  job->Start();
+  EXPECT_EQ(LOW, job->priority());
+
+  job->SetPriority(MEDIUM);
+  EXPECT_EQ(MEDIUM, job->priority());
+}
+
+// Make sure that URLRequestFtpJob passes on its priority to its
+// transaction on start.
+TEST_F(URLRequestFtpJobPriorityTest, SetTransactionPriorityOnStart) {
+  scoped_refptr<TestURLRequestFtpJob> job(new TestURLRequestFtpJob(&req_));
+  job->SetPriority(LOW);
+
+  EXPECT_FALSE(network_layer_.last_transaction());
+
+  job->Start();
+
+  ASSERT_TRUE(network_layer_.last_transaction());
+  EXPECT_EQ(LOW, network_layer_.last_transaction()->priority());
+}
+
+// Make sure that URLRequestFtpJob passes on its priority updates to
+// its transaction.
+TEST_F(URLRequestFtpJobPriorityTest, SetTransactionPriority) {
+  scoped_refptr<TestURLRequestFtpJob> job(new TestURLRequestFtpJob(&req_));
+  job->SetPriority(LOW);
+  job->Start();
+  ASSERT_TRUE(network_layer_.last_transaction());
+  EXPECT_EQ(LOW, network_layer_.last_transaction()->priority());
+
+  job->SetPriority(HIGHEST);
+  EXPECT_EQ(HIGHEST, network_layer_.last_transaction()->priority());
+}
+
+// Make sure that URLRequestFtpJob passes on its priority updates to
+// newly-created transactions after the first one.
+TEST_F(URLRequestFtpJobPriorityTest, SetSubsequentTransactionPriority) {
+  scoped_refptr<TestURLRequestFtpJob> job(new TestURLRequestFtpJob(&req_));
+  job->Start();
+
+  job->SetPriority(LOW);
+  ASSERT_TRUE(network_layer_.last_transaction());
+  EXPECT_EQ(LOW, network_layer_.last_transaction()->priority());
+
+  job->Kill();
+  network_layer_.ClearLastTransaction();
+
+  // Creates a second transaction.
+  job->Start();
+  ASSERT_TRUE(network_layer_.last_transaction());
+  EXPECT_EQ(LOW, network_layer_.last_transaction()->priority());
+}
+
 class FtpTestURLRequestContext : public TestURLRequestContext {
  public:
   FtpTestURLRequestContext(ClientSocketFactory* socket_factory,
                            ProxyService* proxy_service,
                            NetworkDelegate* network_delegate)
       : TestURLRequestContext(true) {
     set_client_socket_factory(socket_factory);
     context_storage_.set_proxy_service(proxy_service);
     set_network_delegate(network_delegate);
     Init();
@@ skipping 288 matching lines @@
   EXPECT_TRUE(url_request2.status().is_success());
   EXPECT_EQ(2, network_delegate()->completed_requests());
   EXPECT_EQ(0, network_delegate()->error_count());
   EXPECT_FALSE(request_delegate2.auth_required_called());
   EXPECT_EQ("test2.html", request_delegate2.data_received());
 }
 
 }  // namespace
 
 }  // namespace net