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

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

BUG=124683
TEST=./net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133766

[szym, 2012-04-23 19:36:50]

mmenke: Please review non-spdy changes.

rch+willchan: Please review spdy changes.
Note that the new ordering has been pushed all the way to SpdyIOBuffer and
OutputQueue.

[willchan no longer on Chromium, 2012-04-23 20:12:13]

I'm going to defer to rch.

On Mon, Apr 23, 2012 at 12:36 PM, <szym@chromium.org> wrote:

> Reviewers: Matt Menke, willchan, Ryan Hamilton,
>
> Message:
> mmenke: Please review non-spdy changes.
>
> rch+willchan: Please review spdy changes.
> Note that the new ordering has been pushed all the way to SpdyIOBuffer and
> OutputQueue.
>
>
> Description:
> [net] Change order of RequestPriority to natural: higher > lower
>
> BUG=124683
> TEST=./net_unittests
>
>
> Please review this at
http://codereview.chromium.**org/10185007/<http://codereview.chromium.org/101...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M net/base/host_resolver_impl.cc
>  M net/base/prioritized_**dispatcher.h
>  M net/base/prioritized_**dispatcher.cc
>  M net/base/prioritized_**dispatcher_unittest.cc
>  M net/base/request_priority.h
>  M net/http/http_network_**transaction.cc
>  M net/socket/client_socket_pool_**base.cc
>  M net/spdy/buffered_spdy_framer.**h
>  M net/spdy/buffered_spdy_framer.**cc
>  M net/spdy/spdy_http_utils.cc
>  M net/spdy/spdy_io_buffer.h
>  M net/spdy/spdy_network_**transaction_spdy2_unittest.cc
>  M net/spdy/spdy_network_**transaction_spdy3_unittest.cc
>  M net/spdy/spdy_session.h
>  M net/spdy/spdy_session.cc
>  M net/spdy/spdy_session_spdy2_**unittest.cc
>  M net/spdy/spdy_session_spdy3_**unittest.cc
>  M net/spdy/spdy_stream.h
>  M net/spdy/spdy_stream.cc
>  M net/url_request/url_request.h
>
>
>

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

I think this mostly looks good.  But my brain is having a hard time in some
places understanding the right thing to do.  Sorry for being obtuse.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
File net/base/prioritized_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
net/base/prioritized_dispatcher.h:16: // A priority-based dispatcher of jobs.
Dispatch order is by priority (highest
I didn't expect to see a comment change in a consumer.  Was the old comment
incorrect, or is there a change in functionality?  (I think the former)

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
net/base/prioritized_dispatcher.h:35: // at priority 1 or 2, but the rest has to
be at 2.
nit: "the rest have to be" (instead of has)

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
File net/base/prioritized_dispatcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
net/base/prioritized_dispatcher_unittest.cc:21: COMPILE_ASSERT(MINIMUM_PRIORITY
== 0u &&
Please also assert that IDLE == 0 (or == MINIMUM_PRIORITY)

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/socket/client_...
File net/socket/client_socket_pool_base.cc (left):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/socket/client_...
net/socket/client_socket_pool_base.cc:335: DCHECK_GE(request->priority(), 0);
Should we DCHECK that priority is between HIGHEST and MINIMUM?

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/buffered_...
File net/spdy/buffered_spdy_framer.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/buffered_...
net/spdy/buffered_spdy_framer.h:113: SpdyPriority priority,
Nice catch!

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
File net/spdy/spdy_http_utils.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
net/spdy/spdy_http_utils.cc:131: DCHECK_GE(priority, MINIMUM_PRIORITY);
One minor problem.  The CQ is about to land this CL:

https://chromiumcodereview.appspot.com/10187006/

It re-writes this method, and in particular eliminates the SPDY_PRIORITY_HIGHEST
symbol.  Not a big deal, but just FYI.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
File net/spdy/spdy_io_buffer.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
net/spdy/spdy_io_buffer.h:27: SpdyIOBuffer(IOBuffer* buffer, int size, int
priority, SpdyStream* stream);
Can you make |priority| a RequestPriority?  It looks like
SpdySession::QueueFrame is construction SpdyIOBuffers with RequestPriority so I
hope this would be an easy change.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:571: for (int i = 0; i < NUM_PRIORITIES; ++i) {
Should this loop be reversed too, like the previous one?

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:593: DCHECK_GE(priority, net::MINIMUM_PRIORITY);
nit: no need for net:: prefix

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:839: RequestPriority priority = net::IDLE;
nit: no need for net::

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:1097: for (int i = 0; i < NUM_PRIORITIES; ++i) {
Same question about reversing the loop

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_stre...
File net/spdy/spdy_stream.cc (left):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_stre...
net/spdy/spdy_stream.cc:666: domain_bound_cert_,
static_cast<RequestPriority>(priority_));
Nice

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

Thanks a lot for the speedy review.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
File net/base/prioritized_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/base/prioritiz...
net/base/prioritized_dispatcher.h:16: // A priority-based dispatcher of jobs.
Dispatch order is by priority (highest
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> I didn't expect to see a comment change in a consumer.  Was the old comment
> incorrect, or is there a change in functionality?  (I think the former)

The old comment was misleading and a source of confusion both in the original
review (eroman@) and the readability review (stevez@). The functionality is the
same.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/buffered_...
File net/spdy/buffered_spdy_framer.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/buffered_...
net/spdy/buffered_spdy_framer.h:113: SpdyPriority priority,
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> Nice catch!

This helped a lot in distinguishing the priority before and after the SPDY
conversion.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
File net/spdy/spdy_http_utils.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
net/spdy/spdy_http_utils.cc:131: DCHECK_GE(priority, MINIMUM_PRIORITY);
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> One minor problem.  The CQ is about to land this CL:
> 
> https://chromiumcodereview.appspot.com/10187006/
> 
> It re-writes this method, and in particular eliminates the
SPDY_PRIORITY_HIGHEST
> symbol.  Not a big deal, but just FYI.

Thanks for the heads up.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
net/spdy/spdy_http_utils.cc:150: switch (priority) {
Do you think this switch would be better expressed as: |return
static_cast<SpdyPriority>(HIGHEST - priority)|.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
File net/spdy/spdy_io_buffer.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
net/spdy/spdy_io_buffer.h:27: SpdyIOBuffer(IOBuffer* buffer, int size, int
priority, SpdyStream* stream);
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> Can you make |priority| a RequestPriority?  It looks like
> SpdySession::QueueFrame is construction SpdyIOBuffers with RequestPriority so
I
> hope this would be an easy change.

I considered that. The only doubt was caused by the unit test which creates
SpdyIOBuffer with priorities 0..100. Would using undefined enum values be ok?

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:571: for (int i = 0; i < NUM_PRIORITIES; ++i) {
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> Should this loop be reversed too, like the previous one?

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

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:593: DCHECK_GE(priority, net::MINIMUM_PRIORITY);
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> nit: no need for net:: prefix

Agreed.

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:839: RequestPriority priority = net::IDLE;
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> nit: no need for net::

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

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:1097: for (int i = 0; i < NUM_PRIORITIES; ++i) {
On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> Same question about reversing the loop

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]

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
File net/spdy/spdy_http_utils.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_http...
net/spdy/spdy_http_utils.cc:150: switch (priority) {
On 2012/04/24 00:19:31, szym wrote:
> Do you think this switch would be better expressed as: |return
> static_cast<SpdyPriority>(HIGHEST - priority)|.

Yup, that sounds great.  The SPDY 2 case could be a similar expression, possibly
offset by 1 for IDLE and LOWEST.  (I did the moral equivalent of this in the new
code)

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
File net/spdy/spdy_io_buffer.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
net/spdy/spdy_io_buffer.h:27: SpdyIOBuffer(IOBuffer* buffer, int size, int
priority, SpdyStream* stream);
On 2012/04/24 00:19:31, szym wrote:
> On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> > Can you make |priority| a RequestPriority?  It looks like
> > SpdySession::QueueFrame is construction SpdyIOBuffers with RequestPriority
so
> I
> > hope this would be an easy change.
> 
> I considered that. The only doubt was caused by the unit test which creates
> SpdyIOBuffer with priorities 0..100. Would using undefined enum values be ok?

Hm.  I think I would be inclined to change the class to use RequestPriority, and
change the test to only create NUM_PRIORITIES priorities.  I fear that having a
class with SPDY in the name that takes an int priority but whose value is not a
SPDY priority, is just asking for problems :>  (You updated the comment, which
is awesome, but I'm still afraid I'd do the wrong thing in the future).  Work
for you?

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:839: RequestPriority priority = net::IDLE;
On 2012/04/24 00:19:31, szym wrote:
> On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> > nit: no need for net::
> 
> net::IDLE is shadowed by enum SpdySession::State IDLE.

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

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:839: RequestPriority priority = net::IDLE;
On 2012/04/24 00:19:31, szym wrote:
> On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> > nit: no need for net::
> 
> net::IDLE is shadowed by enum SpdySession::State IDLE.

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

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:1097: for (int i = 0; i < NUM_PRIORITIES; ++i) {
On 2012/04/24 00:19:31, szym wrote:
> On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> > Same question about reversing the loop
> 
> Same answer. Order-agnostic, unless the order of calling
> |pending_create.callback.Run(ERR_ABORTED)| matters. Does it?

Gotcha.  That makes sense.

[szym, 2012-04-24 00:47:57]

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
File net/spdy/spdy_io_buffer.h (right):

https://chromiumcodereview.appspot.com/10185007/diff/13003/net/spdy/spdy_io_b...
net/spdy/spdy_io_buffer.h:27: SpdyIOBuffer(IOBuffer* buffer, int size, int
priority, SpdyStream* stream);
On 2012/04/24 00:42:11, Ryan Hamilton wrote:
> On 2012/04/24 00:19:31, szym wrote:
> > On 2012/04/23 22:51:05, Ryan Hamilton wrote:
> > > Can you make |priority| a RequestPriority?  It looks like
> > > SpdySession::QueueFrame is construction SpdyIOBuffers with RequestPriority
> so
> > I
> > > hope this would be an easy change.
> > 
> > I considered that. The only doubt was caused by the unit test which creates
> > SpdyIOBuffer with priorities 0..100. Would using undefined enum values be
ok?
> 
> Hm.  I think I would be inclined to change the class to use RequestPriority,
and
> change the test to only create NUM_PRIORITIES priorities.  I fear that having
a
> class with SPDY in the name that takes an int priority but whose value is not
a
> SPDY priority, is just asking for problems :>  (You updated the comment, which
> is awesome, but I'm still afraid I'd do the wrong thing in the future).  Work
> for you?

Yup. I'm on it.

[mmenke, 2012-04-24 14:30:16]

Just one additional nit.

http://codereview.chromium.org/10185007/diff/13003/net/base/prioritized_dispa...
File net/base/prioritized_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10185007/diff/13003/net/base/prioritized_dispa...
net/base/prioritized_dispatcher_unittest.cc:21: COMPILE_ASSERT(MINIMUM_PRIORITY
== 0u &&
nit:  Suggest you switch the order of the above two lines, so you have:

a < b &&
b < c

[szym, 2012-04-24 18:15:25]

Rebased. Please, take another look.

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_http_utils.c...
net/spdy/spdy_http_utils.cc:130: request_priority_incompatible_with_spdy);
Please, take a look at this assert and the mapping.

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_session_spdy...
File net/spdy/spdy_session_spdy2_unittest.cc (right):

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy2_unittest.cc:77: // Insert items with random priority
and increasing buffer size.
Please, take a look at the new test.

[Ryan Hamilton, 2012-04-24 18:45:35]

lgtm

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_http_utils.c...
net/spdy/spdy_http_utils.cc:130: request_priority_incompatible_with_spdy);
On 2012/04/24 18:15:25, szym wrote:
> Please, take a look at this assert and the mapping.

NICE!  Love this assert.

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_session_spdy...
File net/spdy/spdy_session_spdy2_unittest.cc (right):

http://codereview.chromium.org/10185007/diff/23001/net/spdy/spdy_session_spdy...
net/spdy/spdy_session_spdy2_unittest.cc:77: // Insert items with random priority
and increasing buffer size.
On 2012/04/24 18:15:25, szym wrote:
> Please, take a look at the new test.

Cool.

[mmenke, 2012-04-24 19:23:50]

LGTM as well.

[commit-bot: I haz the power, 2012-04-24 19:28:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10185007/24006

[commit-bot: I haz the power, 2012-04-24 21:09:09]

Change committed as 133766