Move some SpdyHeaderBlock instances from heap to stack.

Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905
Committed: https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c
Cr-Commit-Position: refs/heads/master@{#406586}

[Bence, 2016-07-14 15:08:13]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 15:08:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2016-07-14 16:25:51]

bnc@chromium.org changed reviewers:
+ xunjieli@chromium.org

[Bence, 2016-07-14 16:25:51]

xunjieli: PTAL.  Thank you.

[commit-bot: I haz the power, 2016-07-14 17:00:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 17:00:37]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-07-14 17:44:12]

The CL looks good! I am concerned that with the raw type, SpdyHeaderBlock, as
the function argument (e.g. in SpdyStream::SendRequestHeaders(SpdyHeaderBlock
request_headers)), the reader of the code would incorrectly assume that a copy
is being made. Using a std::unique_ptr wrapper would be clearer in this case,
since the reader of the code doesn't need to dig into the header file or
actually compile the code to know that SpdyHeaderBlock doesn't have copy and
assign operators.

I like to think that using "std::unique_ptr" means passing ownership, and using
"const ref" means otherwise. This is what I have seen in my limited experience
with Chromium codebase. This might a result of legacy code, since move semantics
is not supported until recently. I think it might worth getting a second opinion
and/or checking with chromium-dev.

[Bence, 2016-07-15 18:53:49]

Description was changed from

==========
Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905
==========

to

==========
Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905
==========

[Bence, 2016-07-15 18:54:39]

On 2016/07/14 17:44:12, xunjieli wrote:
> The CL looks good! I am concerned that with the raw type, SpdyHeaderBlock, as
> the function argument (e.g. in SpdyStream::SendRequestHeaders(SpdyHeaderBlock
> request_headers)), the reader of the code would incorrectly assume that a copy
> is being made. Using a std::unique_ptr wrapper would be clearer in this case,
> since the reader of the code doesn't need to dig into the header file or
> actually compile the code to know that SpdyHeaderBlock doesn't have copy and
> assign operators.
> 
> I like to think that using "std::unique_ptr" means passing ownership, and
using
> "const ref" means otherwise. This is what I have seen in my limited experience
> with Chromium codebase. This might a result of legacy code, since move
semantics
> is not supported until recently. I think it might worth getting a second
opinion
> and/or checking with chromium-dev.

Thank you for your feedback.  Let me put this CL on hold for the time being, so
that I can think about what would be the best way forward.

[Bence, 2016-07-20 15:08:14]

Helen: thank you for your feedback.  I checked in with chromium-dev@, see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/M6AoLHYCLqw,
and did not see any push back.  I also talked to rch@ offline, who assured me
that while move semantics are still relatively new, they are becoming more and
more popular, and there is no reason to hold back.

If you are still uncomfortable with this change, I'm open for discussion.

[xunjieli, 2016-07-20 15:30:46]

On 2016/07/20 15:08:14, Bence wrote:
> Helen: thank you for your feedback.  I checked in with chromium-dev@, see
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/M6AoLHYCLqw,
> and did not see any push back.  I also talked to rch@ offline, who assured me
> that while move semantics are still relatively new, they are becoming more and
> more popular, and there is no reason to hold back.
> 
> If you are still uncomfortable with this change, I'm open for discussion.

LGTM. Thanks for checking in with Chromium-dev. The email skipped my inbox and
didn't see it until now. For my future reference, the main argument here is that
SpdyHeaderBlock's fields are very few (2 std::unique_ptrs), so it shouldn't be
expensive to pass by value and move. There will be one fewer extra indirection.

[Bence, 2016-07-20 15:33:58]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2016-07-20 15:34:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 16:52:15]

Description was changed from

==========
Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905
==========

to

==========
Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905
==========

[commit-bot: I haz the power, 2016-07-20 16:52:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-20 16:52:24]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 16:54:30]

Description was changed from

==========
Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905
==========

to

==========
Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG=621905

Committed: https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c
Cr-Commit-Position: refs/heads/master@{#406586}
==========

[commit-bot: I haz the power, 2016-07-20 16:54:30]

Patchset 1 (id:??) landed as
https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c
Cr-Commit-Position: refs/heads/master@{#406586}