Skip to content

Commit

Permalink
SocketParams refactor 1: Use PrivacyMode enum in GroupId instead of b…
Browse files Browse the repository at this point in the history
…ools.

Enum classes are safer, and since we're going to use this to create
SocketParams objects, too, they're also simpler, as it's a bit weird
to convert from an enum class to a bool and then back to the original
enum class.

Bug: 533571
Change-Id: Iead421aca8137de7428f8e0434806bf8da3577b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1561962
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652915}
  • Loading branch information
Matt Menke authored and Commit Bot committed Apr 22, 2019
1 parent 05068dc commit bdf7778
Show file tree
Hide file tree
Showing 14 changed files with 86 additions and 70 deletions.
31 changes: 16 additions & 15 deletions net/http/http_network_transaction_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "net/base/load_timing_info.h"
#include "net/base/load_timing_info_test_util.h"
#include "net/base/net_errors.h"
#include "net/base/privacy_mode.h"
#include "net/base/proxy_delegate.h"
#include "net/base/proxy_server.h"
#include "net/base/request_priority.h"
Expand Down Expand Up @@ -11185,15 +11186,15 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForDirectConnections) {
"http://www.example.org/direct",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
false,
},
{
"", // unused
"http://[2001:1418:13:1::25]/direct",
ClientSocketPool::GroupId(HostPortPair("2001:1418:13:1::25", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
false,
},

Expand All @@ -11203,23 +11204,23 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForDirectConnections) {
"https://www.example.org/direct_ssl",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},
{
"", // unused
"https://[2001:1418:13:1::25]/direct",
ClientSocketPool::GroupId(HostPortPair("2001:1418:13:1::25", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},
{
"", // unused
"https://host.with.alternate/direct",
ClientSocketPool::GroupId(HostPortPair("host.with.alternate", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},
};
Expand Down Expand Up @@ -11254,7 +11255,7 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForHTTPProxyConnections) {
"http://www.example.org/http_proxy_normal",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
false,
},

Expand All @@ -11264,7 +11265,7 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForHTTPProxyConnections) {
"https://www.example.org/http_connect_ssl",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},

Expand All @@ -11273,7 +11274,7 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForHTTPProxyConnections) {
"https://host.with.alternate/direct",
ClientSocketPool::GroupId(HostPortPair("host.with.alternate", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},

Expand All @@ -11282,7 +11283,7 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForHTTPProxyConnections) {
"ftp://ftp.google.com/http_proxy_normal",
ClientSocketPool::GroupId(HostPortPair("ftp.google.com", 21),
ClientSocketPool::SocketType::kFtp,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
false,
},
};
Expand Down Expand Up @@ -11319,15 +11320,15 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForSOCKSConnections) {
"http://www.example.org/socks4_direct",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
false,
},
{
"socks5://socks_proxy:1080",
"http://www.example.org/socks5_direct",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
false,
},

Expand All @@ -11337,15 +11338,15 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForSOCKSConnections) {
"https://www.example.org/socks4_ssl",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},
{
"socks5://socks_proxy:1080",
"https://www.example.org/socks5_ssl",
ClientSocketPool::GroupId(HostPortPair("www.example.org", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},

Expand All @@ -11354,7 +11355,7 @@ TEST_F(HttpNetworkTransactionTest, GroupIdForSOCKSConnections) {
"https://host.with.alternate/direct",
ClientSocketPool::GroupId(HostPortPair("host.with.alternate", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */),
PrivacyMode::PRIVACY_MODE_DISABLED),
true,
},
};
Expand Down Expand Up @@ -14414,7 +14415,7 @@ TEST_F(HttpNetworkTransactionTest, MultiRoundAuth) {

const ClientSocketPool::GroupId kSocketGroup(
HostPortPair("www.example.com", 80), ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */);
PrivacyMode::PRIVACY_MODE_DISABLED);

// First round of authentication.
auth_handler->SetGenerateExpectation(false, OK);
Expand Down
8 changes: 4 additions & 4 deletions net/http/http_stream_factory_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -392,11 +392,11 @@ ClientSocketPool::GroupId GetGroupId(const TestCase& test) {
if (test.ssl) {
return ClientSocketPool::GroupId(HostPortPair("www.google.com", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */);
PrivacyMode::PRIVACY_MODE_DISABLED);
}
return ClientSocketPool::GroupId(HostPortPair("www.google.com", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */);
PrivacyMode::PRIVACY_MODE_DISABLED);
}

class CapturePreconnectsTransportSocketPool : public TransportClientSocketPool {
Expand All @@ -422,7 +422,7 @@ class CapturePreconnectsTransportSocketPool : public TransportClientSocketPool {
last_group_id_ = ClientSocketPool::GroupId(
HostPortPair(),
ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
true /* privacy_mode */);
PrivacyMode::PRIVACY_MODE_ENABLED);
}

int RequestSocket(
Expand Down Expand Up @@ -2010,7 +2010,7 @@ TEST_F(HttpStreamFactoryTest, NewSpdySessionCloseIdleH2Sockets) {
ssl_config, PRIVACY_MODE_DISABLED));
ClientSocketPool::GroupId group_id(host_port_pair,
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */);
PrivacyMode::PRIVACY_MODE_DISABLED);
int rv = connection->Init(
group_id,
ClientSocketPool::SocketParams::CreateFromSSLSocketParams(ssl_params),
Expand Down
5 changes: 3 additions & 2 deletions net/socket/client_socket_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,12 @@ ClientSocketPool::SocketParams::CreateFromHttpProxySocketParams(
ClientSocketPool::SocketParams::~SocketParams() = default;

ClientSocketPool::GroupId::GroupId()
: socket_type_(SocketType::kHttp), privacy_mode_(false) {}
: socket_type_(SocketType::kHttp),
privacy_mode_(PrivacyMode::PRIVACY_MODE_DISABLED) {}

ClientSocketPool::GroupId::GroupId(const HostPortPair& destination,
SocketType socket_type,
bool privacy_mode)
PrivacyMode privacy_mode)
: destination_(destination),
socket_type_(socket_type),
privacy_mode_(privacy_mode) {}
Expand Down
9 changes: 5 additions & 4 deletions net/socket/client_socket_pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "net/base/completion_once_callback.h"
#include "net/base/load_states.h"
#include "net/base/net_export.h"
#include "net/base/privacy_mode.h"
#include "net/base/request_priority.h"
#include "net/dns/host_resolver.h"
#include "net/http/http_request_info.h"
Expand Down Expand Up @@ -115,7 +116,7 @@ class NET_EXPORT ClientSocketPool : public LowerLayeredPool {
GroupId();
GroupId(const HostPortPair& destination,
SocketType socket_type,
bool privacy_mode);
PrivacyMode privacy_mode);
GroupId(const GroupId& group_id);

~GroupId();
Expand All @@ -127,7 +128,7 @@ class NET_EXPORT ClientSocketPool : public LowerLayeredPool {

SocketType socket_type() const { return socket_type_; }

bool privacy_mode() const { return privacy_mode_; }
PrivacyMode privacy_mode() const { return privacy_mode_; }

// Returns the group ID as a string, for logging.
std::string ToString() const;
Expand All @@ -150,8 +151,8 @@ class NET_EXPORT ClientSocketPool : public LowerLayeredPool {

SocketType socket_type_;

// True if this request is for a privacy mode / uncredentials connection.
bool privacy_mode_;
// If this request is for a privacy mode / uncredentialed connection.
PrivacyMode privacy_mode_;
};

// Callback to create a ConnectJob using the provided arguments. The lower
Expand Down
15 changes: 9 additions & 6 deletions net/socket/client_socket_pool_base_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "net/base/load_timing_info.h"
#include "net/base/load_timing_info_test_util.h"
#include "net/base/net_errors.h"
#include "net/base/privacy_mode.h"
#include "net/base/request_priority.h"
#include "net/base/test_completion_callback.h"
#include "net/http/http_response_headers.h"
Expand Down Expand Up @@ -69,11 +70,12 @@ const int kDefaultMaxSocketsPerGroup = 2;
constexpr base::TimeDelta kUnusedIdleSocketTimeout =
base::TimeDelta::FromSeconds(10);

ClientSocketPool::GroupId TestGroupId(const std::string& host,
int port = 80,
ClientSocketPool::SocketType socket_type =
ClientSocketPool::SocketType::kHttp,
bool privacy_mode = false) {
ClientSocketPool::GroupId TestGroupId(
const std::string& host,
int port = 80,
ClientSocketPool::SocketType socket_type =
ClientSocketPool::SocketType::kHttp,
PrivacyMode privacy_mode = PrivacyMode::PRIVACY_MODE_DISABLED) {
return ClientSocketPool::GroupId(HostPortPair(host, port), socket_type,
privacy_mode);
}
Expand Down Expand Up @@ -799,7 +801,8 @@ TEST_F(ClientSocketPoolBaseTest, GroupSeparation) {
ClientSocketPool::SocketType::kFtp,
};

const bool kPrivacyModes[] = {false, true};
const PrivacyMode kPrivacyModes[] = {PrivacyMode::PRIVACY_MODE_DISABLED,
PrivacyMode::PRIVACY_MODE_ENABLED};

int total_idle_sockets = 0;

Expand Down
4 changes: 2 additions & 2 deletions net/socket/client_socket_pool_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ scoped_refptr<ClientSocketPool::SocketParams> CreateSocketParamsAndGetGroupId(
}
}

*connection_group = ClientSocketPool::GroupId(
endpoint, socket_type, privacy_mode == PRIVACY_MODE_ENABLED);
*connection_group =
ClientSocketPool::GroupId(endpoint, socket_type, privacy_mode);

if (!proxy_info.is_direct()) {
ProxyServer proxy_server = proxy_info.proxy_server();
Expand Down
24 changes: 13 additions & 11 deletions net/socket/client_socket_pool_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <vector>

#include "net/base/host_port_pair.h"
#include "net/base/privacy_mode.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace net {
Expand All @@ -30,7 +31,8 @@ TEST(ClientSocketPool, GroupIdOperators) {
ClientSocketPool::SocketType::kFtp,
};

const bool kPrivacyModes[] = {false, true};
const PrivacyMode kPrivacyModes[] = {PrivacyMode::PRIVACY_MODE_DISABLED,
PrivacyMode::PRIVACY_MODE_ENABLED};

// All previously created |group_ids|. They should all be less than the
// current group under consideration.
Expand Down Expand Up @@ -69,57 +71,57 @@ TEST(ClientSocketPool, GroupIdToString) {
EXPECT_EQ("foo:80",
ClientSocketPool::GroupId(HostPortPair("foo", 80),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_DISABLED)
.ToString());
EXPECT_EQ("bar:443",
ClientSocketPool::GroupId(HostPortPair("bar", 443),
ClientSocketPool::SocketType::kHttp,
false /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_DISABLED)
.ToString());
EXPECT_EQ("pm/bar:80",
ClientSocketPool::GroupId(HostPortPair("bar", 80),
ClientSocketPool::SocketType::kHttp,
true /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_ENABLED)
.ToString());

EXPECT_EQ("ssl/foo:80",
ClientSocketPool::GroupId(HostPortPair("foo", 80),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_DISABLED)
.ToString());
EXPECT_EQ("ssl/bar:443",
ClientSocketPool::GroupId(HostPortPair("bar", 443),
ClientSocketPool::SocketType::kSsl,
false /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_DISABLED)
.ToString());
EXPECT_EQ("pm/ssl/bar:80",
ClientSocketPool::GroupId(HostPortPair("bar", 80),
ClientSocketPool::SocketType::kSsl,
true /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_ENABLED)
.ToString());

EXPECT_EQ("version-interference-probe/ssl/foo:443",
ClientSocketPool::GroupId(
HostPortPair("foo", 443),
ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
false /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_DISABLED)
.ToString());
EXPECT_EQ("pm/version-interference-probe/ssl/bar:444",
ClientSocketPool::GroupId(
HostPortPair("bar", 444),
ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
true /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_ENABLED)
.ToString());

EXPECT_EQ("ftp/foo:80",
ClientSocketPool::GroupId(HostPortPair("foo", 80),
ClientSocketPool::SocketType::kFtp,
false /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_DISABLED)
.ToString());
EXPECT_EQ("pm/ftp/bar:81",
ClientSocketPool::GroupId(HostPortPair("bar", 81),
ClientSocketPool::SocketType::kFtp,
true /* privacy_mode */)
PrivacyMode::PRIVACY_MODE_ENABLED)
.ToString());
}

Expand Down
Loading

0 comments on commit bdf7778

Please sign in to comment.