[Net] Propagate priority changes from URLRequest to HttpTransaction

net/url_request/url_request_ftp_job_unittest.cc

Index: net/url_request/url_request_ftp_job_unittest.cc
diff --git a/net/url_request/url_request_ftp_job_unittest.cc b/net/url_request/url_request_ftp_job_unittest.cc
index 61dde75326d363335db7e96822a22b15e36355f0..afaec3af5b827e8ae3d78255c606f112c5a337ab 100644
--- a/net/url_request/url_request_ftp_job_unittest.cc
+++ b/net/url_request/url_request_ftp_job_unittest.cc
@@ -2,12 +2,16 @@
 // 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"
@@ -49,6 +53,103 @@ class SimpleProxyConfigService : public ProxyConfigService {
   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,