AK: Make URL::m_port an Optional<u16>, Expose raw port getter

Our current way of signalling a missing port with m_port == 0 was lacking, as 0 is a valid port number in URLs.
fleximus · Sep 13, 2021 · d6cfa34 · d6cfa34
1 parent 1c9c437
commit d6cfa34
Show file tree

Hide file tree

Showing 15 changed files with 42 additions and 41 deletions.
diff --git a/AK/URL.cpp b/AK/URL.cpp
@@ -71,13 +71,13 @@ void URL::set_host(String host)
  m_valid = compute_validity();
 }
 
-void URL::set_port(u16 port)
+void URL::set_port(Optional<u16> port)
 {
  if (port == default_port_for_scheme(m_scheme)) {
- m_port = 0;
+ m_port = {};
  return;
  }
- m_port = port;
+ m_port = move(port);
  m_valid = compute_validity();
 }
 
@@ -234,8 +234,8 @@ String URL::serialize(ExcludeFragment exclude_fragment) const
  }
 
  builder.append(m_host);
- if (m_port != 0)
- builder.appendff(":{}", m_port);
+ if (m_port.has_value())
+ builder.appendff(":{}", *m_port);
  }
 
  if (cannot_be_a_base_url()) {
@@ -278,8 +278,8 @@ String URL::serialize_for_display() const
  if (!m_host.is_null()) {
  builder.append("//");
  builder.append(m_host);
- if (m_port != 0)
- builder.appendff(":{}", m_port);
+ if (m_port.has_value())
+ builder.appendff(":{}", *m_port);
  }
 
  if (cannot_be_a_base_url()) {
@@ -329,8 +329,8 @@ String URL::serialize_origin() const
  builder.append(m_scheme);
  builder.append(":https://"sv);
  builder.append(m_host);
- if (m_port != 0)
- builder.append(":{}", m_port);
+ if (m_port.has_value())
+ builder.append(":{}", *m_port);
  return builder.build();
 }
 

diff --git a/AK/URL.h b/AK/URL.h
@@ -56,7 +56,8 @@ class URL {
  Vector<String> const& paths() const { return m_paths; }
  String const& query() const { return m_query; }
  String const& fragment() const { return m_fragment; }
- u16 port() const { return m_port ? m_port : default_port_for_scheme(m_scheme); }
+ Optional<u16> port() const { return m_port; }
+ u16 port_or_default() const { return m_port.value_or(default_port_for_scheme(m_scheme)); }
  bool cannot_be_a_base_url() const { return m_cannot_be_a_base_url; }
  bool cannot_have_a_username_or_password_or_port() const { return m_host.is_null() || m_host.is_empty() || m_cannot_be_a_base_url || m_scheme == "file"sv; }
 
@@ -68,7 +69,7 @@ class URL {
  void set_username(String);
  void set_password(String);
  void set_host(String);
- void set_port(u16);
+ void set_port(Optional<u16>);
  void set_paths(Vector<String>);
  void set_query(String);
  void set_fragment(String);
@@ -129,8 +130,8 @@ class URL {
  String m_username;
  String m_password;
  String m_host;
- // NOTE: If the port is the default port for the scheme, m_port should be 0.
- u16 m_port { 0 };
+ // NOTE: If the port is the default port for the scheme, m_port should be empty.
+ Optional<u16> m_port;
  String m_path;
  Vector<String> m_paths;
  String m_query;

diff --git a/AK/URLParser.cpp b/AK/URLParser.cpp
@@ -472,7 +472,7 @@ URL URLParser::parse(StringView const& raw_input, URL const* base_url, Optional<
  return {};
  }
  if (port.value() == URL::default_port_for_scheme(url->scheme()))
- url->m_port = 0;
+ url->m_port = {};
  else
  url->m_port = port.value();
  buffer.clear();

diff --git a/Tests/AK/TestURL.cpp b/Tests/AK/TestURL.cpp
@@ -21,7 +21,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "http");
  EXPECT_EQ(url.host(), "www.serenityos.org");
- EXPECT_EQ(url.port(), 80);
+ EXPECT_EQ(url.port_or_default(), 80);
  EXPECT_EQ(url.path(), "/");
  EXPECT(url.query().is_null());
  EXPECT(url.fragment().is_null());
@@ -31,7 +31,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "https");
  EXPECT_EQ(url.host(), "www.serenityos.org");
- EXPECT_EQ(url.port(), 443);
+ EXPECT_EQ(url.port_or_default(), 443);
  EXPECT_EQ(url.path(), "/index.html");
  EXPECT(url.query().is_null());
  EXPECT(url.fragment().is_null());
@@ -41,7 +41,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "https");
  EXPECT_EQ(url.host(), "localhost");
- EXPECT_EQ(url.port(), 1234);
+ EXPECT_EQ(url.port_or_default(), 1234);
  EXPECT_EQ(url.path(), "/~anon/test/page.html");
  EXPECT(url.query().is_null());
  EXPECT(url.fragment().is_null());
@@ -51,7 +51,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "http");
  EXPECT_EQ(url.host(), "www.serenityos.org");
- EXPECT_EQ(url.port(), 80);
+ EXPECT_EQ(url.port_or_default(), 80);
  EXPECT_EQ(url.path(), "/index.html");
  EXPECT_EQ(url.query(), "");
  EXPECT_EQ(url.fragment(), "");
@@ -61,7 +61,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "http");
  EXPECT_EQ(url.host(), "www.serenityos.org");
- EXPECT_EQ(url.port(), 80);
+ EXPECT_EQ(url.port_or_default(), 80);
  EXPECT_EQ(url.path(), "/index.html");
  EXPECT_EQ(url.query(), "foo=1&bar=2");
  EXPECT(url.fragment().is_null());
@@ -71,7 +71,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "http");
  EXPECT_EQ(url.host(), "www.serenityos.org");
- EXPECT_EQ(url.port(), 80);
+ EXPECT_EQ(url.port_or_default(), 80);
  EXPECT_EQ(url.path(), "/index.html");
  EXPECT(url.query().is_null());
  EXPECT_EQ(url.fragment(), "fragment");
@@ -81,7 +81,7 @@ TEST_CASE(basic)
  EXPECT_EQ(url.is_valid(), true);
  EXPECT_EQ(url.scheme(), "http");
  EXPECT_EQ(url.host(), "www.serenityos.org");
- EXPECT_EQ(url.port(), 80);
+ EXPECT_EQ(url.port_or_default(), 80);
  EXPECT_EQ(url.path(), "/index.html");
  EXPECT_EQ(url.query(), "foo=1&bar=2&baz=/?");
  EXPECT_EQ(url.fragment(), "frag/ment?test#");
@@ -105,7 +105,7 @@ TEST_CASE(some_bad_urls)
 TEST_CASE(serialization)
 {
  EXPECT_EQ(URL("https://www.serenityos.org/").serialize(), "https://www.serenityos.org/");
- EXPECT_EQ(URL("https://www.serenityos.org:0/").serialize(), "https://www.serenityos.org/");
+ EXPECT_EQ(URL("https://www.serenityos.org:0/").serialize(), "https://www.serenityos.org:0/");
  EXPECT_EQ(URL("https://www.serenityos.org:80/").serialize(), "https://www.serenityos.org/");
  EXPECT_EQ(URL("https://www.serenityos.org:81/").serialize(), "https://www.serenityos.org:81/");
  EXPECT_EQ(URL("https://www.serenityos.org:443/foo/bar.html?query#fragment").serialize(), "https://www.serenityos.org/foo/bar.html?query#fragment");
@@ -117,7 +117,7 @@ TEST_CASE(file_url_with_hostname)
  EXPECT(url.is_valid());
  EXPECT_EQ(url.scheme(), "file");
  EXPECT_EQ(url.host(), "courage");
- EXPECT_EQ(url.port(), 0);
+ EXPECT_EQ(url.port_or_default(), 0);
  EXPECT_EQ(url.path(), "/my/file");
  EXPECT_EQ(url.serialize(), "file:https://courage/my/file");
  EXPECT(url.query().is_null());
@@ -200,7 +200,7 @@ TEST_CASE(mailto_url)
  EXPECT(url.is_valid());
  EXPECT_EQ(url.scheme(), "mailto");
  EXPECT(url.host().is_null());
- EXPECT_EQ(url.port(), 0);
+ EXPECT_EQ(url.port_or_default(), 0);
  EXPECT_EQ(url.paths().size(), 1u);
  EXPECT_EQ(url.paths()[0], "[email protected]");
  EXPECT(url.query().is_null());
@@ -291,7 +291,7 @@ TEST_CASE(trailing_slash_with_complete_url)
 TEST_CASE(trailing_port)
 {
  URL url("https://example.com:8086");
- EXPECT_EQ(url.port(), 8086);
+ EXPECT_EQ(url.port_or_default(), 8086);
 }
 
 TEST_CASE(port_overflow)
@@ -312,7 +312,7 @@ TEST_CASE(create_with_file_scheme)
  auto url = URL::create_with_file_scheme("/home/anon/README.md");
  EXPECT(url.is_valid());
  EXPECT_EQ(url.scheme(), "file");
- EXPECT_EQ(url.port(), 0);
+ EXPECT_EQ(url.port_or_default(), 0);
  EXPECT_EQ(url.paths().size(), 3u);
  EXPECT_EQ(url.paths()[0], "home");
  EXPECT_EQ(url.paths()[1], "anon");

diff --git a/Userland/Libraries/LibGemini/GeminiJob.cpp b/Userland/Libraries/LibGemini/GeminiJob.cpp
@@ -45,7 +45,7 @@ void GeminiJob::start()
  if (on_certificate_requested)
  on_certificate_requested(*this);
  };
- bool success = ((TLS::TLSv12&)*m_socket).connect(m_request.url().host(), m_request.url().port());
+ bool success = ((TLS::TLSv12&)*m_socket).connect(m_request.url().host(), m_request.url().port_or_default());
  if (!success) {
  deferred_invoke([this] {
  return did_fail(Core::NetworkJob::Error::ConnectionFailed);

diff --git a/Userland/Libraries/LibHTTP/HttpJob.cpp b/Userland/Libraries/LibHTTP/HttpJob.cpp
@@ -26,7 +26,7 @@ void HttpJob::start()
  did_fail(Core::NetworkJob::Error::ConnectionFailed);
  });
  };
- bool success = m_socket->connect(m_request.url().host(), m_request.url().port());
+ bool success = m_socket->connect(m_request.url().host(), m_request.url().port_or_default());
  if (!success) {
  deferred_invoke([this] {
  return did_fail(Core::NetworkJob::Error::ConnectionFailed);

diff --git a/Userland/Libraries/LibHTTP/HttpsJob.cpp b/Userland/Libraries/LibHTTP/HttpsJob.cpp
@@ -46,7 +46,7 @@ void HttpsJob::start()
  if (on_certificate_requested)
  on_certificate_requested(*this);
  };
- bool success = ((TLS::TLSv12&)*m_socket).connect(m_request.url().host(), m_request.url().port());
+ bool success = ((TLS::TLSv12&)*m_socket).connect(m_request.url().host(), m_request.url().port_or_default());
  if (!success) {
  deferred_invoke([this] {
  return did_fail(Core::NetworkJob::Error::ConnectionFailed);

diff --git a/Userland/Libraries/LibWeb/Bindings/LocationObject.cpp b/Userland/Libraries/LibWeb/Bindings/LocationObject.cpp
@@ -74,7 +74,7 @@ JS_DEFINE_NATIVE_FUNCTION(LocationObject::host_getter)
 {
  auto& window = static_cast<WindowObject&>(global_object);
  auto url = window.impl().associated_document().url();
- return JS::js_string(vm, String::formatted("{}:{}", url.host(), url.port()));
+ return JS::js_string(vm, String::formatted("{}:{}", url.host(), url.port_or_default()));
 }
 
 JS_DEFINE_NATIVE_FUNCTION(LocationObject::hash_getter)

diff --git a/Userland/Libraries/LibWeb/DOM/Document.cpp b/Userland/Libraries/LibWeb/DOM/Document.cpp
@@ -134,7 +134,7 @@ Origin Document::origin() const
 {
  if (!m_url.is_valid())
  return {};
- return { m_url.protocol(), m_url.host(), m_url.port() };
+ return { m_url.protocol(), m_url.host(), m_url.port_or_default() };
 }
 
 void Document::set_origin(const Origin& origin)

diff --git a/Userland/Libraries/LibWeb/Loader/FrameLoader.cpp b/Userland/Libraries/LibWeb/Loader/FrameLoader.cpp
@@ -165,7 +165,7 @@ bool FrameLoader::load(LoadRequest& request, Type type)
  AK::URL favicon_url;
  favicon_url.set_protocol(url.protocol());
  favicon_url.set_host(url.host());
- favicon_url.set_port(url.port());
+ favicon_url.set_port(url.port_or_default());
  favicon_url.set_paths({ "favicon.ico" });
 
  ResourceLoader::the().load(

diff --git a/Userland/Libraries/LibWeb/Loader/ResourceLoader.cpp b/Userland/Libraries/LibWeb/Loader/ResourceLoader.cpp
@@ -108,8 +108,8 @@ void ResourceLoader::load(LoadRequest& request, Function<void(ReadonlyBytes, con
  dbgln("ResourceLoader: Failed load of: \"{}\", \033[32;1mError: {}\033[0m, Duration: {}ms", url, error_message, load_time_ms);
  };
 
- if (is_port_blocked(url.port())) {
- log_failure(request, String::formatted("The port #{} is blocked", url.port()));
+ if (is_port_blocked(url.port_or_default())) {
+ log_failure(request, String::formatted("The port #{} is blocked", url.port_or_default()));
  return;
  }
 

diff --git a/Userland/Libraries/LibWeb/XHR/XMLHttpRequest.cpp b/Userland/Libraries/LibWeb/XHR/XMLHttpRequest.cpp
@@ -171,7 +171,7 @@ DOM::ExceptionOr<void> XMLHttpRequest::send()
  dbgln("XHR send from {} to {}", m_window->associated_document().url(), request_url);
 
  // TODO: Add support for preflight requests to support CORS requests
- Origin request_url_origin = Origin(request_url.protocol(), request_url.host(), request_url.port());
+ Origin request_url_origin = Origin(request_url.protocol(), request_url.host(), request_url.port_or_default());
 
  bool should_enforce_same_origin_policy = true;
  if (auto* page = m_window->page())

diff --git a/Userland/Libraries/LibWebSocket/Impl/TCPWebSocketConnectionImpl.cpp b/Userland/Libraries/LibWebSocket/Impl/TCPWebSocketConnectionImpl.cpp
@@ -34,7 +34,7 @@ void TCPWebSocketConnectionImpl::connect(ConnectionInfo const& connection)
  m_socket->on_connected = [this] {
  on_connected();
  };
- bool success = m_socket->connect(connection.url().host(), connection.url().port());
+ bool success = m_socket->connect(connection.url().host(), connection.url().port_or_default());
  if (!success) {
  deferred_invoke([this] {
  on_connection_error();

diff --git a/Userland/Libraries/LibWebSocket/Impl/TLSv12WebSocketConnectionImpl.cpp b/Userland/Libraries/LibWebSocket/Impl/TLSv12WebSocketConnectionImpl.cpp
@@ -42,7 +42,7 @@ void TLSv12WebSocketConnectionImpl::connect(ConnectionInfo const& connection)
  m_socket->on_tls_certificate_request = [](auto&) {
  // FIXME : Once we handle TLS certificate requests, handle it here as well.
  };
- bool success = m_socket->connect(connection.url().host(), connection.url().port());
+ bool success = m_socket->connect(connection.url().host(), connection.url().port_or_default());
  if (!success) {
  deferred_invoke([this] {
  on_connection_error();

diff --git a/Userland/Libraries/LibWebSocket/WebSocket.cpp b/Userland/Libraries/LibWebSocket/WebSocket.cpp
@@ -151,10 +151,10 @@ void WebSocket::send_client_handshake()
  // 4. Host
  auto url = m_connection.url();
  builder.appendff("Host: {}", url.host());
- if (!m_connection.is_secure() && url.port() != 80)
- builder.appendff(":{}", url.port());
- else if (m_connection.is_secure() && url.port() != 443)
- builder.appendff(":{}", url.port());
+ if (!m_connection.is_secure() && url.port_or_default() != 80)
+ builder.appendff(":{}", url.port_or_default());
+ else if (m_connection.is_secure() && url.port_or_default() != 443)
+ builder.appendff(":{}", url.port_or_default());
  builder.append("\r\n");
 
  // 5. and 6. Connection Upgrade