Cleanup TODOs, implement trivial ones

Inspired by this week's "Healthy Code on the Commode", this patch
reviews some of the TODOs in the Open Screen repository, making sure the
following things are true:

1. All TODOs should have a username or issue number.
2. TODOs should be actionable, explaining how and when they can be
resolved.
3. TODOs should define a clear and addressable problem.
4. Stale TODOs should be removed from the code.

Change-Id: I6860a4d70983c0742cc9dd37ce9ee06c0d9348e0
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/1715254
Reviewed-by: Max Yakimakha <yakimakha@chromium.org>
Commit-Queue: Jordan Bayles <jophba@chromium.org>

10 files changed, 44 insertions, 31 deletions

diff --git a/osp/docs/pi.md b/osp/docs/pi.md
index 76b35ea2..d19af05f 100644
--- a/osp/docs/pi.md
+++ b/osp/docs/pi.md
@@ -25,11 +25,11 @@ Either of the flash instructions below will make a bootable microSD card.
 
 ### Flashing via commandline
 
-TODO: Write a script
+TODO(mfoltz): Write a script
 
 ### Logging in and configuring WiFi
 
-TODO: Figure this out
+TODO(mfoltz): Figure this out
 
 ## Compiling for Raspberry Pi
 
diff --git a/osp/go/cmd/osp.go b/osp/go/cmd/osp.go
index 86e87b07..ad8d7f0d 100644
--- a/osp/go/cmd/osp.go
+++ b/osp/go/cmd/osp.go
@@ -4,8 +4,7 @@
 
 package main
 
-// TODO: 
-//  Add response messages from receiver
+// TODO(pthatcher): Add response messages from receiver
 
 //  Inject JS into viewURL to using .Eval and .Bind to send and receiver presentation connection messages
 
@@ -44,7 +43,7 @@ func flingUrl(ctx context.Context, target string, url string) {
 	entries, err := osp.BrowseMdns(ctx)
 	if (err != nil) {
 		log.Fatalf("Failed to browse mDNS: %v\n", err)
-	} 
+	}
 	for entry := range entries {
 		if entry.Instance == target {
 			log.Printf("Fling %s to %s:%d\n", url, entry.HostName, entry.Port)
@@ -82,7 +81,7 @@ func main() {
 	if len(args) < 1 {
 		log.Fatalln("Usage: osp server name; osp browse; osp fling url; osp view url")
 	}
-	
+
 	ctx := context.Background()
 
 	cmd := args[0]
diff --git a/osp/go/cmd/test.go b/osp/go/cmd/test.go
index 5eac0aa8..ad6142b8 100644
--- a/osp/go/cmd/test.go
+++ b/osp/go/cmd/test.go
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// TODO: Use proper testing framework
+// TODO(pthatcher): Use proper testing framework
 
 package main
 
@@ -17,7 +17,7 @@ import (
 )
 
 func testMdns() {
-	// TODO: log error if it fails
+	// TODO(pthatcher): log error if it fails
 	ctx := context.Background()
 	instance := "TV"
 	port := 10000
diff --git a/osp/go/quic.go b/osp/go/quic.go
index 39cbe958..8fdff9aa 100644
--- a/osp/go/quic.go
+++ b/osp/go/quic.go
@@ -56,7 +56,7 @@ func readAllStreams(ctx context.Context, session quic.Session, streams chan<- io
 
 // Returns a quic.Session object with a .OpenStreamSync method to send streams
 func DialAsQuicClient(ctx context.Context, hostname string, port int) (quic.Session, error) {
-	// TODO: Change InsecureSkipVerify
+	// TODO(pthatcher): Change InsecureSkipVerify
 	tlsConfig := &tls.Config{InsecureSkipVerify: true}
 	addr := fmt.Sprintf("%s:%d", hostname, port)
 	session, err := quic.DialAddrContext(ctx, addr, tlsConfig, nil)
diff --git a/osp/go/server.go b/osp/go/server.go
index a2ee06c1..e7770576 100644
--- a/osp/go/server.go
+++ b/osp/go/server.go
@@ -14,7 +14,7 @@ import (
 )
 
 func ReadMessagesAsServer(ctx context.Context, instanceName string, port int, cert tls.Certificate, messages chan<- interface{}) error {
-	// TODO: log error if it fails
+	// TODO(pthatcher): log error if it fails
 	go RunMdnsServer(ctx, instanceName, port)
 	streams := make(chan io.ReadWriteCloser)
 	go RunQuicServer(ctx, port, cert, streams)
diff --git a/osp/impl/discovery/mdns/domain_name.cc b/osp/impl/discovery/mdns/domain_name.cc
index c901cd5d..7e584d60 100644
--- a/osp/impl/discovery/mdns/domain_name.cc
+++ b/osp/impl/discovery/mdns/domain_name.cc
@@ -42,7 +42,7 @@ ErrorOr<DomainName> DomainName::Append(const DomainName& first,
   return result;
 }
 
-DomainName::DomainName() : domain_name_{'\0'} {}
+DomainName::DomainName() : domain_name_{0u} {}
 DomainName::DomainName(std::vector<uint8_t>&& domain_name)
     : domain_name_(std::move(domain_name)) {
   OSP_CHECK_LE(domain_name_.size(), kDomainNameMaxLength);
@@ -54,8 +54,15 @@ DomainName& DomainName::operator=(const DomainName& domain_name) = default;
 DomainName& DomainName::operator=(DomainName&& domain_name) = default;
 
 bool DomainName::operator==(const DomainName& other) const {
-  // TODO: case-insensitive comparison
-  return domain_name_ == other.domain_name_;
+  if (domain_name_.size() != other.domain_name_.size()) {
+    return false;
+  }
+  for (int i = 0; i < domain_name_.size(); ++i) {
+    if (tolower(domain_name_[i]) != tolower(other.domain_name_[i])) {
+      return false;
+    }
+  }
+  return true;
 }
 
 bool DomainName::operator!=(const DomainName& other) const {
@@ -74,7 +81,7 @@ bool DomainName::EndsWithLocalDomain() const {
 
 Error DomainName::Append(const DomainName& after) {
   OSP_CHECK(after.domain_name_.size());
-  OSP_DCHECK_EQ(after.domain_name_.back(), '\0');
+  OSP_DCHECK_EQ(after.domain_name_.back(), 0u);
 
   if ((domain_name_.size() + after.domain_name_.size() - 1) >
       kDomainNameMaxLength) {
@@ -86,15 +93,19 @@ Error DomainName::Append(const DomainName& after) {
   return Error::None();
 }
 
-std::vector<std::string> DomainName::GetLabels() const {
+std::vector<absl::string_view> DomainName::GetLabels() const {
   OSP_DCHECK_GT(domain_name_.size(), 0u);
-  std::vector<std::string> result;
-  auto it = domain_name_.begin();
-  while (*it != 0) {
-    OSP_DCHECK_LT(it - domain_name_.begin(), kDomainNameMaxLength);
-    OSP_DCHECK_LT((it + 1 + *it) - domain_name_.begin(), kDomainNameMaxLength);
-    result.emplace_back(it + 1, it + 1 + *it);
-    it += 1 + *it;
+  OSP_DCHECK_LT(domain_name_.size(), kDomainNameMaxLength);
+
+  std::vector<absl::string_view> result;
+  const uint8_t* data = domain_name_.data();
+  while (*data != 0) {
+    const size_t label_length = *data;
+    OSP_DCHECK_LT(label_length, kDomainNameMaxLabelLength);
+
+    ++data;
+    result.emplace_back(reinterpret_cast<const char*>(data), label_length);
+    data += label_length;
   }
   return result;
 }
diff --git a/osp/impl/discovery/mdns/domain_name.h b/osp/impl/discovery/mdns/domain_name.h
index 7f1e0931..20efe0c8 100644
--- a/osp/impl/discovery/mdns/domain_name.h
+++ b/osp/impl/discovery/mdns/domain_name.h
@@ -10,6 +10,7 @@
 #include <string>
 #include <vector>
 
+#include "absl/strings/string_view.h"
 #include "platform/api/logging.h"
 #include "platform/base/error.h"
 
@@ -63,9 +64,7 @@ struct DomainName {
   bool IsEmpty() const { return domain_name_.size() == 1 && !domain_name_[0]; }
 
   Error Append(const DomainName& after);
-  // TODO: If there's significant use of this, we would rather have string_span
-  // or similar for this so we could use iterators for zero-copy.
-  std::vector<std::string> GetLabels() const;
+  std::vector<absl::string_view> GetLabels() const;
 
   const std::vector<uint8_t>& domain_name() const { return domain_name_; }
 
diff --git a/osp/impl/discovery/mdns/domain_name_unittest.cc b/osp/impl/discovery/mdns/domain_name_unittest.cc
index ebfb7cc5..48ea272b 100644
--- a/osp/impl/discovery/mdns/domain_name_unittest.cc
+++ b/osp/impl/discovery/mdns/domain_name_unittest.cc
@@ -159,8 +159,12 @@ TEST(DomainNameTest, Append) {
 
 TEST(DomainNameTest, GetLabels) {
   const auto labels = std::vector<std::string>{"alpha", "beta", "gamma", "org"};
-  DomainName d = UnpackErrorOr(FromLabels(labels));
-  EXPECT_EQ(d.GetLabels(), labels);
+  DomainName domain_name = UnpackErrorOr(FromLabels(labels));
+
+  const auto actual_labels = domain_name.GetLabels();
+  for (auto i = 0; i < labels.size(); ++i) {
+    EXPECT_EQ(labels[i], actual_labels[i]);
+  }
 }
 
 TEST(DomainNameTest, StreamEscaping) {
diff --git a/osp/impl/discovery/mdns/embedder_demo.cc b/osp/impl/discovery/mdns/embedder_demo.cc
index 2603dd59..d52098df 100644
--- a/osp/impl/discovery/mdns/embedder_demo.cc
+++ b/osp/impl/discovery/mdns/embedder_demo.cc
@@ -210,9 +210,9 @@ void HandleEvents(mdns::MdnsResponderAdapterImpl* mdns_adapter) {
     }
   }
   for (const auto& a_event : mdns_adapter->TakeAResponses()) {
-    // TODO: If multiple SRV records specify the same domain, the A will only
-    // update the first.  I didn't think this would happen but I noticed this
-    // happens for cast groups.
+    // TODO(btolsch): If multiple SRV records specify the same domain, the A
+    // will only update the first.  I didn't think this would happen but I
+    // noticed this happens for cast groups.
     auto it =
         std::find_if(g_services->begin(), g_services->end(),
                      [&a_event](const std::pair<mdns::DomainName, Service>& s) {
diff --git a/osp/impl/mdns_responder_service.cc b/osp/impl/mdns_responder_service.cc
index 15836fb4..3599c508 100644
--- a/osp/impl/mdns_responder_service.cc
+++ b/osp/impl/mdns_responder_service.cc
@@ -216,7 +216,7 @@ void MdnsResponderService::HandleMdnsEvents() {
     }
 
     // TODO(btolsch): Verify UTF-8 here.
-    std::string friendly_name = instance_name.GetLabels()[0];
+    std::string friendly_name(instance_name.GetLabels()[0]);
 
     if (receiver_info_entry == receiver_info_.end()) {
       ServiceInfo receiver_info{