[net] Change order of RequestPriority to natural: higher > lower

net/spdy/spdy_session.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/spdy/spdy_session.h"
 
 #include <map>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
@@ skipping 517 matching lines @@
   create_stream_queues_[priority].push(
       PendingCreateStream(url, priority, spdy_stream,
                           stream_net_log, callback));
   return ERR_IO_PENDING;
 }
 
 void SpdySession::ProcessPendingCreateStreams() {
   while (!max_concurrent_streams_ ||
          active_streams_.size() < max_concurrent_streams_) {
     bool no_pending_create_streams = true;
-    for (int i = 0;i < NUM_PRIORITIES;++i) {
+    for (int i = NUM_PRIORITIES - 1; i >= MINIMUM_PRIORITY; --i) {
       if (!create_stream_queues_[i].empty()) {
         PendingCreateStream pending_create = create_stream_queues_[i].front();
         create_stream_queues_[i].pop();
         no_pending_create_streams = false;
         int error = CreateStreamImpl(*pending_create.url,
                                      pending_create.priority,
                                      pending_create.spdy_stream,
                                      *pending_create.stream_net_log);
         scoped_refptr<SpdyStream>* stream = pending_create.spdy_stream;
         DCHECK(!ContainsKey(pending_callback_map_, stream));
@@ skipping 12 matching lines @@
 }
 
 void SpdySession::CancelPendingCreateStreams(
     const scoped_refptr<SpdyStream>* spdy_stream) {
   PendingCallbackMap::iterator it = pending_callback_map_.find(spdy_stream);
   if (it != pending_callback_map_.end()) {
     pending_callback_map_.erase(it);
     return;
   }
 
-  for (int i = 0;i < NUM_PRIORITIES;++i) {
+  for (int i = 0; i < NUM_PRIORITIES; ++i) {

[Ryan Hamilton, 2012/04/23 22:51:05]

Should this loop be reversed too, like the previous one?

[szym, 2012/04/24 00:19:31]

This loop is order-agnostic. It goes through all elements and its iterations are
independent. The previous loop breaks on first hit.

     PendingCreateStreamQueue tmp;
     // Make a copy removing this trans
     while (!create_stream_queues_[i].empty()) {
       PendingCreateStream pending_create = create_stream_queues_[i].front();
       create_stream_queues_[i].pop();
       if (pending_create.spdy_stream != spdy_stream)
         tmp.push(pending_create);
     }
     // Now copy it back
     while (!tmp.empty()) {
       create_stream_queues_[i].push(tmp.front());
       tmp.pop();
     }
   }
 }
 
 int SpdySession::CreateStreamImpl(
     const GURL& url,
     RequestPriority priority,
     scoped_refptr<SpdyStream>* spdy_stream,
     const BoundNetLog& stream_net_log) {
+  DCHECK_GE(priority, net::MINIMUM_PRIORITY);

[Ryan Hamilton, 2012/04/23 22:51:05]

nit: no need for net:: prefix

[szym, 2012/04/24 00:19:31]

Agreed.

+  DCHECK_LT(priority, net::NUM_PRIORITIES);
+
   // Make sure that we don't try to send https/wss over an unauthenticated, but
   // encrypted SSL socket.
   if (is_secure_ && certificate_error_code_ != OK &&
       (url.SchemeIs("https") || url.SchemeIs("wss"))) {
     CloseSessionOnError(
         static_cast<net::Error>(certificate_error_code_),
         true,
         "Tried to create SPDY stream for secure content over an "
         "unauthenticated session.");
     return ERR_SPDY_PROTOCOL_ERROR;
@@ skipping 12 matching lines @@
   stream->set_priority(priority);
   stream->set_path(path);
   stream->set_send_window_size(initial_send_window_size_);
   stream->set_recv_window_size(initial_recv_window_size_);
   ActivateStream(stream);
 
   UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdyPriorityCount",
       static_cast<int>(priority), 0, 10, 11);
 
   // TODO(mbelshe): Optimize memory allocations
-  DCHECK(priority >= net::HIGHEST && priority < net::NUM_PRIORITIES);
 
   DCHECK_EQ(active_streams_[stream_id].get(), stream.get());
   return OK;
 }
 
 bool SpdySession::NeedsCredentials() const {
   if (!is_secure_)
     return false;
   SSLClientSocket* ssl_socket = GetSSLClientSocket();
   if (ssl_socket->GetNegotiatedProtocol() < kProtoSPDY3)
@@ skipping 191 matching lines @@
   net_log().AddEvent(
       NetLog::TYPE_SPDY_SESSION_SEND_RST_STREAM,
       make_scoped_refptr(new NetLogSpdyRstParameter(stream_id, status,
                                                     description)));
 
   DCHECK(buffered_spdy_framer_.get());
   scoped_ptr<SpdyRstStreamControlFrame> rst_frame(
       buffered_spdy_framer_->CreateRstStream(stream_id, status));
 
   // Default to lowest priority unless we know otherwise.
-  int priority = 3;
+  RequestPriority priority = net::IDLE;

[Ryan Hamilton, 2012/04/23 22:51:05]

nit: no need for net::

[szym, 2012/04/24 00:19:31]

net::IDLE is shadowed by enum SpdySession::State IDLE.

[Ryan Hamilton, 2012/04/24 00:42:11]

I see.  *shakes fist at C++*  I'm looking fwd to c++ class enums to solve this
annoying issue.

[Ryan Hamilton, 2012/04/24 00:42:11]

I see.  *shakes fist at C++*  I'm looking fwd to c++ class enums to solve this
annoying issue.

   if(IsStreamActive(stream_id)) {
     scoped_refptr<SpdyStream> stream = active_streams_[stream_id];
     priority = stream->priority();
   }
   QueueFrame(rst_frame.get(), priority, NULL);
   DeleteStream(stream_id, ERR_SPDY_PROTOCOL_ERROR);
 }
 
 bool SpdySession::IsStreamActive(SpdyStreamId stream_id) const {
   return ContainsKey(active_streams_, stream_id);
@@ skipping 237 matching lines @@
       "spdy.abandoned_push_streams");
 
   if (!active_streams_.empty())
     abandoned_streams.Add(active_streams_.size());
   if (!unclaimed_pushed_streams_.empty()) {
     streams_abandoned_count_ += unclaimed_pushed_streams_.size();
     abandoned_push_streams.Add(unclaimed_pushed_streams_.size());
     unclaimed_pushed_streams_.clear();
   }
 
-  for (int i = 0;i < NUM_PRIORITIES;++i) {
+  for (int i = 0; i < NUM_PRIORITIES; ++i) {

[Ryan Hamilton, 2012/04/23 22:51:05]

Same question about reversing the loop

[szym, 2012/04/24 00:19:31]

Same answer. Order-agnostic, unless the order of calling
|pending_create.callback.Run(ERR_ABORTED)| matters. Does it?

[Ryan Hamilton, 2012/04/24 00:42:11]

Gotcha.  That makes sense.

     while (!create_stream_queues_[i].empty()) {
       PendingCreateStream pending_create = create_stream_queues_[i].front();
       create_stream_queues_[i].pop();
       pending_create.callback.Run(ERR_ABORTED);
     }
   }
 
   while (!active_streams_.empty()) {
     ActiveStreamMap::iterator it = active_streams_.begin();
     const scoped_refptr<SpdyStream>& stream = it->second;
@@ skipping 11 matching lines @@
 
 int SpdySession::GetNewStreamId() {
   int id = stream_hi_water_mark_;
   stream_hi_water_mark_ += 2;
   if (stream_hi_water_mark_ > 0x7fff)
     stream_hi_water_mark_ = 1;
   return id;
 }
 
 void SpdySession::QueueFrame(SpdyFrame* frame,
-                             SpdyPriority priority,
+                             RequestPriority priority,
                              SpdyStream* stream) {
   int length = SpdyFrame::kHeaderSize + frame->length();
   IOBuffer* buffer = new IOBuffer(length);
   memcpy(buffer->data(), frame->data(), length);
   queue_.push(SpdyIOBuffer(buffer, length, priority, stream));
 
   WriteSocketLater();
 }
 
 void SpdySession::CloseSessionOnError(net::Error err,
@@ skipping 594 matching lines @@
 
   net_log_.AddEvent(
       NetLog::TYPE_SPDY_SESSION_SEND_SETTINGS,
       make_scoped_refptr(new NetLogSpdySettingsParameter(settings_map_new)));
 
   // Create the SETTINGS frame and send it.
   DCHECK(buffered_spdy_framer_.get());
   scoped_ptr<SpdySettingsControlFrame> settings_frame(
       buffered_spdy_framer_->CreateSettings(settings_map_new));
   sent_settings_ = true;
-  QueueFrame(settings_frame.get(), 0, NULL);
+  QueueFrame(settings_frame.get(), HIGHEST, NULL);
 }
 
 void SpdySession::HandleSetting(uint32 id, uint32 value) {
   switch (id) {
     case SETTINGS_MAX_CONCURRENT_STREAMS:
       max_concurrent_streams_ = std::min(static_cast<size_t>(value),
                                          g_max_concurrent_stream_limit);
       ProcessPendingCreateStreams();
       break;
     case SETTINGS_INITIAL_WINDOW_SIZE:
@@ skipping 56 matching lines @@
 void SpdySession::SendTrailingPing() {
   DCHECK(trailing_ping_pending_);
   trailing_ping_pending_ = false;
   WritePingFrame(next_ping_id_);
 }
 
 void SpdySession::WritePingFrame(uint32 unique_id) {
   DCHECK(buffered_spdy_framer_.get());
   scoped_ptr<SpdyPingControlFrame> ping_frame(
       buffered_spdy_framer_->CreatePingFrame(next_ping_id_));
-  QueueFrame(
-      ping_frame.get(), buffered_spdy_framer_->GetHighestPriority(), NULL);
+  QueueFrame(ping_frame.get(), HIGHEST, NULL);
 
   if (net_log().IsLoggingAllEvents()) {
     net_log().AddEvent(
         NetLog::TYPE_SPDY_SESSION_PING,
         make_scoped_refptr(new NetLogSpdyPingParameter(next_ping_id_, "sent")));
   }
   if (unique_id % 2 != 0) {
     next_ping_id_ += 2;
     ++pings_in_flight_;
     need_to_send_ping_ = false;
@@ skipping 133 matching lines @@
 SSLClientSocket* SpdySession::GetSSLClientSocket() const {
   if (!is_secure_)
     return NULL;
   SSLClientSocket* ssl_socket =
       reinterpret_cast<SSLClientSocket*>(connection_->socket());
   DCHECK(ssl_socket);
   return ssl_socket;
 }
 
 }  // namespace net