Fix bug in net::RequestPriority -> HTTP/2 dependency conversion.

net/spdy/http2_priority_dependencies.cc

+// Copyright 2016 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/http2_priority_dependencies.h"
+
+#include "base/metrics/field_trial.h"
+
+namespace net {
+
+// Field trial constants
+const char kSpdyDependenciesFieldTrial[] = "SpdyEnableDependencies";
+const char kSpdyDepencenciesFieldTrialDisable[] = "Disable";
+
+// Whether the creation of SPDY dependencies based on priority is
+// enabled by default.
+static bool priority_dependency_enabled_default = true;
+
+Http2PriorityDependencies::Http2PriorityDependencies()
+    : enabled_(priority_dependency_enabled_default) {
+  if (base::FieldTrialList::FindFullName(kSpdyDependenciesFieldTrial) ==
+      kSpdyDepencenciesFieldTrialDisable) {
+    enabled_ = false;
+  }
+}
+
+Http2PriorityDependencies::~Http2PriorityDependencies() {}
+
+bool Http2PriorityDependencies::SetEnabledDefaultForTesting(bool enabled) {
+  bool result = priority_dependency_enabled_default;
+  priority_dependency_enabled_default = enabled;
+  return result;
+}
+
+void Http2PriorityDependencies::OnStreamSynSent(
+    SpdyStreamId id,
+    RequestPriority priority,
+    SpdyStreamId* dependent_stream_id,
+    bool* exclusive) {
+  if (!enabled_) {
+    *dependent_stream_id = 0;
+    *exclusive = false;
+    return;
+  }
+
+  DCHECK(entry_by_stream_id_.end() == entry_by_stream_id_.find(id));

[Bence, 2016/03/10 20:21:13]

I'm not sure it is customary to use Yoda condition as in DCHECK(expected ==
actual), I would write DCHECK(actual == expected) instead (though I am aware of
the DCHECK(expected, actual) order).

[Randy Smith (Not in Mondays), 2016/03/16 19:38:10]

I've seen at least one tear on chromium-dev supporting the expected, actual
order, but I don't think it's policy and I agree with you about readability. 
Done.

+
+  *dependent_stream_id = 0ul;
+  *exclusive = true;
+
+  // Find the next highest entry in total order.
+  for (int i = priority; i <= HIGHEST; ++i) {
+    if (!id_priority_lists_[i].empty()) {
+      *dependent_stream_id = id_priority_lists_[i].back().first;
+      break;
+    }
+  }
+
+  id_priority_lists_[priority].push_back(std::make_pair(id, priority));
+  IdList::iterator it = id_priority_lists_[priority].end();
+  entry_by_stream_id_[id] = --it;

[Bence, 2016/03/10 20:21:13]

Nit: use back instead of end-1.

[Randy Smith (Not in Mondays), 2016/03/16 19:38:10]

The problem with back() is that it returns the entry, and not an iterator.  We
need an iterator because that's what you use to erase entries in constant time.

[Bence, 2016/03/17 01:48:59]

Sorry, I missed that.  Okay.

+}
+
+void Http2PriorityDependencies::OnStreamDestruction(SpdyStreamId id) {
+  if (!enabled_)
+    return;
+
+  EntryMap::iterator emit = entry_by_stream_id_.find(id);
+
+  // This routine may be called without a matching call to
+  // OnStreamSynSent above, in the case of server push.  In that case,
+  // it's a no-op.
+  if (emit == entry_by_stream_id_.end())
+    return;

[Bence, 2016/03/10 20:21:13]

Nit: add empty line after return.

[Randy Smith (Not in Mondays), 2016/03/16 19:38:10]

Done.

+  id_priority_lists_[emit->second->second].erase(emit->second);

[Bence, 2016/03/10 20:21:14]

Optional: |IdList::iterator it = emit->second| for better readability.

[Randy Smith (Not in Mondays), 2016/03/16 19:38:10]

Done.

+  entry_by_stream_id_.erase(emit);
+}
+
+}  // namespace net