[Net] Assert that URLRequests with LOAD_IGNORE_LIMITS have MAXIMUM_PRIORITY

net/url_request/url_request.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 "net/url_request/url_request.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
@@ skipping 480 matching lines @@
 void URLRequest::GetCharset(string* charset) {
   DCHECK(job_.get());
   job_->GetCharset(charset);
 }
 
 int URLRequest::GetResponseCode() const {
   DCHECK(job_.get());
   return job_->GetResponseCode();
 }
 
+void URLRequest::SetLoadFlags(int flags) {
+  if ((load_flags_ & LOAD_IGNORE_LIMITS) != (flags & LOAD_IGNORE_LIMITS)) {
+    DCHECK(!job_);
+    DCHECK(flags & LOAD_IGNORE_LIMITS);

[mmenke, 2013/11/01 19:06:39]

So the IGNORE_LIMITS flag can be set, but not unset?  We DCHECK when we override
the priority due to MAX_LIMITS, anyways, so I think it's fine to clear the
flag...Isn't it?

I think the only case where we really have to DCHECK here is when setting the
flag, when there's either a job or priority is not MAXIMUM_PRIORITY.

[akalin, 2013/11/01 23:15:51]

It's fine, but the only use case doesn't ever clear the flag, right? I'd like to
keep the API simple until we actually need to relax the restriction.

+    DCHECK_EQ(priority_, MAXIMUM_PRIORITY);
+  }
+  load_flags_ = flags;
+
+  // This should be a no-op given the above DCHECKs, but do this
+  // anyway for release mode.
+  if ((load_flags_ & LOAD_IGNORE_LIMITS) != 0)
+    SetPriority(MAXIMUM_PRIORITY);
+}
+
 // static
 void URLRequest::SetDefaultCookiePolicyToBlock() {
   CHECK(!g_url_requests_started);
   g_default_can_use_cookies = false;
 }
 
 // static
 bool URLRequest::IsHandledProtocol(const std::string& scheme) {
   return URLRequestJobManager::GetInstance()->SupportsScheme(scheme);
 }
@@ skipping 442 matching lines @@
   int64 expected_content_size = -1;
   if (job_.get())
     expected_content_size = job_->expected_content_size();
 
   return expected_content_size;
 }
 
 void URLRequest::SetPriority(RequestPriority priority) {
   DCHECK_GE(priority, MINIMUM_PRIORITY);
   DCHECK_LE(priority, MAXIMUM_PRIORITY);
+
+  if (((load_flags_ & LOAD_IGNORE_LIMITS) != 0) &&

[mmenke, 2013/11/01 19:06:39]

optional nit:  Think !(load_flags_ & LOAD_IGNORE_LIMITS) is more intuitive

[akalin, 2013/11/01 23:15:51]

Done.

+      (priority != MAXIMUM_PRIORITY)) {

[mmenke, 2013/11/01 19:06:39]

nit:  Parens not needed.

[akalin, 2013/11/01 23:15:51]

Not needed, but I think it's clearer.

+    NOTREACHED();
+    // Maintain the invariant that requests with IGNORE_LIMITS set
+    // have MAXIMUM_PRIORITY for release mode.
+    priority = MAXIMUM_PRIORITY;

[mmenke, 2013/11/01 19:06:39]

optional nit:  In this case, priority_ will now be priority, so we can just
return early.  Doesn't save us anything, just think it makes the logic simpler.

[akalin, 2013/11/01 23:15:51]

Done.

+  }
+
   if (priority_ == priority)
     return;
 
   priority_ = priority;
   if (job_.get()) {
     net_log_.AddEvent(NetLog::TYPE_URL_REQUEST_SET_PRIORITY,
                       NetLog::IntegerCallback("priority", priority_));
     job_->SetPriority(priority_);
   }
 }
@@ skipping 186 matching lines @@
       new base::debug::StackTrace(NULL, 0);
   *stack_trace_copy = stack_trace;
   stack_trace_.reset(stack_trace_copy);
 }
 
 const base::debug::StackTrace* URLRequest::stack_trace() const {
   return stack_trace_.get();
 }
 
 }  // namespace net