New issue
Advanced search Search tips

Issue 810752 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Clean up spdy_test_util_common

Project Member Reported by b...@chromium.org, Feb 9 2018

Issue description

* Rename to spdy_test_util. (What does common mean here?)
* Remove unused SpdyHeaderInfo.
* Inline ConstructSpdyGoAway() to all three call sites.
* Remove "The caller takes ownership of the frame." comments.  (SpdySerializedFrame is constructed on the stack, not heap, so this does not apply any longer.)
* Consolidate multiple ConstructSpdyGet() signatures, possibly with some Java-like syntax: ConstructSpdyGet(stream_id).AddHeaders(...).SetPriority(RequestPriority), given that very few call sites add headers or use non-default priority.
* Same with multiple ConstructSpdyPush() signatures.
* Same with multiple ConstructSpdyPush() signatures.
* Same with multiple ConstructSpdyReplyError() signatures.
* Same with multiple ConstructSpdyDataFrame() signatures.
* See if every call site of ConstructSpdyPushPromise() can use ConstructSpdyPush() instead, and ConstructSpdyPushPromise() could be inlined or made private.
* Make ConstructSpdyDataFrame() take StringPiece instead of char* and uint.
* Possibly add ConstructSpdyGet().SetUrl() instead of set_default_url().



 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 11 2018

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

commit dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125
Author: Bence Béky <bnc@chromium.org>
Date: Sun Feb 11 01:32:29 2018

Misc cleanup: remove unused, fix comment, improve logging.

Change logging in StaticSocketDataHelper::VerifyWriteData() such that if
writted data does not match mock write, then print actual written data in full length, not
only truncated to the length of the mock write data.

Remove unused StaticSocketDataProvider:CompleteRead().

The rest of this CL belongs to https://crbug.com/810752:

Inline ConstructSpdyGoAway() as ConstructSpdyGoAway(0).

Remove unused struct SpdyHeaderInfo.

Remove obsolete "The caller takes ownership of the frame." comment from
spdy_test_util_common.h, leftover from when methods used to return raw
pointers.  This is a follow-up to
https://crrev.com/2156643002/diff/1/net/spdy/spdy_test_util_common.h.

Also remove useless "Returns a SpdySerializedFrame." comments from the
line above the definition showing return type SpdySerializedFrame.

Bug: 600438, 810752
Change-Id: I1f3471f4e4a9b71603257cf82986d0efe0288c7f
Reviewed-on: https://chromium-review.googlesource.com/912013
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535993}
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/socket/socket_test_util.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/socket/socket_test_util.h
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/spdy/chromium/spdy_test_util_common.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/spdy/chromium/spdy_test_util_common.h

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 20 2018

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

commit d74f4381d34aba88bc34cf54140b16fdb4ab428e
Author: Bence Béky <bnc@chromium.org>
Date: Tue Feb 20 18:26:19 2018

Change ConstructSpdyDataFrame() to take StringPiece.

This is a test-only change.

Change ConstructSpdyDataFrame() to take a base::StringPiece argument
instead of separate data and length.  To prevent headaches while writing
unittests if one forgets to change the length and has a hard time
figuring out why mock data does not make any sense.

Also move |content| local variable from heap to stack in multiple tests.

Also use std::string::data() instead of std::string::c_str() where
substrings are constructed in tests.

Also replace some potentially politically incorrect text with more
neutral text of the exact same length.

Bug: 810752
Change-Id: Ib22f9ab5c12be8e2c95bd68205995547fda4e9ba
Reviewed-on: https://chromium-review.googlesource.com/926421
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537823}
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/bidirectional_stream_spdy_impl_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_http_stream_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_proxy_client_socket_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_stream_unittest.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_test_util_common.cc
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/spdy/chromium/spdy_test_util_common.h
[modify] https://crrev.com/d74f4381d34aba88bc34cf54140b16fdb4ab428e/net/websockets/websocket_basic_stream_adapters_test.cc

Sign in to add a comment