Add tab persistence support for CustomTabs.

Add tab persistence support for CustomTabs.

BUG=606513
Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Committed: https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43
Cr-Original-Commit-Position: refs/heads/master@{#415786}
Cr-Commit-Position: refs/heads/master@{#416015}

[Ted C, 2016-08-30 17:17:16]

tedchoc@chromium.org changed reviewers:
+ dfalcantara@chromium.org, twellington@chromium.org, yusufo@chromium.org

[Ted C, 2016-08-30 17:17:17]

+dfalcantara, twellington - tab saving logic
+yusufo - cct integration and stuff and things

[Theresa, 2016-08-30 21:45:23]

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:245:
CustomTabActivity activity , Set<Integer> tabIds) {
nit: remove space before comma

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:276:
SparseArray<AsyncTabParams> asyncTabParams =
AsyncTabParamsManager.getAsyncTabParams();
If a tab is in getAsyncTabParams(), it's moving out of a custom tab (or regular
tabbed activity) to a tabbed activity, so we probably don't need to keep the
custom tabs tab state file for it.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:325:
System.currentTimeMillis(), metadataFiles);
Right now we're reading through the metadata files twice to determine which tabs
can be removed (here and on line 368). If we get mDeletableMetadataFiles first,
can we use that info above (lines 307 - 321) to determine which tabs shouldn't
be added to mAllReferencedTabIds so they can be cleaned up in block 347-354?

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:235:
public void initializeForTesting(TabModel normalModel, TabModel incognitoModel)
{
Does this need the @VisibleForTesting annotation?

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java:90:
public void testDeletableMetadataSelection() {
Can these tests have the @Feature({"TabPersistentStore"}) annotation so that
it's easy to run all the TabPersisten[ce|t] tests?

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java:124:
filesToTest = new ArrayList<>();
Either this line or 121 can be removed.

[Yusuf, 2016-08-30 23:19:43]

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:64:
private static final Object DIR_CREATION_LOCK = new Object();
space here

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:221:
// Copied from Long#compare, which is only available in API 19+
I know this is extremely short, but should we make this an APICompatibilityUtils
thing so there is a far chance for deleting it?

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:244:
private static void getAllTabIdsForActivity(
Dont we have this for a generic ChromeActivity in TabModelImplSelector or
something? Maybe we should. Then we can still do the instanceof check below for
CustomTabActivity and continue if not, but would just cast to CHromeActivity and
send it over. 

dfalcantara@ you might actually have this somewhere.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java:91:
long currentTime = CustomTabTabPersistencePolicy.STATE_EXPIRY_THRESHOLD * 10;
Can we have this broken into like 4 separate tests with a shared init?

https://codereview.chromium.org/2296833002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
(right):

https://codereview.chromium.org/2296833002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:222:
boolean needsMigration = mPersistencePolicy.performInitialization(
needsInitialization?

[Ted C, 2016-08-30 23:52:28]

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:64:
private static final Object DIR_CREATION_LOCK = new Object();
On 2016/08/30 23:19:43, Yusuf wrote:
> space here

Done.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:221:
// Copied from Long#compare, which is only available in API 19+
On 2016/08/30 23:19:43, Yusuf wrote:
> I know this is extremely short, but should we make this an
APICompatibilityUtils
> thing so there is a far chance for deleting it?

Done.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:244:
private static void getAllTabIdsForActivity(
On 2016/08/30 23:19:43, Yusuf wrote:
> Dont we have this for a generic ChromeActivity in TabModelImplSelector or
> something? Maybe we should. Then we can still do the instanceof check below
for
> CustomTabActivity and continue if not, but would just cast to CHromeActivity
and
> send it over. 
> 
> dfalcantara@ you might actually have this somewhere.

Happy to be wrong, but I didn't see anything.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:245:
CustomTabActivity activity , Set<Integer> tabIds) {
On 2016/08/30 21:45:23, Theresa Wellington wrote:
> nit: remove space before comma

Done.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:276:
SparseArray<AsyncTabParams> asyncTabParams =
AsyncTabParamsManager.getAsyncTabParams();
On 2016/08/30 21:45:23, Theresa Wellington wrote:
> If a tab is in getAsyncTabParams(), it's moving out of a custom tab (or
regular
> tabbed activity) to a tabbed activity, so we probably don't need to keep the
> custom tabs tab state file for it.

Good point.  Removing this (and the corresponding test bit)

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:325:
System.currentTimeMillis(), metadataFiles);
On 2016/08/30 21:45:23, Theresa Wellington wrote:
> Right now we're reading through the metadata files twice to determine which
tabs
> can be removed (here and on line 368). If we get mDeletableMetadataFiles
first,
> can we use that info above (lines 307 - 321) to determine which tabs shouldn't
> be added to mAllReferencedTabIds so they can be cleaned up in block 347-354?

If I follow, I think I did it this way to be overly cautious.

I ignore any files earmarked for deletion if they belong to a live task ID,
which needs to be done on the UI thread.

One thing I see I can do is this:
mAllTabIds.removeAll(mAllReferencedTabIds); can be done in this call
and only a single list saved for UI thread processing.

And to note, I'm not reading the files more than once, but I am iterating over
them.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:235:
public void initializeForTesting(TabModel normalModel, TabModel incognitoModel)
{
On 2016/08/30 21:45:23, Theresa Wellington wrote:
> Does this need the @VisibleForTesting annotation?

Ah yes, or proguard will remove it and the tests will fail.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java:90:
public void testDeletableMetadataSelection() {
On 2016/08/30 21:45:23, Theresa Wellington wrote:
> Can these tests have the @Feature({"TabPersistentStore"}) annotation so that
> it's easy to run all the TabPersisten[ce|t] tests?

Done.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java:91:
long currentTime = CustomTabTabPersistencePolicy.STATE_EXPIRY_THRESHOLD * 10;
On 2016/08/30 23:19:43, Yusuf wrote:
> Can we have this broken into like 4 separate tests with a shared init?

Done.

https://codereview.chromium.org/2296833002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java:124:
filesToTest = new ArrayList<>();
On 2016/08/30 21:45:23, Theresa Wellington wrote:
> Either this line or 121 can be removed.

Done.

https://codereview.chromium.org/2296833002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
(right):

https://codereview.chromium.org/2296833002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:222:
boolean needsMigration = mPersistencePolicy.performInitialization(
On 2016/08/30 23:19:43, Yusuf wrote:
> needsInitialization?

Done.

[Yusuf, 2016-08-31 06:29:00]

lgtm

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2296833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:244:
private static void getAllTabIdsForActivity(
On 2016/08/30 23:52:27, Ted C wrote:
> On 2016/08/30 23:19:43, Yusuf wrote:
> > Dont we have this for a generic ChromeActivity in TabModelImplSelector or
> > something? Maybe we should. Then we can still do the instanceof check below
> for
> > CustomTabActivity and continue if not, but would just cast to CHromeActivity
> and
> > send it over. 
> > 
> > dfalcantara@ you might actually have this somewhere.
> 
> Happy to be wrong, but I didn't see anything.

Yeah looks line the one I remember is on IncognitoNotificationCenter and it is
not viable to be made that generic.

[gone, 2016-08-31 17:59:17]

lgtm

Some of these comments are probably irrelevant now :/

https://chromiumcodereview.appspot.com/2296833002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://chromiumcodereview.appspot.com/2296833002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:234:
*/
@VisibleForTesting?

https://chromiumcodereview.appspot.com/2296833002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://chromiumcodereview.appspot.com/2296833002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:52:
@VisibleForTesting
You only need this annotation to prevent ProGuard from stripping it out.  Not
indicating it seems fine here.

https://chromiumcodereview.appspot.com/2296833002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:218:
// Copied from Long#compare, which is only available in API 19+
This keeps coming up... can we just add a function to ApiCompatibilityUtils?

https://chromiumcodereview.appspot.com/2296833002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://chromiumcodereview.appspot.com/2296833002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:138:
if (metadataFile.exists()) {
Flip it so if (mShouldRestore) is on top?

[Theresa, 2016-08-31 18:00:01]

lgtm

[Ted C, 2016-08-31 18:37:53]

https://codereview.chromium.org/2296833002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2296833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:52:
@VisibleForTesting
On 2016/08/31 17:59:16, dfalcantara wrote:
> You only need this annotation to prevent ProGuard from stripping it out.  Not
> indicating it seems fine here.

Done.

https://codereview.chromium.org/2296833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:218:
// Copied from Long#compare, which is only available in API 19+
On 2016/08/31 17:59:16, dfalcantara wrote:
> This keeps coming up... can we just add a function to ApiCompatibilityUtils?

Done.

https://codereview.chromium.org/2296833002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2296833002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java:138:
if (metadataFile.exists()) {
On 2016/08/31 17:59:16, dfalcantara wrote:
> Flip it so if (mShouldRestore) is on top?

Done.

[Ted C, 2016-08-31 18:38:43]

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

[Ted C, 2016-08-31 18:38:44]

+nyquist for base/ OWNERS

[nyquist, 2016-08-31 19:54:00]

base/android lgtm

[Ted C, 2016-08-31 20:19:27]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-08-31 20:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, yusufo@chromium.org, dfalcantara@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2296833002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-31 20:20:11]

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, 2016-08-31 22:35:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-31 22:37:08]

Description was changed from

==========
Add tab persistence support for CustomTabs.

BUG=606513
==========

to

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}
==========

[commit-bot: I haz the power, 2016-08-31 22:37:09]

Patchset 6 (id:??) landed as
https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}

[yhirano, 2016-09-01 04:06:03]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2292083004/ by yhirano@chromium.org.

The reason for reverting is: The added tests are failing.
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg....

[Ted C, 2016-09-01 18:15:25]

Description was changed from

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}
==========

to

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}
==========

[Ted C, 2016-09-01 18:16:19]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-09-01 18:16:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, yusufo@chromium.org, dfalcantara@chromium.org,
nyquist@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2296833002/#ps120001 (title: "Fix test")

[commit-bot: I haz the power, 2016-09-01 18:16:37]

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, 2016-09-01 19:23:42]

Description was changed from

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}
==========

to

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}
==========

[commit-bot: I haz the power, 2016-09-01 19:23:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-01 19:26:21]

Description was changed from

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Cr-Commit-Position: refs/heads/master@{#415786}
==========

to

==========
Add tab persistence support for CustomTabs.

BUG=606513

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Committed: https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43
Cr-Original-Commit-Position: refs/heads/master@{#415786}
Cr-Commit-Position: refs/heads/master@{#416015}
==========

[commit-bot: I haz the power, 2016-09-01 19:26:22]

Patchset 7 (id:??) landed as
https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43
Cr-Commit-Position: refs/heads/master@{#416015}