Make Android Clipboard Keep Track of Last Modified Time

ui/android/java/src/org/chromium/ui/base/Clipboard.java

Index: ui/android/java/src/org/chromium/ui/base/Clipboard.java
diff --git a/ui/android/java/src/org/chromium/ui/base/Clipboard.java b/ui/android/java/src/org/chromium/ui/base/Clipboard.java
index 4497c60fa49b925005f3322337b1146f626c9770..ecf60e687a7f00fe704355c141b73e470eb0e970 100644
--- a/ui/android/java/src/org/chromium/ui/base/Clipboard.java
+++ b/ui/android/java/src/org/chromium/ui/base/Clipboard.java
@@ -8,24 +8,41 @@ import android.content.ClipData;
 import android.content.ClipboardManager;
 import android.content.Context;
 
+import org.chromium.base.Log;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.SuppressFBWarnings;
+import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.ui.R;
 import org.chromium.ui.widget.Toast;
 
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Arrays;
+
 /**
  * Simple proxy that provides C++ code with an access pathway to the Android
  * clipboard.
  */
 @JNINamespace("ui")
-public class Clipboard {
+public class Clipboard implements ClipboardManager.OnPrimaryClipChangedListener {

[Ted C, 2017/03/24 03:26:51]

Hmm...a couple things I realized.

This is not a singleton, and I have no idea when we are initializing this
initially so our change times are not going to accurately reflect what we want.

So, we seem to have stumbled onto something bigger than a bread box.  I think we
really should make this a singleton so we can register early on startup and make
sure the time reporting is only handled in a single place.

Without making this a singleton, we're going to have to introduce a way to
delete/destroy these instances to unregister, which will be annoying I think.

[Mark P, 2017/03/24 05:20:25]

Have several questions here.

1. Why does it matter that it's not a singleton?
  - Are you concerned that one will be created, say, on startup, and then
another later, and if an event happens in between then the first will have the
right time for it and the second will not, and it depends which one the C++ code
interrogates for what it gets?  (After all, once they're all running, all will
get messages at the same time, and then the times will all align so it doesn't
matter which you talk to.)
  - This problem with different instantiation times becomes moot if we store the
clipboard information in prefs, as I plan to do in a later changelist.
  - Or perhaps you're concerned about not being able to delete a Clipboard
instance properly?  (Instead, it will remain around, not garbage collected,
because it's registered as a listener and never unregistered.  Am I
understanding Android right?)
  - I see the appeal of singletons.  Yet this class has gotten along with them
thus far.

2. It looks to me like this class is only instantiated in two places in the
chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
so perhaps we can simple make these two instances behave correctly and then not
worry about it?

3. On the other hand, ui/base/clipboard/clipboard_android.cc must talk to a
Android Clipboard.java somehow.  I wonder there that is instantiated and why it
didn't come up in my searches.  I assume that target that clipboard_android.cc
talks with always remains around, so maybe I just need to make Clipboard.java
work in the way I want in only that setting.

[Ted C, 2017/03/24 16:13:50]

I'm worried one isn't created on startup and only when some subsystem starts
to consider needing the clipboard.  If any changes to the clipdata happen
between that, then we'll miss them.
That is correct.  Once we add listeners to the clipboardmanager, we are
preventing
GC of our Clipboard objects and anything they hold on to (thankfully very
little).
We've gotten by because we were simply a pull model.  Now that we need to
register
listeners, I don't think it holds.  Honestly, I don't think it made sense to
ever be anything but a singleton.
I think we could do that as well, but it highlights to me that we are
registering
listeners for clients that simply never need that data.  I think if we don't go
the singleton route, we should no register the listener on creation but have an
api that allows you to register a listener to the clipboard yourself (here the
omnibox code would get a clipboard and register a listener and be in charge of
unregistering if ever).

I'm thinking this is much more work and churn than migrating to a singleton.

FWIW, I threw together a patch in a few minutes that makes it a singleton that
you can take a look at to see if you want to incorporate it:
https://codereview.chromium.org/2772073002

[Mark P, 2017/03/24 21:53:25]

Thanks for the detailed responses to my comments.

I have a slight preference to landing the singleton changelist separately, just
in case there is fallout, and it's a clearly separate concept.

That said, I'm happy incorporating the singleton changelist into min if you
don't want to put in the time of getting it reviewed and whatnot.  (I'm willing
to include it in mine because I understand it.  Though I'm glad you wrote: it
would've taken me much longer, and maybe with some more questions.)

+    private static final String TAG = "Clipboard";
+
     // Necessary for coercing clipboard contents to text if they require
     // access to network resources, etceteras (e.g., URI in clipboard)
     private final Context mContext;
 
     private final ClipboardManager mClipboardManager;
 
+    // A message hasher that's used to hash clipboard contents so we can tell
+    // when a clipboard changes without storing the full contents.
+    private MessageDigest mMd5Hasher;
+    // The hash of the current clipboard.
+    private byte[] mClipboardMd5;
+    // The time when the clipboard was last updated.  Set to 0 if unknown.
+    private long mClipboardChangeTime;
+
     /**
      * Use the factory constructor instead.
      *
@@ -35,6 +52,20 @@ public class Clipboard {
         mContext = context;
         mClipboardManager = (ClipboardManager)
                 context.getSystemService(Context.CLIPBOARD_SERVICE);
+        mClipboardManager.addPrimaryClipChangedListener(this);
+        try {
+            mMd5Hasher = MessageDigest.getInstance("MD5");
+            mClipboardMd5 = weakMd5Hash();
+        } catch (NoSuchAlgorithmException e) {
+            Log.e(TAG,
+                    "Unable to construct MD5 MessageDigest: %s; assume "
+                            + "clipboard last update time is start of epoch.",
+                    e);
+            mMd5Hasher = null;
+            mClipboardMd5 = new byte[] {};
+        }
+        RecordHistogram.recordBooleanHistogram("Clipboard.ConstructedHasher", mMd5Hasher != null);
+        mClipboardChangeTime = 0;
     }
 
     /**
@@ -97,6 +128,21 @@ public class Clipboard {
     }
 
     /**
+     * Gets the time the clipboard content last changed.
+     *
+     * This is calculated according to the device's clock.  E.g., it continues
+     * increasing when the device is suspended.  Likewise, it can be in the
+     * future if the user's clock updated after this information was recorded.
+     *
+     * @return a Java long recording the last changed time in milliseconds since
+     * epoch, or 0 if the time could not be determined.
+     */
+    @CalledByNative
+    public long getClipboardContentChangeTimeInMillis() {
+        return mClipboardChangeTime;
+    }
+
+    /**
      * Emulates the behavior of the now-deprecated
      * {@link android.text.ClipboardManager#setText(CharSequence)}, setting the
      * clipboard's current primary clip to a plain-text clip that consists of
@@ -141,4 +187,37 @@ public class Clipboard {
             Toast.makeText(mContext, text, Toast.LENGTH_SHORT).show();
         }
     }
+
+    /**
+     * Updates mClipboardMd5 and mClipboardChangeTime when the clipboard updates.
+     *
+     * Implements OnPrimaryClipChangedListener to listen for clipboard updates.
+     */
+    @Override
+    public void onPrimaryClipChanged() {
+        if (mMd5Hasher == null) return;
+        RecordUserAction.record("MobileOmniboxClipboardChanged");

[Ted C, 2017/03/24 03:26:51]

The "Omnibox" portion here seems misleading to me.  We don't know where the
clipboard change came from (nor if it came from this app), so
MobileClipboardChanged seems more correct to me.

[Mark P, 2017/03/24 05:20:25]

I agree.  But this is the user action that iOS uses.

I'm happy to migrate them both over to the new name (I think it's better too),
but I'd like to do metrics renaming in a separate changelist.

Added to crbug.com/704715

+        mClipboardMd5 = weakMd5Hash();
+        // Always update the clipboard change time even if the clipboard

[Ted C, 2017/03/24 03:26:51]

D'oh.  I was thinking we were using the hash to determine change and prevent
updating.  Obviously didn't read that so my test comment was invalid.

[Mark P, 2017/03/24 05:20:25]

Acknowledged.

+        // content hasn't changed.  This is because if the user put something
+        // in the clipboard recently (even if it was not necessary because it
+        // was already there), that content should be considered recent.
+        mClipboardChangeTime = System.currentTimeMillis();
+    }
+
+    /**
+     * Returns a weak hash of getCoercedText().
+     *
+     * @return a Java byte[] with the weak hash.
+     */
+    private byte[] weakMd5Hash() {
+        if (getCoercedText() == null) {
+            return new byte[] {};
+        }
+        // Compute a hash consisting of the first 4 bytes of the MD5 hash of
+        // getCoercedText().  This value is used to detect clipboard content
+        // change. Keeping only 4 bytes is a privacy requirement to introduce
+        // collision and allow deniability of having copied a given string.
+        return Arrays.copyOfRange(mMd5Hasher.digest(getCoercedText().getBytes()), 0, 4);
+    }
 }