New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 637407 link

Starred by 3 users
Status: Fixed
Owner:
█ rdsmith@chromium.org
Last visit > 30 days ago
Closed: Aug 2016
Cc:
rch@chromium.org
b...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

HttpServerProperties::SupportsRequestPriority() is misnamed for usage that probes connection sharing

Project Member Reported by rdsmith@chromium.org, Aug 12 2016 
Version: Top of Chromium tree (p410474)
OS: All

In HttpStreamFactoryImpl::Job::Preconnect there is the following code:

  HttpServerProperties* http_server_properties =
      session_->http_server_properties();
  if (http_server_properties &&
      http_server_properties->SupportsRequestPriority(
          url::SchemeHostPort(request_info_.url))) {
    num_streams_ = 1;
  } else {
    num_streams_ = num_streams;
  }

This code is clearly using SupportsRequestPriority() as an alias for "Shares all connections to a host", which seems fragile.  The name of the routine should be changed, or a new routine added with the same implementation.  

(It's a nit, but it annoyed me during a code review, so is getting a bug filed.  If either of you have opinions on this, please let me know--I'm happy to execute on them.)

Comment 1 by rch@chromium.org, Aug 12 2016 

No objection to renaming. Go for it! (SupportsRequestMultiplexing?)

That being said, I'm not sure it's actually fragile. If a connection does not support multiplexing, then I don't think it could really support priorities, could it?

But still, go for it! Clarity is always better.

Comment 2 by rdsmith@chromium.org, Aug 29 2016 

Hmmm.  Good point.  Clarity is good, but user interface minimality is also good, and if the two names really are equivalent, I don't feel a need to just create a synonym.  

I think what I'm inclined to do given your point is just update the comment on the method to indicate that supporting priorities also implies supporting multiplexing.  WDYT?

Comment 3 by rch@chromium.org, Aug 29 2016 

Oh, that seems like a good idea! +1
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 30 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c31e0606bacb84a3766bdcf75e93d044bdbe917e

commit c31e0606bacb84a3766bdcf75e93d044bdbe917e
Author: rdsmith <rdsmith@chromium.org>
Date: Tue Aug 30 06:27:23 2016

Make clear SupportsRequestPriority() also implies connection sharing.

BUG= 637407 
R=rch@chromium.org

Review-Url: https://codereview.chromium.org/2286373002
Cr-Commit-Position: refs/heads/master@{#415222}

[modify] https://crrev.com/c31e0606bacb84a3766bdcf75e93d044bdbe917e/net/http/http_server_properties.h

Comment 5 by rdsmith@chromium.org, Aug 30 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment