Refactor Clipboard Last Modified Time Storage

Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses |last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h for
this.

* remove all the logic about hashing clipboard content from Clipboard.java.  This is
not needed based on our usage of clipboard last modified time.  (This usage has been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls" clipboard
information from the Java side when someone asks it about the current state of the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

TBR=sky,jif
(sky@ for the trivial changes to ui/base/test/...)
(jif@ for the refactoring / removal of logic from 	components/open_from_clipboard/...)

BUG=682446, 707366
Review-Url: https://codereview.chromium.org/2812773002
Cr-Commit-Position: refs/heads/master@{#464594}
Committed: https://chromium.googlesource.com/chromium/src/+/71dfbd1f2228ae70efcea0656b3555ab539430fd

[Mark P, 2017-04-10 23:20:34]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

BUG=682446,707366
==========

[Mark P, 2017-04-10 23:20:35]

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

[Mark P, 2017-04-10 23:22:33]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

BUG=682446,707366
==========

[Mark P, 2017-04-10 23:45:36]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to owner the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.

As a consequence of this decision, this change

* clipboard_android.cc needs to register a last modified time pref and read and
store it at the right times.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as that suppression logic is unnecessary. 
Instead, when needing to suppress a suggestion, it tells clipboard_android to
set its time to 0, which would cause suppression.  Adds a function
ClearClipboardLastModifiedTime() to clipboard.h for this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is not needed based on our usage of clipboard last modified time.  (This
usage has been approved by privacy.)  This also means marking a related
histogram as obsolete.

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.  Make Clipboard.java send
clipboard updated messages to clipboard_android.cc.

* Delete ClipboardTest.java.  The only thing this tests was updating the last
modified time, which isn't used anymore.


BUG=682446,707366
==========

[Mark P, 2017-04-10 23:48:52]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to owner the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.

As a consequence of this decision, this change

* clipboard_android.cc needs to register a last modified time pref and read and
store it at the right times.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as that suppression logic is unnecessary. 
Instead, when needing to suppress a suggestion, it tells clipboard_android to
set its time to 0, which would cause suppression.  Adds a function
ClearClipboardLastModifiedTime() to clipboard.h for this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is not needed based on our usage of clipboard last modified time.  (This
usage has been approved by privacy.)  This also means marking a related
histogram as obsolete.

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.  Make Clipboard.java send
clipboard updated messages to clipboard_android.cc.

* Delete ClipboardTest.java.  The only thing this tests was updating the last
modified time, which isn't used anymore.


BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to owner the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.

As a consequence of this decision, this change

* clipboard_android.cc needs to register a last modified time pref and read and
store it at the right times.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as that suppression logic is unnecessary. 
Instead, when needing to suppress a suggestion, it tells clipboard_android to
set its time to 0, which would cause suppression.  Adds a function
ClearClipboardLastModifiedTime() to clipboard.h for this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is not needed based on our usage of clipboard last modified time.  (This
usage has been approved by privacy.)  This also means marking a related
histogram as obsolete.

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.  Make Clipboard.java send
clipboard updated messages to clipboard_android.cc.

* Because Clipboard.java sends clipboard change messages now to
clipboard_android, add support for sequence numbers to clipboard_anroid.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it no longer needs to "pull" clipboard information when someone
asks it about the current state of the clipboard.  Instead, it "pull" the
current state of the clipboard only when it is told the clipboard has changed. 
Thus, it's data structures always stay up-to-date and no longer need to be
updated when someone queries for them.

* Delete ClipboardTest.java.  The only thing this tests was updating the last
modified time, which isn't used anymore.

BUG=682446,707366
==========

[Mark P, 2017-04-11 04:12:02]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to owner the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.

As a consequence of this decision, this change

* clipboard_android.cc needs to register a last modified time pref and read and
store it at the right times.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as that suppression logic is unnecessary. 
Instead, when needing to suppress a suggestion, it tells clipboard_android to
set its time to 0, which would cause suppression.  Adds a function
ClearClipboardLastModifiedTime() to clipboard.h for this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is not needed based on our usage of clipboard last modified time.  (This
usage has been approved by privacy.)  This also means marking a related
histogram as obsolete.

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.  Make Clipboard.java send
clipboard updated messages to clipboard_android.cc.

* Because Clipboard.java sends clipboard change messages now to
clipboard_android, add support for sequence numbers to clipboard_anroid.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it no longer needs to "pull" clipboard information when someone
asks it about the current state of the clipboard.  Instead, it "pull" the
current state of the clipboard only when it is told the clipboard has changed. 
Thus, it's data structures always stay up-to-date and no longer need to be
updated when someone queries for them.

* Delete ClipboardTest.java.  The only thing this tests was updating the last
modified time, which isn't used anymore.

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to owner the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_android.cc register a last modified time pref and read and store
it at the right times.

* clipboard_recent_content_generic.{cc,h} lose
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_anroid.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it no longer necessarily has to "pull" clipboard information when
someone asks it about the current state of the clipboard.  Instead, we have it
"pull"
the current state of the clipboard only when it is told the clipboard has
changed.
Thus, clipboard_android's data structures always stay up-to-date and no longer
need to be updated when someone queries for them.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

BUG=682446,707366
==========

[Mark P, 2017-04-11 04:31:26]

dcheng@,

Can you please take a look at draft changelist and tell me what you
think?

It's not quite complete.  Nonetheless, I'd feedback to make sure I'm on a
reasonable track before I finish it up.  Right now there are some things
missing:
* it's basically just a sketch of the code.  There is at least one bug
somewhere, as I can tell because the code crashes. :-)
* It doesn't actually save from or load from prefs.  This is mainly because I
cannot figure out where / how ClipboardAndroid is constructed so I can pass a
LocalState variable to it so it can access prefs.
* I need to revise / expand
clipboard_recent_content_generic_unittests.cc
* Likewise, I'll expand test_clipboard.{cc,h} for those purposes.

thanks,
mark

[dcheng, 2017-04-12 00:56:09]

Sorry for the delayed review. The overall approach looks reasonable. As
mentioned in the comments, I wonder if we can delay reading out actual content
from the clipboard until it's actually requested though.

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.h:213: virtual void
ClearClipboardLastModifiedTime();
Nit: omit Clipboard (probably should omit from GetClipboardLastModifiedTime too)

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

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:61: void
ClearClipboardLastModifiedTime();
Ditto: omit Clipboard in the names here.

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:75: base::Time
clipboard_last_modified_time_;
Nit: no clipboard_

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:120: UpdateFromAndroidClipboard();
I do wonder if we want to do this work up-front when the clipboard changes, or
just delay it to when the clipboard contents are actually read. The latter would
be a little more complex, but if we only invalidate here and don't repopulate
until data is actually read, that might be nicer from a performance perspective?

[Mark P, 2017-04-12 04:21:11]

Thanks for your preliminary feedback.  I'll get it this first thing tomorrow.

Do you have insight into this question:
* It doesn't actually save from or load from prefs.  This is mainly
because I cannot figure out where / how ClipboardAndroid is
constructed so I can pass a LocalState variable to it so it can access
prefs.

I'm still having trouble figuring out the plumbing here.

thanks,
mark

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

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:120: UpdateFromAndroidClipboard();
On 2017/04/12 00:56:09, dcheng wrote:
> I do wonder if we want to do this work up-front when the clipboard changes, or
> just delay it to when the clipboard contents are actually read. The latter
would
> be a little more complex, but if we only invalidate here and don't repopulate
> until data is actually read, that might be nicer from a performance
perspective?

Sure, I think I can do that pretty cleanly.
I'll make a bool |map_up_to_date|.

[dcheng, 2017-04-12 07:10:18]

On 2017/04/12 04:21:11, Mark P wrote:
> Thanks for your preliminary feedback.  I'll get it this first thing tomorrow.
> 
> Do you have insight into this question:
> * It doesn't actually save from or load from prefs.  This is mainly
> because I cannot figure out where / how ClipboardAndroid is
> constructed so I can pass a LocalState variable to it so it can access
> prefs.

Ah... that may be tricky. ui::Clipboard is global, while profiles/prefs are not.
How do we expect this to interact with multiple profiles? Maybe the prefs code
could live outside clipboard and push the mtime read from prefs in?

> 
> I'm still having trouble figuring out the plumbing here.
> 
> thanks,
> mark
> 
>
https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
> File ui/base/clipboard/clipboard_android.cc (right):
> 
>
https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
> ui/base/clipboard/clipboard_android.cc:120: UpdateFromAndroidClipboard();
> On 2017/04/12 00:56:09, dcheng wrote:
> > I do wonder if we want to do this work up-front when the clipboard changes,
or
> > just delay it to when the clipboard contents are actually read. The latter
> would
> > be a little more complex, but if we only invalidate here and don't
repopulate
> > until data is actually read, that might be nicer from a performance
> perspective?
> 
> Sure, I think I can do that pretty cleanly.
> I'll make a bool |map_up_to_date|.

[Mark P, 2017-04-12 18:32:01]

On Wed, Apr 12, 2017 at 12:10 AM, <dcheng@chromium.org> wrote:

> On 2017/04/12 04:21:11, Mark P wrote:
> > Thanks for your preliminary feedback. I'll get it this first thing
> tomorrow.
> >
> > Do you have insight into this question:
> > * It doesn't actually save from or load from prefs. This is mainly
> > because I cannot figure out where / how ClipboardAndroid is
> > constructed so I can pass a LocalState variable to it so it can access
> > prefs.
>
> Ah... that may be tricky. ui::Clipboard is global, while profiles/prefs
> are not.
> How do we expect this to interact with multiple profiles? Maybe the prefs
> code
> could live outside clipboard and push the mtime read from prefs in?
>

Oh, this last modified "pref" is meant to be global, not associated with a
particular profile.

I guess I misread the docs about PrefRegistrySimple.

How/where do I store global state?

Regardless of the answer, I think this code ought to live in
clipboard_android.cc.  After all, the state has to be current, which means
clipboard android has to push writes to this state when necessary.  (The
state cannot be depended on to pull information.)  And if clipboard android
has to push information, then it needs to depend on the code that it needs
to call, which means adding a new dependency to ui/base/clipboard. :-(

The only other option to make an OnClipboardChangedListener and have
clipboard_android announce changes in that way.  But that seems heavyweight.

--mark


>
>
> >
> > I'm still having trouble figuring out the plumbing here.
> >
> > thanks,
> > mark
> >
> >
> https://codereview.chromium.org/2812773002/diff/20001/ui/
> base/clipboard/clipboard_android.cc
> > File ui/base/clipboard/clipboard_android.cc (right):
> >
> >
> https://codereview.chromium.org/2812773002/diff/20001/ui/
> base/clipboard/clipboard_android.cc#newcode120
> > ui/base/clipboard/clipboard_android.cc:120:
> UpdateFromAndroidClipboard();
> > On 2017/04/12 00:56:09, dcheng wrote:
> > > I do wonder if we want to do this work up-front when the clipboard
> changes,
> or
> > > just delay it to when the clipboard contents are actually read. The
> latter
> > would
> > > be a little more complex, but if we only invalidate here and don't
> repopulate
> > > until data is actually read, that might be nicer from a performance
> > perspective?
> >
> > Sure, I think I can do that pretty cleanly.
> > I'll make a bool |map_up_to_date|.
>
> https://codereview.chromium.org/2812773002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2017-04-12 18:46:26]

On Wed, Apr 12, 2017 at 11:31 AM, Mark Pearson <mpearson@chromium.org>
wrote:

>
> On Wed, Apr 12, 2017 at 12:10 AM, <dcheng@chromium.org> wrote:
>
>> On 2017/04/12 04:21:11, Mark P wrote:
>> > Thanks for your preliminary feedback. I'll get it this first thing
>> tomorrow.
>> >
>> > Do you have insight into this question:
>> > * It doesn't actually save from or load from prefs. This is mainly
>> > because I cannot figure out where / how ClipboardAndroid is
>> > constructed so I can pass a LocalState variable to it so it can access
>> > prefs.
>>
>> Ah... that may be tricky. ui::Clipboard is global, while profiles/prefs
>> are not.
>> How do we expect this to interact with multiple profiles? Maybe the prefs
>> code
>> could live outside clipboard and push the mtime read from prefs in?
>>
>
> Oh, this last modified "pref" is meant to be global, not associated with a
> particular profile.
>
> I guess I misread the docs about PrefRegistrySimple.
>

Nevermind, I see I'm doing it right. :

> Preferences can be *associated* with a *browser profile* or with *local
> state*. Local state contains everything that is not directly associated
> to a specific profile but rather represents values that are associated with
> the user account on the host computer (e.g. “Was the browser shut down
> correctly last time?”, “When was the ‘GPU black list’ downloaded the last
> time?”, etc.).

http://www.chromium.org/developers/design-documents/preferences

That's why I was asking about

> where / how ClipboardAndroid is constructed so I can pass a LocalState
> variable to it so it can access prefs


:-)

feeling brighter,
mark


> How/where do I store global state?
>
> Regardless of the answer, I think this code ought to live in
> clipboard_android.cc.  After all, the state has to be current, which means
> clipboard android has to push writes to this state when necessary.  (The
> state cannot be depended on to pull information.)  And if clipboard android
> has to push information, then it needs to depend on the code that it needs
> to call, which means adding a new dependency to ui/base/clipboard. :-(
>
> The only other option to make an OnClipboardChangedListener and have
> clipboard_android announce changes in that way.  But that seems heavyweight.
>
> --mark
>
>
>>
>>
>> >
>> > I'm still having trouble figuring out the plumbing here.
>> >
>> > thanks,
>> > mark
>> >
>> >
>> https://codereview.chromium.org/2812773002/diff/20001/ui/bas
>> e/clipboard/clipboard_android.cc
>> > File ui/base/clipboard/clipboard_android.cc (right):
>> >
>> >
>> https://codereview.chromium.org/2812773002/diff/20001/ui/bas
>> e/clipboard/clipboard_android.cc#newcode120
>> > ui/base/clipboard/clipboard_android.cc:120:
>> UpdateFromAndroidClipboard();
>> > On 2017/04/12 00:56:09, dcheng wrote:
>> > > I do wonder if we want to do this work up-front when the clipboard
>> changes,
>> or
>> > > just delay it to when the clipboard contents are actually read. The
>> latter
>> > would
>> > > be a little more complex, but if we only invalidate here and don't
>> repopulate
>> > > until data is actually read, that might be nicer from a performance
>> > perspective?
>> >
>> > Sure, I think I can do that pretty cleanly.
>> > I'll make a bool |map_up_to_date|.
>>
>> https://codereview.chromium.org/2812773002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2017-04-12 21:28:55]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage, Add to Prefs

This changelist solves two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to owner the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_android.cc register a last modified time pref and read and store
it at the right times.

* clipboard_recent_content_generic.{cc,h} lose
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_anroid.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it no longer necessarily has to "pull" clipboard information when
someone asks it about the current state of the clipboard.  Instead, we have it
"pull"
the current state of the clipboard only when it is told the clipboard has
changed.
Thus, clipboard_android's data structures always stay up-to-date and no longer
need to be updated when someone queries for them.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

BUG=682446,707366
==========

[Mark P, 2017-04-12 21:29:40]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

BUG=682446,707366
==========

[Mark P, 2017-04-12 21:33:52]

Hey dcheng@

This changelist is complete and ready for review.  It works.

I ripped out the prefs storage stuff, leaving this changelist as simply the
last_modified_time refactoring and its side benefits of supporting sequence
numbers and also only pulling the clipboard state from
Java when necessary.

I'd prefer if this changelist was submitted this week.  Can you please
review it promptly?

Then I'll do the follow-up changelist to add the prefs storage.  I figure if
that doesn't get by the branch point, that's okay, as it'll probably be easier
to merge later than this complex one that spans Java and C++.

I would still like your help in figuring out where ClipboardAndroid is
constructed globally so I can give it a pointer to LocalState to store the pref.

thanks,
mark

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.h:213: virtual void
ClearClipboardLastModifiedTime();
On 2017/04/12 00:56:09, dcheng wrote:
> Nit: omit Clipboard (probably should omit from GetClipboardLastModifiedTime
too)

Good idea.  Clipboard is obvious.  Did both.

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

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:61: void
ClearClipboardLastModifiedTime();
On 2017/04/12 00:56:09, dcheng wrote:
> Ditto: omit Clipboard in the names here.

Done.

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:75: base::Time
clipboard_last_modified_time_;
On 2017/04/12 00:56:09, dcheng wrote:
> Nit: no clipboard_

Done.

https://codereview.chromium.org/2812773002/diff/20001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_android.cc:120: UpdateFromAndroidClipboard();
On 2017/04/12 04:21:11, Mark P wrote:
> On 2017/04/12 00:56:09, dcheng wrote:
> > I do wonder if we want to do this work up-front when the clipboard changes,
or
> > just delay it to when the clipboard contents are actually read. The latter
> would
> > be a little more complex, but if we only invalidate here and don't
repopulate
> > until data is actually read, that might be nicer from a performance
> perspective?
> 
> Sure, I think I can do that pretty cleanly.
> I'll make a bool |map_up_to_date|.

Done.

[Mark P, 2017-04-12 23:55:32]

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

[Mark P, 2017-04-12 23:55:32]

Ted,

This will need your review too.  Please take a look at the
ui/android/... changes
as well as the
ui/base/android/ui_base_jni_registrar.cc

I'm trusting dcheng@ to review the whole changelist, especially the
clipboard_android stuff, so no need for you to look beyond those files I listed
above (unless you want to).

thanks,
mark

[Mark P, 2017-04-12 23:55:49]

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

[commit-bot: I haz the power, 2017-04-12 23:56:52]

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

[commit-bot: I haz the power, 2017-04-13 01:55:43]

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

[commit-bot: I haz the power, 2017-04-13 01:55:44]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-13 18:01:15]

On 2017/04/12 23:55:32, Mark P wrote:
> Ted,
> 
> This will need your review too.  Please take a look at the
> ui/android/... changes
> as well as the
> ui/base/android/ui_base_jni_registrar.cc
> 
> I'm trusting dcheng@ to review the whole changelist, especially the
> clipboard_android stuff, so no need for you to look beyond those files I
listed
> above (unless you want to).
> 
> thanks,
> mark

android-y things lgtm

[Mark P, 2017-04-13 19:08:42]

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

[commit-bot: I haz the power, 2017-04-13 19:09:17]

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-04-13 19:57:46]

Oy, apparently today is the branch, not tomorrow...

--mark

[Mark P, 2017-04-13 20:00:49]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

TBR=sky
(for the trivial changes to ui/base/test/...)

BUG=682446,707366
==========

[Mark P, 2017-04-13 20:03:16]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

TBR=sky
(for the trivial changes to ui/base/test/...)

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

TBR=sky,jif
(sky@ for the trivial changes to ui/base/test/...)
(jif@ for the refactoring / removal of logic from
	components/open_from_clipboard/...)

BUG=682446,707366
==========

[commit-bot: I haz the power, 2017-04-13 20:25:13]

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

[commit-bot: I haz the power, 2017-04-13 20:25:14]

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

[dcheng, 2017-04-13 20:35:37]

Mostly LG. Two minor comments, and also just to double-check: since the
clipboard code isn't really tested by unit tests on Android, did you verify that
it still works?

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:82: map_up_to_date_ = false;
Nit: use the initializer list (or use in-class initialization)

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:119: map_up_to_date_ = false;
Can we make this a tri-state enum? Calling UpdateFromAndroidClipboard() in this
state should be considered an error (since this would blow away the temporary
state that we're trying to commit to the clipboard).

[Mark P, 2017-04-13 20:54:05]

On 2017/04/13 20:35:37, dcheng wrote:
> Mostly LG. Two minor comments, and also just to double-check: since the
> clipboard code isn't really tested by unit tests on Android, did you verify
that
> it still works?

I did some basic copying, pasting, and cutting, both in the Chrome UI (omnibox)
and within web content.  It all seems to work.  I'm not sure if I'm tickling all
the possible behaviors though; I don't know everywhere that clipboard may be
used.

--mark

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:82: map_up_to_date_ = false;
On 2017/04/13 20:35:37, dcheng wrote:
> Nit: use the initializer list (or use in-class initialization)

Done.

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:119: map_up_to_date_ = false;
On 2017/04/13 20:35:37, dcheng wrote:
> Can we make this a tri-state enum? Calling UpdateFromAndroidClipboard() in
this
> state should be considered an error (since this would blow away the temporary
> state that we're trying to commit to the clipboard).

Do you mean
MAP_OUT_OF_DATE
MAP_FRESHER_THAN_JAVA
MAP_UP_TO_DATE
?

And DCHECK if UpdateFromAndroidClipboard() is called from the middle state?

[Mark P, 2017-04-13 20:54:14]

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

[commit-bot: I haz the power, 2017-04-13 20:54:47]

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

[dcheng, 2017-04-13 20:59:18]

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:119: map_up_to_date_ = false;
On 2017/04/13 20:54:05, Mark P wrote:
> On 2017/04/13 20:35:37, dcheng wrote:
> > Can we make this a tri-state enum? Calling UpdateFromAndroidClipboard() in
> this
> > state should be considered an error (since this would blow away the
temporary
> > state that we're trying to commit to the clipboard).
> 
> Do you mean
> MAP_OUT_OF_DATE
> MAP_FRESHER_THAN_JAVA
> MAP_UP_TO_DATE
> ?
> 
> And DCHECK if UpdateFromAndroidClipboard() is called from the middle state?
> 

MAP_OUT_OF_DATE
MAP_UP_TO_DATE
MAP_PREPARING_COMMIT

would be my suggestions or if you want to use scoped enums

enum class MapState {
  kOutOfDate,
  kUpToDate,
  kPreparingCommit,
};

[Mark P, 2017-04-13 21:21:46]

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_android.cc (right):

https://codereview.chromium.org/2812773002/diff/100001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:119: map_up_to_date_ = false;
On 2017/04/13 20:59:18, dcheng wrote:
> On 2017/04/13 20:54:05, Mark P wrote:
> > On 2017/04/13 20:35:37, dcheng wrote:
> > > Can we make this a tri-state enum? Calling UpdateFromAndroidClipboard() in
> > this
> > > state should be considered an error (since this would blow away the
> temporary
> > > state that we're trying to commit to the clipboard).
> > 
> > Do you mean
> > MAP_OUT_OF_DATE
> > MAP_FRESHER_THAN_JAVA
> > MAP_UP_TO_DATE
> > ?
> > 
> > And DCHECK if UpdateFromAndroidClipboard() is called from the middle state?
> > 
> 
> MAP_OUT_OF_DATE
> MAP_UP_TO_DATE
> MAP_PREPARING_COMMIT
> 
> would be my suggestions or if you want to use scoped enums
> 
> enum class MapState {
>   kOutOfDate,
>   kUpToDate,
>   kPreparingCommit,
> };

Done with the scoped enum.  Please take a look.

Thanks by the way for your suggestion of names.  "Preparing commit" is much
better than "fresher than java".

[dcheng, 2017-04-13 21:23:40]

clipboard lgtm

thanks for cleaning this up

[Mark P, 2017-04-13 21:25:20]

On 2017/04/13 21:23:40, dcheng wrote:
> clipboard lgtm

To be clear, did you review the whole changelist or just the 
ui/base/clipboard/...
parts?

I need to know what to ask other reviewers to look at, if anything?

thanks,
mark

[Mark P, 2017-04-13 21:25:40]

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

[commit-bot: I haz the power, 2017-04-13 21:26:37]

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

[dcheng, 2017-04-13 21:29:46]

sorry, i did look at the whole cl, so lgtm for the whole cl

[commit-bot: I haz the power, 2017-04-13 22:35:05]

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

[commit-bot: I haz the power, 2017-04-13 22:35:06]

Dry run: This issue passed the CQ dry run.

[Mark P, 2017-04-13 22:36:21]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2017-04-13 22:36:22]

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/2812773002/#ps140001
(title: "itri-state enum")

[commit-bot: I haz the power, 2017-04-13 22:36:56]

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

[commit-bot: I haz the power, 2017-04-13 23:07:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492122981661740,
"parent_rev": "94182b78878d55cb968e15312c66abfe21e3caf4", "commit_rev":
"71dfbd1f2228ae70efcea0656b3555ab539430fd"}

[commit-bot: I haz the power, 2017-04-13 23:08:34]

Description was changed from

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

TBR=sky,jif
(sky@ for the trivial changes to ui/base/test/...)
(jif@ for the refactoring / removal of logic from
	components/open_from_clipboard/...)

BUG=682446,707366
==========

to

==========
Refactor Clipboard Last Modified Time Storage

This changelist is the first step to solving two problems:
* clipboard last modified time isn't remembered from run to run.  It should be.
* likewise, clipboard suggestion suppression (which happens after clearing
browsing data) should also be preserved from run to run.

The key to solving both these problems is to store the clipboard last modified
time in prefs.  (Store a 0 to represent that the clipboard shouldn't be
suggested.)

To do this, we need a central place to own the last modified time value.  Upon
consultation with dcheng@, we decided this should be clipboard_android.cc.
Because Chrome on Android can be killed almost anytime, the best strategy
to have clipboard_android.cc to always have the correct value is to have
Clipboard.java "push" notification of the modification to clipboard_android.cc. 

After this changelist, I will submit a changelist that stores the
last_modified_time
information to prefs / reads it from prefs.

As a consequence of this decision, this change makes

* remove storage and retrieval of last modified time from Clipboard.java. 
clipboard_android.cc is the canonical source now.

* clipboard_recent_content_generic.{cc,h} loses
|last_modified_time_to_suppress_|, as
that suppression logic is unnecessary.  Instead, when needing to suppress a
suggestion, it tells clipboard_android to set its time to 0, which would cause
suppression.  Adds a function ClearClipboardLastModifiedTime() to clipboard.h
for
this.

* remove all the logic about hashing clipboard content from Clipboard.java. 
This is
not needed based on our usage of clipboard last modified time.  (This usage has
been
approved by privacy.)  This also means marking a related histogram as obsolete.

* Because Clipboard.java sends clipboard changed messages now to
clipboard_android, add support for sequence numbers to clipboard_android.

* Likewise, because clipboard_android is "pushed" messages about the clipboard
being updated, it knows when its local data is out of data.  It only "pulls"
clipboard
information from the Java side when someone asks it about the current state of
the
clipboard and it knows that it is out of date.

* Delete ClipboardTest.java.  The only thing this tested was updating the last
modified time in Clipboard.java, which doesn't exist anymore.

TBR=sky,jif
(sky@ for the trivial changes to ui/base/test/...)
(jif@ for the refactoring / removal of logic from
	components/open_from_clipboard/...)

BUG=682446,707366

Review-Url: https://codereview.chromium.org/2812773002
Cr-Commit-Position: refs/heads/master@{#464594}
Committed:
https://chromium.googlesource.com/chromium/src/+/71dfbd1f2228ae70efcea0656b35...
==========

[commit-bot: I haz the power, 2017-04-13 23:08:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/71dfbd1f2228ae70efcea0656b35...

[Shimi Zhang, 2017-04-14 01:58:41]

On 2017/04/13 23:08:36, commit-bot: I haz the power wrote:
> Committed patchset #8 (id:140001) as
>
https://chromium.googlesource.com/chromium/src/+/71dfbd1f2228ae70efcea0656b35...

Hi mpearson@, I added a test in ClipboardTest.java
(https://codereview.chromium.org/2812933003) to ensure the functionality of a
change to getHtmlText() in Clipboard.java. However, it seems git didn't resolve
the conflicts properly, so the whole ClipboardTest.java file got deleted, can we
have a closer look on how to bring that test back? Thanks.

[Mark P, 2017-04-14 04:01:48]

On 2017/04/14 01:58:41, Shimi Zhang wrote:
> On 2017/04/13 23:08:36, commit-bot: I haz the power wrote:
> > Committed patchset #8 (id:140001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/71dfbd1f2228ae70efcea0656b35...
> 
> Hi mpearson@, I added a test in ClipboardTest.java
> (https://codereview.chromium.org/2812933003) to ensure the functionality of a
> change to getHtmlText() in Clipboard.java. However, it seems git didn't
resolve
> the conflicts properly, so the whole ClipboardTest.java file got deleted, can
we
> have a closer look on how to bring that test back? Thanks.

Oh, that's a weird behavior for git.  I'm glad to see your functional changes
were submitted.

It shouldn't be hard to revive ClipboardTest.java and have it simply contain
your new test.  I'm willing to do it if you don't want to.  (I'd start with your
ClipboardTest.java, remove the the beforeClass(), afterClass(), and
testGetClipboardContentChangeTimeInMillis() functions, remove all the
now-unnecessary includes, and re-add it to ui/android/BUILD.gn.  I think that's
it.)

--mark

[Shimi Zhang, 2017-04-14 16:41:43]

On 2017/04/14 04:01:48, Mark P wrote:
> On 2017/04/14 01:58:41, Shimi Zhang wrote:
> > On 2017/04/13 23:08:36, commit-bot: I haz the power wrote:
> > > Committed patchset #8 (id:140001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/71dfbd1f2228ae70efcea0656b35...
> > 
> > Hi mpearson@, I added a test in ClipboardTest.java
> > (https://codereview.chromium.org/2812933003) to ensure the functionality of
a
> > change to getHtmlText() in Clipboard.java. However, it seems git didn't
> resolve
> > the conflicts properly, so the whole ClipboardTest.java file got deleted,
can
> we
> > have a closer look on how to bring that test back? Thanks.
> 
> Oh, that's a weird behavior for git.  I'm glad to see your functional changes
> were submitted.
> 
> It shouldn't be hard to revive ClipboardTest.java and have it simply contain
> your new test.  I'm willing to do it if you don't want to.  (I'd start with
your
> ClipboardTest.java, remove the the beforeClass(), afterClass(), and
> testGetClipboardContentChangeTimeInMillis() functions, remove all the
> now-unnecessary includes, and re-add it to ui/android/BUILD.gn.  I think
that's
> it.)
> 
> --mark

Sounds cool, I will send a cl soon, and have both you and my reviewer check it
again. Thanks!