[SPDY] Remove some setters in SpdyStream

[SPDY] Remove some setters in SpdyStream

Instead pass parameters into SpdyStream's constructor. This simplifies SpdyStream, as we can now be sure that its priority is fixed.

Add function to convert from SpdyPriority to RequestPriority, and use them to set the correct priority for push streams.

Replace uses of Unretained() with weak pointers.

BUG=176582
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190481

[akalin, 2013-03-24 06:20:33]

+rch for review

[Ryan Hamilton, 2013-03-24 15:08:40]

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_http_utils.cc#n...
net/spdy/spdy_http_utils.cc:153: // Handle invalid values gracefully, and pick
LOW to map 2 back
nit: outdent

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h#oldco...
net/spdy/spdy_session.h:203: DISALLOW_COPY_AND_ASSIGN(SpdyIOBufferProducer);
How come this needs to be removed?

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_stream.h#oldcod...
net/spdy/spdy_stream.h:142: void set_priority(RequestPriority priority) {
priority_ = priority; }
Just FYI, with SPDY/4, a stream's priority is mutable.  (via a REPRI flag)

[akalin, 2013-03-24 19:24:53]

PTAL

Trybots are wigging out (the tests pass but they're still marked as red).

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_http_utils.cc#n...
net/spdy/spdy_http_utils.cc:153: // Handle invalid values gracefully, and pick
LOW to map 2 back
On 2013/03/24 15:08:40, Ryan Hamilton wrote:
> nit: outdent

Done.

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h#oldco...
net/spdy/spdy_session.h:203: DISALLOW_COPY_AND_ASSIGN(SpdyIOBufferProducer);
On 2013/03/24 15:08:40, Ryan Hamilton wrote:
> How come this needs to be removed?

It doesn't have to, but on the other hand it doesn't make sense. One cannot
instantiate a SpdyIOBufferProducer by itself since it's abstract, so there would
be no occasion for the copy/assign to happen with it.

The only thing this prevents is subclass instances being copyable/assignable,
but that seems like something that's best left for individual subclasses to
decide.

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_stream.h#oldcod...
net/spdy/spdy_stream.h:142: void set_priority(RequestPriority priority) {
priority_ = priority; }
On 2013/03/24 15:08:40, Ryan Hamilton wrote:
> Just FYI, with SPDY/4, a stream's priority is mutable.  (via a REPRI flag)

I know. :) I'm thinking about how to handle that. I don't like exposing
set_priority() since we only really want the SpdySession to be able to set a
stream's priority (after adjusting all the queues, etc.) and not the delegate.
Maybe I'll move it out of the stream entirely, but I don't know how practical
that is.

[Ryan Hamilton, 2013-03-25 17:07:47]

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h#oldco...
net/spdy/spdy_session.h:203: DISALLOW_COPY_AND_ASSIGN(SpdyIOBufferProducer);
On 2013/03/24 19:24:53, akalin wrote:
> On 2013/03/24 15:08:40, Ryan Hamilton wrote:
> > How come this needs to be removed?
> 
> It doesn't have to, but on the other hand it doesn't make sense. One cannot
> instantiate a SpdyIOBufferProducer by itself since it's abstract, so there
would
> be no occasion for the copy/assign to happen with it.
> 
> The only thing this prevents is subclass instances being copyable/assignable,
> but that seems like something that's best left for individual subclasses to
> decide.

But the style guide says to only allow copy constructors when they're required,
otherwise disallow them.  To me, this says to use the macro unless we *NEED* the
copy construction behavior.  Since we don't need it it this case, it seems like
the style guide requires in.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_stream.h#oldcod...
net/spdy/spdy_stream.h:142: void set_priority(RequestPriority priority) {
priority_ = priority; }
On 2013/03/24 19:24:53, akalin wrote:
> On 2013/03/24 15:08:40, Ryan Hamilton wrote:
> > Just FYI, with SPDY/4, a stream's priority is mutable.  (via a REPRI flag)
> 
> I know. :)

Heh, I figured you did :>

> I'm thinking about how to handle that. I don't like exposing
> set_priority() since we only really want the SpdySession to be able to set a
> stream's priority (after adjusting all the queues, etc.) and not the delegate.
> Maybe I'll move it out of the stream entirely, but I don't know how practical
> that is.

Interesting.  That seems like a good idea.  Though we have also discussed
allowing the delegate to change the priority of a stream when, for example, we
discover that a higher priority resource needs to be fetched, or if a background
tab becomes foreground.  Perhaps what this says is that the Session needs to
"own" the priority, and if there is a SetPriority method exposed on the stream
that it should merely delegate to the Session's SetStreamPriority method.  (or
something like that)

[akalin, 2013-03-25 19:24:48]

PTAL!

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (left):

https://codereview.chromium.org/12989038/diff/1/net/spdy/spdy_session.h#oldco...
net/spdy/spdy_session.h:203: DISALLOW_COPY_AND_ASSIGN(SpdyIOBufferProducer);
On 2013/03/25 17:07:47, Ryan Hamilton wrote:
> On 2013/03/24 19:24:53, akalin wrote:
> > On 2013/03/24 15:08:40, Ryan Hamilton wrote:
> > > How come this needs to be removed?
> > 
> > It doesn't have to, but on the other hand it doesn't make sense. One cannot
> > instantiate a SpdyIOBufferProducer by itself since it's abstract, so there
> would
> > be no occasion for the copy/assign to happen with it.
> > 
> > The only thing this prevents is subclass instances being
copyable/assignable,
> > but that seems like something that's best left for individual subclasses to
> > decide.
> 
> But the style guide says to only allow copy constructors when they're
required,
> otherwise disallow them.  To me, this says to use the macro unless we *NEED*
the
> copy construction behavior.  Since we don't need it it this case, it seems
like
> the style guide requires in.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...

Hmm you're right. Okay, put it back in.

[akalin, 2013-03-25 19:33:56]

Oops, did some digging, looks like DISALLOW_COPY_AND_ASSIGN is intended only for
concrete classes.

I'll see if I can get a wording clarification into the style guide.

Removed the macro again, PTAL!

[Ryan Hamilton, 2013-03-25 19:38:45]

lgtm

[commit-bot: I haz the power, 2013-03-25 19:44:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/12989038/19001

[akalin, 2013-03-25 21:11:19]

Committed patchset #5 manually as r190481 (presubmit successful).