Make Android Clipboard Keep Track of Last Modified Time

Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

To do in a later changelist: add lightweight code to store the hash and update time in prefs so that Chrome can have a better chance of using the current clipboard content on startup.  (If the clipboard content hasn't changed since the last time Chrome ran, the last modified date is likely correct.)

BUG=682446
Review-Url: https://codereview.chromium.org/2766623003
Cr-Commit-Position: refs/heads/master@{#461234}
Committed: https://chromium.googlesource.com/chromium/src/+/6f6d07f111d9df9bc70ff3dc71612d940d9b2d17

[Mark P, 2017-03-21 22:50:35]

Description was changed from

==========
Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

BUG=682446
==========

to

==========
Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

BUG=682446
==========

[Mark P, 2017-03-21 22:50:35]

mpearson@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Mark P, 2017-03-21 22:53:47]

Hey Ted,

Can you please take a look at this (or delegate someone to do so)?  No hurry.

Apologies if I inadvertantly violate Java style conventions.  This is the first
time I'm writing >5 line Java changelists. :-)

I tested this using logging lines on both the Java and C++ side.  It seems to
work.  The current junit test I have does not pass [1].  Your help would be
appreciated to configuring the build / test to get it to pass.

thanks,
mark

[1] The junit test I wrote fails at the beginning with the message:
[ RUN      ]
org.chromium.ui.base.ClipboardTest.testGetClipboardContentChangeTimeInMillis
java.lang.NoClassDefFoundError: android/test/mock/MockContext
        at
org.chromium.ui.base.ClipboardTest.testGetClipboardContentChangeTimeInMillis(ClipboardTest.java:26)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
...

[Mark P, 2017-03-21 22:55:28]

Description was changed from

==========
Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

BUG=682446
==========

to

==========
Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

To do in a later changelist: add lightweight code to store the hash and update
time in prefs so that Chrome can have a better chance of using the current
clipboard content on startup.  (If the clipboard content hasn't changed since
the last time Chrome ran, the last modified date is likely correct.)

BUG=682446
==========

[Ted C, 2017-03-23 20:59:31]

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:133: * This is
calculated according to the device's clock.  E.g., it continues
Although this file isn't consistent, we wrap at 100 chars for javadocs normally

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:198: if (mMd5Hasher ==
null) {
in javaland, you can put the statement and conditional all on a single line if
it fits and reads ok (although I don't recall if git cl format complains these
days...which I should file a bug for).

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:207:
mClipboardChangeTime = System.currentTimeMillis();
Are we persisting this across cold starts?  If not, I would recommend
https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil...

or

https://developer.android.com/reference/android/os/SystemClock.html#elapsedRe...

which are monotonically increasing and avoids time travel backwards.  But that
only works if you use this same value for comparison but it looks like we are
passing the time to native and it is likely comparing against a similar behavior
as System.currentTimeMillis (where we could use these clocks, but we'd need to
be reporting clipboard age instead of time)

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:223: return
Arrays.copyOfRange(mMd5Hasher.digest(getCoercedText().getBytes()), 0, 4);
We want to use MD5 here because it gives a consistent/unpredictability compared
to something like Object#hashCode from java?

https://codereview.chromium.org/2766623003/diff/20001/ui/android/junit/src/or...
File ui/android/junit/src/org/chromium/ui/base/ClipboardTest.java (right):

https://codereview.chromium.org/2766623003/diff/20001/ui/android/junit/src/or...
ui/android/junit/src/org/chromium/ui/base/ClipboardTest.java:26: Clipboard
clipboard = new Clipboard(new AdvancedMockContext());
The AndvancedMockContext is more designed for instrumentation tests.  Here, much
of this is done for you by Robolectric.  You might be able to use
org.robolectric.RuntimeEnvironment.application here as the context.

I think a dummy implementation is already provided for you here:
https://cs.chromium.org/chromium/src/third_party/robolectric/robolectric/robo...

There is the concept of Shadows in Robolectric as well, which is likely how you
would need to mock it out.  But you simply might not need to, you could likely
just use the above context to getSystemService and then push clip data yourself
if you needed to test (things like the comment I wrote below, which I wrote
first :-P)

https://codereview.chromium.org/2766623003/diff/20001/ui/android/junit/src/or...
ui/android/junit/src/org/chromium/ui/base/ClipboardTest.java:31: }
Maybe we could add a test where we mock the ClipboardManager in your mock
context (we could overwrite getSystemService in a local AdvancedMockContext and
return our own manager that can provide getPrimaryClip to be mocked.  Then we
could add a test where we notify onPrimaryClipChanged without the text changing
and ensure the change time doesn't increase.

And now that i've written all of that, we could likely just duplicate the test
above and call onPrimaryClipChanged again.  It's not like the text will change
even if empty so it should work as is.

[Mark P, 2017-03-23 23:40:32]

Replies to your comments.

Also, I changed the C++ side slightly since my earlier patchset.  (Not sure if
you look at that side at all yet.)  Now, with the current Java and C++ side and
another pending C++ patch based off of this one, I have things working
end-to-end.  Yay, the implementation plan is functional! :-)

I haven't yet made the test work.  It'll take me some additional head-scratching
over Android docs and your pointer Robolectric and Shadow contexts to figure it
out.

cheers,
mark

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:133: * This is
calculated according to the device's clock.  E.g., it continues
On 2017/03/23 20:59:31, Ted C wrote:
> Although this file isn't consistent, we wrap at 100 chars for javadocs
normally

Yeah, I actually had this file originally at 100 chars, then noticed the
majority of the comments here wrapped at 80 and decided to go with local
consistency.

Let me know if you want me to reformat everything...?

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:198: if (mMd5Hasher ==
null) {
On 2017/03/23 20:59:31, Ted C wrote:
> in javaland, you can put the statement and conditional all on a single line if
> it fits and reads ok (although I don't recall if git cl format complains these
> days...which I should file a bug for).

Done.

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:207:
mClipboardChangeTime = System.currentTimeMillis();
On 2017/03/23 20:59:31, Ted C wrote:
> Are we persisting this across cold starts?

Yes, that's the intention.  It's not there at the moment.  I'll add it in
another changelist, per this changelist description.

> If not, I would recommend
>
https://developer.android.com/reference/android/os/SystemClock.html#uptimeMil...
> 
> or
> 
>
https://developer.android.com/reference/android/os/SystemClock.html#elapsedRe...
> 
> which are monotonically increasing and avoids time travel backwards.  But that
> only works if you use this same value for comparison but it looks like we are
> passing the time to native and it is likely comparing against a similar
behavior
> as System.currentTimeMillis (where we could use these clocks, but we'd need to
> be reporting clipboard age instead of time)

https://codereview.chromium.org/2766623003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:223: return
Arrays.copyOfRange(mMd5Hasher.digest(getCoercedText().getBytes()), 0, 4);
On 2017/03/23 20:59:31, Ted C wrote:
> We want to use MD5 here because it gives a consistent/unpredictability
compared
> to something like Object#hashCode from java?

I didn't think through this at all.  This comment and reasoning comes from the
iOS implementation of this same feature.  I suppose I can open the question with
legal about whether they think the Java's hash function is good enough.  If you
push me, I will, though obviously I'd rather not jump through that hoop.

https://codereview.chromium.org/2766623003/diff/20001/ui/android/junit/src/or...
File ui/android/junit/src/org/chromium/ui/base/ClipboardTest.java (right):

https://codereview.chromium.org/2766623003/diff/20001/ui/android/junit/src/or...
ui/android/junit/src/org/chromium/ui/base/ClipboardTest.java:31: }
On 2017/03/23 20:59:31, Ted C wrote:
> Maybe we could add a test where we mock the ClipboardManager in your mock
> context (we could overwrite getSystemService in a local AdvancedMockContext
and
> return our own manager that can provide getPrimaryClip to be mocked.  Then we
> could add a test where we notify onPrimaryClipChanged without the text
changing
> and ensure the change time doesn't increase.
> 
> And now that i've written all of that, we could likely just duplicate the test
> above and call onPrimaryClipChanged again.  It's not like the text will change
> even if empty so it should work as is.

Why would we want to test that the change time doesn't increase with
onPrimaryClipChanged changed without changing the text?  The code explicitly
updates the time in case, and already has a comment to this effect.

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

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:29: public class
Clipboard implements ClipboardManager.OnPrimaryClipChangedListener {
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.

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:199:
RecordUserAction.record("MobileOmniboxClipboardChanged");
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.

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:201: // Always update
the clipboard change time even if the clipboard
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]

Some mixed-up thoughts.  Thanks for the fast feedback!

--mark

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:29: public class
Clipboard implements ClipboardManager.OnPrimaryClipChangedListener {
On 2017/03/24 03:26:51, Ted C wrote:
> 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.

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.

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:199:
RecordUserAction.record("MobileOmniboxClipboardChanged");
On 2017/03/24 03:26:51, Ted C wrote:
> 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.

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

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:201: // Always update
the clipboard change time even if the clipboard
On 2017/03/24 03:26:51, Ted C wrote:
> 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.

Acknowledged.

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

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:29: public class
Clipboard implements ClipboardManager.OnPrimaryClipChangedListener {
On 2017/03/24 05:20:25, Mark P wrote:
> On 2017/03/24 03:26:51, Ted C wrote:
> > 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.
> 
> 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.)

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.

>   - 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?)

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).

>   - I see the appeal of singletons.  Yet this class has gotten along with them
> thus far.

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.

> 
> 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?

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

> 
> 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.

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

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:29: public class
Clipboard implements ClipboardManager.OnPrimaryClipChangedListener {
On 2017/03/24 16:13:50, Ted C wrote:
> On 2017/03/24 05:20:25, Mark P wrote:
> > On 2017/03/24 03:26:51, Ted C wrote:
> > > 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.
> > 
> > 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.)
> 
> 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.
> 
> >   - 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?)
> 
> 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).
> 
> >   - I see the appeal of singletons.  Yet this class has gotten along with
them
> > thus far.
> 
> 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.
> 
> > 
> > 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?
> 
> 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

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.)



> > 
> > 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.

[Mark P, 2017-03-29 20:08:44]

Ted,

Aside from still needing to make the test run, I think this is ready for a real
review.  Can you please get this started, and then we'll add dcheng@ when you
think it's good enough for him?

thanks,
mark

[Mark P, 2017-03-30 05:05:37]

Yay, the test works!

--mark

[Ted C, 2017-03-30 16:55:21]

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
How often are we going to be asking for the clipboard age?  Every omnibox
keystroke?

I'm wondering if we can use the last_clipboard_change_time_ms_ to actually not
pull the text and html text if it hasn't changed (it might need to be a
base::Optional or something to handle the initial unset state).

JNI for strings isn't "super" expensive but it isn't free either and if someone
were to copy a very long string, this could actually get somewhat expensive.

Another option entirely would be to not hook the change time into the update
from android call and have the above getter just call directly into the java
side.  That would avoid any of this caching logic as well, which doesn't buy us
much here.

[Mark P, 2017-03-30 16:59:26]

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
On 2017/03/30 16:55:21, Ted C wrote:
> How often are we going to be asking for the clipboard age?  Every omnibox
> keystroke?

Not per keystroke.  The current plan only involves checking for and showing the
clipboard suggestion on omnibox focus.  It disappears when the user types.

Given this answer, do you consider either of the suggestions below necessary?

> I'm wondering if we can use the last_clipboard_change_time_ms_ to actually not
> pull the text and html text if it hasn't changed (it might need to be a
> base::Optional or something to handle the initial unset state).
> 
> JNI for strings isn't "super" expensive but it isn't free either and if
someone
> were to copy a very long string, this could actually get somewhat expensive.
> 
> Another option entirely would be to not hook the change time into the update
> from android call and have the above getter just call directly into the java
> side.  That would avoid any of this caching logic as well, which doesn't buy
us
> much here.

[Ted C, 2017-03-30 17:02:06]

lgtm

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
On 2017/03/30 16:59:26, Mark P wrote:
> On 2017/03/30 16:55:21, Ted C wrote:
> > How often are we going to be asking for the clipboard age?  Every omnibox
> > keystroke?
> 
> Not per keystroke.  The current plan only involves checking for and showing
the
> clipboard suggestion on omnibox focus.  It disappears when the user types.
> 
> Given this answer, do you consider either of the suggestions below necessary?
> 
> > I'm wondering if we can use the last_clipboard_change_time_ms_ to actually
not
> > pull the text and html text if it hasn't changed (it might need to be a
> > base::Optional or something to handle the initial unset state).
> > 
> > JNI for strings isn't "super" expensive but it isn't free either and if
> someone
> > were to copy a very long string, this could actually get somewhat expensive.
> > 
> > Another option entirely would be to not hook the change time into the update
> > from android call and have the above getter just call directly into the java
> > side.  That would avoid any of this caching logic as well, which doesn't buy
> us
> > much here.

I would personally probably inline the JNI into the above getter, but I'll defer
to you.  This class is weird in general and I don't know why there is a map at
all.

[Mark P, 2017-03-30 17:27:16]

mpearson@chromium.org changed reviewers:
+ dcheng@chromium.org

[Mark P, 2017-03-30 17:33:36]

dcheng@,

Can you please review the ui/base/clipboard changes?

thanks,
mark

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
On 2017/03/30 17:02:06, Ted C wrote:
> On 2017/03/30 16:59:26, Mark P wrote:
> > On 2017/03/30 16:55:21, Ted C wrote:
> > > How often are we going to be asking for the clipboard age?  Every omnibox
> > > keystroke?
> > 
> > Not per keystroke.  The current plan only involves checking for and showing
> the
> > clipboard suggestion on omnibox focus.  It disappears when the user types.
> > 
> > Given this answer, do you consider either of the suggestions below
necessary?
> > 
> > > I'm wondering if we can use the last_clipboard_change_time_ms_ to actually
> not
> > > pull the text and html text if it hasn't changed (it might need to be a
> > > base::Optional or something to handle the initial unset state).
> > > 
> > > JNI for strings isn't "super" expensive but it isn't free either and if
> > someone
> > > were to copy a very long string, this could actually get somewhat
expensive.
> > > 
> > > Another option entirely would be to not hook the change time into the
update
> > > from android call and have the above getter just call directly into the
java
> > > side.  That would avoid any of this caching logic as well, which doesn't
buy
> > us
> > > much here.
> 
> I would personally probably inline the JNI into the above getter, but I'll
defer
> to you.  This class is weird in general and I don't know why there is a map at
> all.

I don't understand the current structure either.  I'll leave this comment here
so dcheng@ can comment/decide.

[dcheng, 2017-03-30 19:01:11]

The change LGTM as-is, but if you're up for it, I think it wouldn't hurt to
modernize UpdateFromAndroidClipboard() a bit.

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
On 2017/03/30 17:33:36, Mark P wrote:
> On 2017/03/30 17:02:06, Ted C wrote:
> > On 2017/03/30 16:59:26, Mark P wrote:
> > > On 2017/03/30 16:55:21, Ted C wrote:
> > > > How often are we going to be asking for the clipboard age?  Every
omnibox
> > > > keystroke?
> > > 
> > > Not per keystroke.  The current plan only involves checking for and
showing
> > the
> > > clipboard suggestion on omnibox focus.  It disappears when the user types.
> > > 
> > > Given this answer, do you consider either of the suggestions below
> necessary?
> > > 
> > > > I'm wondering if we can use the last_clipboard_change_time_ms_ to
actually
> > not
> > > > pull the text and html text if it hasn't changed (it might need to be a
> > > > base::Optional or something to handle the initial unset state).
> > > > 
> > > > JNI for strings isn't "super" expensive but it isn't free either and if
> > > someone
> > > > were to copy a very long string, this could actually get somewhat
> expensive.
> > > > 
> > > > Another option entirely would be to not hook the change time into the
> update
> > > > from android call and have the above getter just call directly into the
> java
> > > > side.  That would avoid any of this caching logic as well, which doesn't
> buy
> > > us
> > > > much here.
> > 
> > I would personally probably inline the JNI into the above getter, but I'll
> defer
> > to you.  This class is weird in general and I don't know why there is a map
at
> > all.
> 
> I don't understand the current structure either.  I'll leave this comment here
> so dcheng@ can comment/decide.

Either option seems reasonable here. The original idea was to allow
ClipboardAndroid to accumulate changes that couldn't easily be persisted back to
the Android clipboard, but make sure that the internal ClipboardAndroid state
and the Android system clipboard state remained roughly in sync.

That being said, I'd probably use an actual sequence number rather than relying
on the last changed time for shortcutting this work. It should be pretty easy to
implement now that we have a clipboard change listener, right?

(And this would let us implement ClipboardAndroid::GetSequenceNumber correctly
as well)

[Mark P, 2017-03-30 19:10:48]

The CQ bit was checked by mpearson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 19:11:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-03-30 19:15:03]

>>>
The change LGTM as-is, but if you're up for it, I think it wouldn't hurt to
modernize UpdateFromAndroidClipboard() a bit.
>>>

I'm not up for it. :-P  This is my first changelist that crosses the Java<->C++
boundary.  I'm uncomfortable at this point to muck around with this more than
strictly necessary.

Tentatively planning to subject assuming the trybots look good.

cheers,
mark

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
On 2017/03/30 19:01:11, dcheng wrote:
> On 2017/03/30 17:33:36, Mark P wrote:
> > On 2017/03/30 17:02:06, Ted C wrote:
> > > On 2017/03/30 16:59:26, Mark P wrote:
> > > > On 2017/03/30 16:55:21, Ted C wrote:
> > > > > How often are we going to be asking for the clipboard age?  Every
> omnibox
> > > > > keystroke?
> > > > 
> > > > Not per keystroke.  The current plan only involves checking for and
> showing
> > > the
> > > > clipboard suggestion on omnibox focus.  It disappears when the user
types.
> > > > 
> > > > Given this answer, do you consider either of the suggestions below
> > necessary?
> > > > 
> > > > > I'm wondering if we can use the last_clipboard_change_time_ms_ to
> actually
> > > not
> > > > > pull the text and html text if it hasn't changed (it might need to be
a
> > > > > base::Optional or something to handle the initial unset state).
> > > > > 
> > > > > JNI for strings isn't "super" expensive but it isn't free either and
if
> > > > someone
> > > > > were to copy a very long string, this could actually get somewhat
> > expensive.
> > > > > 
> > > > > Another option entirely would be to not hook the change time into the
> > update
> > > > > from android call and have the above getter just call directly into
the
> > java
> > > > > side.  That would avoid any of this caching logic as well, which
doesn't
> > buy
> > > > us
> > > > > much here.
> > > 
> > > I would personally probably inline the JNI into the above getter, but I'll
> > defer
> > > to you.  This class is weird in general and I don't know why there is a
map
> at
> > > all.
> > 
> > I don't understand the current structure either.  I'll leave this comment
here
> > so dcheng@ can comment/decide.
> 
> Either option seems reasonable here. The original idea was to allow
> ClipboardAndroid to accumulate changes that couldn't easily be persisted back
to
> the Android clipboard, but make sure that the internal ClipboardAndroid state
> and the Android system clipboard state remained roughly in sync.

Ah, that makes sense (as an ambition).

> That being said, I'd probably use an actual sequence number rather than
relying
> on the last changed time for shortcutting this work. It should be pretty easy
to
> implement now that we have a clipboard change listener, right?

Actually, for the changelist that comes next, I need the actual timestamp. 
(This is to get the omnibox to suggest URLs in the clipboard, and we want it not
to suggest URLs that are too old.  Hence, the timestamp; a sequence number
doesn't suffice.)

> (And this would let us implement ClipboardAndroid::GetSequenceNumber correctly
> as well)

[commit-bot: I haz the power, 2017-03-30 19:47:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 19:47:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mark P, 2017-03-30 22:56:30]

The CQ bit was checked by mpearson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:57:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-03-30 23:44:27]

The CQ bit was checked by mpearson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 23:45:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-31 01:01:37]

On 2017/03/30 19:15:03, Mark P wrote:
> >>>
> The change LGTM as-is, but if you're up for it, I think it wouldn't hurt to
> modernize UpdateFromAndroidClipboard() a bit.
> >>>
> 
> I'm not up for it. :-P  This is my first changelist that crosses the
Java<->C++
> boundary.  I'm uncomfortable at this point to muck around with this more than
> strictly necessary.
> 
> Tentatively planning to subject assuming the trybots look good.
> 
> cheers,
> mark
> 
>
https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
> File ui/base/clipboard/clipboard_android.cc (right):
> 
>
https://codereview.chromium.org/2766623003/diff/80001/ui/base/clipboard/clipb...
> ui/base/clipboard/clipboard_android.cc:173: last_clipboard_change_time_ms_ =
> On 2017/03/30 19:01:11, dcheng wrote:
> > On 2017/03/30 17:33:36, Mark P wrote:
> > > On 2017/03/30 17:02:06, Ted C wrote:
> > > > On 2017/03/30 16:59:26, Mark P wrote:
> > > > > On 2017/03/30 16:55:21, Ted C wrote:
> > > > > > How often are we going to be asking for the clipboard age?  Every
> > omnibox
> > > > > > keystroke?
> > > > > 
> > > > > Not per keystroke.  The current plan only involves checking for and
> > showing
> > > > the
> > > > > clipboard suggestion on omnibox focus.  It disappears when the user
> types.
> > > > > 
> > > > > Given this answer, do you consider either of the suggestions below
> > > necessary?
> > > > > 
> > > > > > I'm wondering if we can use the last_clipboard_change_time_ms_ to
> > actually
> > > > not
> > > > > > pull the text and html text if it hasn't changed (it might need to
be
> a
> > > > > > base::Optional or something to handle the initial unset state).
> > > > > > 
> > > > > > JNI for strings isn't "super" expensive but it isn't free either and
> if
> > > > > someone
> > > > > > were to copy a very long string, this could actually get somewhat
> > > expensive.
> > > > > > 
> > > > > > Another option entirely would be to not hook the change time into
the
> > > update
> > > > > > from android call and have the above getter just call directly into
> the
> > > java
> > > > > > side.  That would avoid any of this caching logic as well, which
> doesn't
> > > buy
> > > > > us
> > > > > > much here.
> > > > 
> > > > I would personally probably inline the JNI into the above getter, but
I'll
> > > defer
> > > > to you.  This class is weird in general and I don't know why there is a
> map
> > at
> > > > all.
> > > 
> > > I don't understand the current structure either.  I'll leave this comment
> here
> > > so dcheng@ can comment/decide.
> > 
> > Either option seems reasonable here. The original idea was to allow
> > ClipboardAndroid to accumulate changes that couldn't easily be persisted
back
> to
> > the Android clipboard, but make sure that the internal ClipboardAndroid
state
> > and the Android system clipboard state remained roughly in sync.
> 
> Ah, that makes sense (as an ambition).
> 
> > That being said, I'd probably use an actual sequence number rather than
> relying
> > on the last changed time for shortcutting this work. It should be pretty
easy
> to
> > implement now that we have a clipboard change listener, right?
> 
> Actually, for the changelist that comes next, I need the actual timestamp. 
> (This is to get the omnibox to suggest URLs in the clipboard, and we want it
not
> to suggest URLs that are too old.  Hence, the timestamp; a sequence number
> doesn't suffice.)
> 
> > (And this would let us implement ClipboardAndroid::GetSequenceNumber
correctly
> > as well)

Yeah, I know we need timestamp -- but I'm suggesting that doing all the JNI
calls, etc be gated on whether or not the clipboard sequence number changed,
rather than the last changed time.

[commit-bot: I haz the power, 2017-03-31 01:16:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 01:16:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mark P, 2017-03-31 04:03:44]

> Yeah, I know we need timestamp -- but I'm suggesting that doing all the JNI
> calls, etc be gated on whether or not the clipboard sequence number changed,
> rather than the last changed time.

Oh, I think I understand.  Add a new function on the Java side,
GetSequenceNumber(), that's calculated correctly.  (Incremented whenever the
clipboard changes.)  Give ClipboardMap a local copy of the sequence number. 
Also expose a function to fetch it from the Java side.  Have the
UpdateFromAndroidClipboard() first do a Java call to fetch the current sequence
number.  Only if it's different does UpdateFromAndroidClipboard() bother
updating its other cached variables.

And this also lets ClipboardAndroid return a plausible sequence number.  (Ask
ClipboarMap to fetch the current sequence number from the Java side.)

Is that right?

I think that should happen in a different changelist, not this one.

If I got that right, I can file a bug to track the idea / work.

--mark

[dcheng, 2017-03-31 04:08:52]

On 2017/03/31 04:03:44, Mark P wrote:
> > Yeah, I know we need timestamp -- but I'm suggesting that doing all the JNI
> > calls, etc be gated on whether or not the clipboard sequence number changed,
> > rather than the last changed time.
> 
> Oh, I think I understand.  Add a new function on the Java side,
> GetSequenceNumber(), that's calculated correctly.  (Incremented whenever the
> clipboard changes.)  Give ClipboardMap a local copy of the sequence number. 
> Also expose a function to fetch it from the Java side.  Have the
> UpdateFromAndroidClipboard() first do a Java call to fetch the current
sequence
> number.  Only if it's different does UpdateFromAndroidClipboard() bother
> updating its other cached variables.
> 
> And this also lets ClipboardAndroid return a plausible sequence number.  (Ask
> ClipboarMap to fetch the current sequence number from the Java side.)
> 
> Is that right?
> 
> I think that should happen in a different changelist, not this one.
> 
> If I got that right, I can file a bug to track the idea / work.
> 
> --mark

Yep, and separate CL sounds pretty reasonable to me.

(My LGTM still stands)

[Mark P, 2017-03-31 04:59:10]

https://codereview.chromium.org/2766623003/diff/120001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/Clipboard.java (right):

https://codereview.chromium.org/2766623003/diff/120001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/Clipboard.java:44: // TODO(mpearson):
unsuppress this warning once saving and restoring
I'm sneaking in this suppression because the hashing code and its related
metrics have already been reviewed.  Admittedly, it isn't needed until the
saving/restoring is added, but I expect to do that next week. Ted, please shout
if instead you want me to rip out the hashing code and put it in the next
changelist instead.

[Mark P, 2017-03-31 20:54:01]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2017-03-31 20:54:01]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2766623003/#ps160001
(title: "final attempt at restoring")

[commit-bot: I haz the power, 2017-03-31 20:54:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-03-31 20:58:48]

On 2017/03/31 04:08:52, dcheng wrote:
> On 2017/03/31 04:03:44, Mark P wrote:
> > > Yeah, I know we need timestamp -- but I'm suggesting that doing all the
JNI
> > > calls, etc be gated on whether or not the clipboard sequence number
changed,
> > > rather than the last changed time.
> > 
> > Oh, I think I understand.  Add a new function on the Java side,
> > GetSequenceNumber(), that's calculated correctly.  (Incremented whenever the
> > clipboard changes.)  Give ClipboardMap a local copy of the sequence number. 
> > Also expose a function to fetch it from the Java side.  Have the
> > UpdateFromAndroidClipboard() first do a Java call to fetch the current
> sequence
> > number.  Only if it's different does UpdateFromAndroidClipboard() bother
> > updating its other cached variables.
> > 
> > And this also lets ClipboardAndroid return a plausible sequence number. 
(Ask
> > ClipboarMap to fetch the current sequence number from the Java side.)
> > 
> > Is that right?
> > 
> > I think that should happen in a different changelist, not this one.
> > 
> > If I got that right, I can file a bug to track the idea / work.
> > 
> > --mark
> 
> Yep, and separate CL sounds pretty reasonable to me.
Filed crbug.com/707366 so I don't forget.

> (My LGTM still stands)

[commit-bot: I haz the power, 2017-03-31 21:27:02]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490993641455720,
"parent_rev": "1a54df6adc97a74a1747a41e948f1346b080156e", "commit_rev":
"6f6d07f111d9df9bc70ff3dc71612d940d9b2d17"}

[commit-bot: I haz the power, 2017-03-31 21:27:52]

Description was changed from

==========
Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

To do in a later changelist: add lightweight code to store the hash and update
time in prefs so that Chrome can have a better chance of using the current
clipboard content on startup.  (If the clipboard content hasn't changed since
the last time Chrome ran, the last modified date is likely correct.)

BUG=682446
==========

to

==========
Make Android Clipboard Keep Track of Last Modified Time

And expose it the C++ code.

To do in a later changelist: add lightweight code to store the hash and update
time in prefs so that Chrome can have a better chance of using the current
clipboard content on startup.  (If the clipboard content hasn't changed since
the last time Chrome ran, the last modified date is likely correct.)

BUG=682446

Review-Url: https://codereview.chromium.org/2766623003
Cr-Commit-Position: refs/heads/master@{#461234}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f6d07f111d9df9bc70ff3dc7161...
==========

[commit-bot: I haz the power, 2017-03-31 21:27:53]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/6f6d07f111d9df9bc70ff3dc7161...