Make CanonicalCookie::Source() private.

chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java

Index: chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java b/chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
index c8d67b5dc21bdc64b43c738a4ef8e25b6da2e3e9..0f430da0c3218af0d3c191cbb9cf8ddcfdebef43 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
@@ -16,7 +16,6 @@ import java.util.List;
  * Also has static methods serialize Cookies.
  */
 class CanonicalCookie {
-    private final String mUrl;
     private final String mName;
     private final String mValue;
     private final String mDomain;
@@ -30,10 +29,9 @@ class CanonicalCookie {
     private final int mPriority;
 
     /** Constructs a CanonicalCookie */
-    CanonicalCookie(String url, String name, String value, String domain, String path,
-            long creation, long expiration, long lastAccess, boolean secure, boolean httpOnly,
-            int sameSite, int priority) {
-        mUrl = url;
+    CanonicalCookie(String name, String value, String domain, String path, long creation,
+            long expiration, long lastAccess, boolean secure, boolean httpOnly, int sameSite,
+            int priority) {
         mName = name;
         mValue = value;
         mDomain = domain;
@@ -92,11 +90,6 @@ class CanonicalCookie {
         return mPath;
     }
 
-    /** @return Cookie URL. */
-    String getUrl() {
-        return mUrl;
-    }
-
     /** @return Cookie domain. */
     String getDomain() {
         return mDomain;
@@ -164,7 +157,8 @@ class CanonicalCookie {
     }
 
     private void saveToStream(DataOutputStream out) throws IOException {
-        out.writeUTF(mUrl);
+        // URL is no longer included. Keep for backward compatability.
+        out.writeUTF("");
         out.writeUTF(mName);
         out.writeUTF(mValue);
         out.writeUTF(mDomain);
@@ -179,8 +173,11 @@ class CanonicalCookie {
     }
 
     private static CanonicalCookie createFromStream(DataInputStream in) throws IOException {
+        // Used to save the URL. Unclear what this method does, or if backward compatibility
+        // matters.

[gone, 2016/07/13 18:15:38]

This comment doesn't inspire any confidence :P  This function looks to be only
called from readListFromStream, which is called from
CookiesFetcher.restoreCookiesInternal(), which looks like it restores a set of
encrypted Incognito cookies from disk.

Android throws away our Incognito cipher keys frequently, including (I believe)
after Chrome upgrades.  I wouldn't be surprised if we didn't need to worry about
backward compatibility because we wouldn't be able to restore the cookies
anyway.

[mmenke, 2016/07/14 17:34:57]

Then it's serving its intended purpose - this code isn't well documented, and it
took me a while to figure out what it's for.  Seems like things could improve
there.

"Not being surprised if it's not needed" doesn't exactly give me the urge to
take responsibility for removing it, and keep an eye out for any resulting
problems.  :)

The possibility that we'd end up writing weird stuff to the cookie store if it's
not deleted during the upgrade seems more than a bit concerning.

[gone, 2016/07/14 21:25:10]

The new comment doesn't document anything though -- it just says that you don't
know what it does.
How so?  If the cipher key is invalid on upgrade (or when the user explicitly
closes Chrome), then the file can't be read and the cookie store file is
useless.  Same principle applies to the tab state files, which are discarded if
the key is invalid.  

Android doesn't tell us when the user explicitly closes us by swiping away the
task, nor does it give Chrome any opportunity to cleanly shut down before it
kills the process.  While there is a hook for upgrades that we've started
listening to, addressing that corner case while the more common scenarios can't
be addressed is less helpful.

[mmenke, 2016/07/14 21:38:28]

Which is true.  It does however, document the lack of documentation.  Since I'll
probably never be touching this code again, I'm reluctant to invest time in
understanding it well enough to document it.  If you prefer, I can remove the
comment.
Cypher key?  I don't work on Android, and have no clue how things work there. 
I'm concerned we would end up reading the file from an old version, and that all
the fields we'd read here would be one off for the first cookie, off by two for
the second, etc, and we'd try setting cookies with the resulting corrupt data. 
You seem unsure of whether this would happen or not.
Addressing the corner cases?  Once we have corrupted our cookie store, it's
potentially corrupted eternally here, since we keep on saving it and re-loading
it.  At some point this code will presumably throw it away.  I don't know when. 
Since I'll never be touching this code again, I'm reluctant to invest any time
in learning it better.  I'm certainly open to improving how this CL fixes the
problem, just reluctant to invest time in figuring out how to improve how this
CL fixes the problem, since it touches on a number of topics I know nothing
about (Which includes learning what happens to whatever type of data this is
stored as on Android when upgrading between Chrome versions).

[gone, 2016/07/14 21:55:58]

Yeah, just remove it.  It's only called from one function, which is only called
from one Java class (which has a class comment describing what it's for) and a
Java test.  Just leave the readUTF() line in, too; it shouldn't have any effect
either way.

I started writing an explanation of exactly what I was unsure about, what the
cipher key was, and why corrupt files shouldn't be an issue because of how
exceptions are caught in CookiesFetcher#restoreCookiesInternal() to deal with
how Android works, but based on your comment you don't seem at all interested in
the topic.

[mmenke, 2016/07/14 22:18:19]

Done (Well, I duplicated the backward compatibility comment from above).
Unfortunately, I just don't have time to be interested, lately.

Relying on readLong() / readInt() / readBoolean() to throw an exception in the
case of misalignment (Which I think is what you're suggesting?) just seems
inherently flaky to me.

+        in.readUTF();
         return new CanonicalCookie(in.readUTF(), in.readUTF(), in.readUTF(), in.readUTF(),
-                in.readUTF(), in.readLong(), in.readLong(), in.readLong(), in.readBoolean(),
-                in.readBoolean(), in.readInt(), in.readInt());
+                in.readLong(), in.readLong(), in.readLong(), in.readBoolean(), in.readBoolean(),
+                in.readInt(), in.readInt());
     }
 }