Implement and test CanonicalCookie::IsCanonical()

net/cookies/canonical_cookie.cc

Index: net/cookies/canonical_cookie.cc
diff --git a/net/cookies/canonical_cookie.cc b/net/cookies/canonical_cookie.cc
index 6c81a55dc7df1d768618d9d76b338a0b0fd79974..949cb9da187651bc1e9bba3ddd1cd9803655c889 100644
--- a/net/cookies/canonical_cookie.cc
+++ b/net/cookies/canonical_cookie.cc
@@ -50,10 +50,12 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "net/base/url_util.h"
 #include "net/cookies/cookie_util.h"
 #include "net/cookies/parsed_cookie.h"
 #include "url/gurl.h"
 #include "url/url_canon.h"
+#include "url/url_util.h"
 
 using base::Time;
 using base::TimeDelta;
@@ -399,6 +401,29 @@ bool CanonicalCookie::FullCompare(const CanonicalCookie& other) const {
   return Priority() < other.Priority();
 }
 
+bool CanonicalCookie::IsCanonical() const {

[mmenke, 2017/05/25 19:49:56]

Is there a single method I should be comparing this logic to?

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

I modeled it after SetCookieWithDetails minus the checks against URL.

+  if (name_.empty())

[mmenke, 2017/05/25 19:49:56]

Don't we support "Set-Cookie: =foo" and "Set-Cookie: foo", which both do the
same thing?

[mmenke, 2017/05/25 21:25:21]

Fun table that indicates we support both of these: 
http://inikulin.github.io/cookie-compat/

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Nice catch; my bad.  This snuck in a couple of CLs ago (see DCHECKs added in
CanonicalCookie::Create in https://codereview.chromium.org/2861063003) and
caused http://crbug.com/723734.  I've removed the check  above and add some
tests to make sure this remains true, at least until we explicitly try to change
the behavior.

+    return false;
+
+  if (ParsedCookie::ParseTokenString(name_) != name_ ||
+      ParsedCookie::ParseValueString(value_) != value_ ||
+      ParsedCookie::ParseValueString(domain_) != domain_ ||
+      ParsedCookie::ParseValueString(path_) != path_) {
+    return false;
+  }
+
+  url::CanonHostInfo ignored;
+  if (!url::HostIsIPAddress(domain_) &&
+      CanonicalizeHost(domain_, &ignored).empty()) {

[mmenke, 2017/05/25 19:49:56]

What if CanonicalizeHost(domain_) != domain_?

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Also note that the DCHECK(IsCanonical()) you suggested for CC::Create turned up
the fact that we were (in the case of strange schemes, specifically "foo:" in
the tests) creating cookies with empty domains.  I went back and read
SetCookieWithDetails() more carefully and realized that it actually *allowed*
empty domains despite the line "if (domain.empty()) return false;" part way
through :-J.  I've filed http://crbug.com/730633 to track the WTF aspects of
this, and modified the logic above to allow empty domains.

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Oh, tricky.  So as noted above I'm copying this logic from SetCookieWithDetails
and subtracting out the verification against the URL.  SetCookieWithDetails (in
GetCookieDomainWithSring()) has the above check, and then just adopts the domain
returned by CanonicalizeHost() (prefixed with a "." if is isn't already there
:-} :-J).  I think modifying the incoming domain is incompatible with the sense
of the CC constructor API.  So I think I agree with you that
CanonicalizeHost(domain_) != domain_ should be a failure from this function.  
But that's depending on a property of CanonicalizeHost() we haven't really
depended on before (loosely its idempotence).  I think I'll put that in, and
presume that if there's a problem it'll show up in testing, but if you think of
any gotchas in this space, please raise a flag.

Done.

+    return false;
+  }
+
+  if (path_[0] != '/')
+    return false;

[mmenke, 2017/05/25 19:49:56]

Do we always set path to be non-empty, even when none is provided?

Regardless, this should at least be (path_.empty() || path_[0] != '/')

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Yes, at least at a couple of places in the stack--see
CanonicalCookie::CanonPathWithString.  
Right!  Done.

+
+  return true;
+}
+
 // static
 CanonicalCookie::CookiePrefix CanonicalCookie::GetCookiePrefix(
     const std::string& name) {