Extract tabbed mode specific logic from the TabPersistenceStore.

Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Nothing to see here
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

BUG=606513
Committed: https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461
Cr-Commit-Position: refs/heads/master@{#414510}

[Ted C, 2016-08-24 00:48:31]

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

[Ted C, 2016-08-24 00:48:31]

PTAL

This "should" be mainly a reshuffling of tabbed mode stuff out of the base
persistent store that allows Custom Tabs just to be added w/o more complexity. 
Yolo

[Ted C, 2016-08-24 00:50:24]

Description was changed from

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration now uses a callback to kick of the additional tasks
    (should be functionally the same as using the serial executor
    as before).
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Both prefetching tasks are now using the THREAD_POOL_EXECUTOR.
    Now that we block triggering them on migration completing, it
    seems to be the same desired behavior as before.  From what I
    can decipher, we only used SERIAL for both prefetch to prevent
    them from happening in parallel with migration, but we don't
    care if they execute at the same time.

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

to

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration now uses a callback to kick of the additional tasks
    (should be functionally the same as using the serial executor
    as before).
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Both prefetching tasks are now using the THREAD_POOL_EXECUTOR.
    Now that we block triggering them on migration completing, it
    seems to be the same desired behavior as before.  From what I
    can decipher, we only used SERIAL for both prefetch to prevent
    them from happening in parallel with migration, but we don't
    care if they execute at the same time.
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

[agrieve, 2016-08-24 03:43:09]

Looks like a nice change overall!

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java:43:
*                             required, then this will be called synchronously.
Conditionally calling callbacks synchronously adds complexity, since now you
need to test both the case where it's called synchronously and the case where
it's not. It'd be better to just always call it async. It'd also be good to
document on which thread it'll be called on.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java:78:
void cleanupUnusedFiles(Callback<List<String>> filesToDelete);
This is a bit awkward. The policy does all of the migration logic, deletes it's
own thumbnails, but then delegates deleting files to this callback? Looking at
the code, I can see why you didn't want to change this all around in this CL,
but might be worth a TODO / mention that this could be improved.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:402:
mPersistencePolicy.setMergeInProgress(true);
Is this safe to change from a static to per-instance? I'd guess it was a static
because a merge involves multiple TabPersistentStores. Maybe instead it should
be setting it to true for both involved stores?

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:636:
* Deletes all files in the tab state directory.
nit: Can you elaborate this comment a bit to point out that it deletes more than
just its own state file in the case where multiple selectors share a directory?

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:641:
new AsyncTask<Void, Void, Void>() {
nit: no need for AsyncTask here.
AsyncTask.SERIAL_EXECUTOR.execute(new Runnable() { ... });

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:938:
Queue<File> pendingFiles = new LinkedList<>();
nit: you could avoid storing the list of files with an inner loop:

for each subdir:
  for each file:

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:946:
Pair<Integer, Boolean> tabStateInfo =
Looks like this is new logic compared to the old version. Seems like a faster
way to go, but is reading the state file also necessary if we're already looking
at the filenames?

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1172:
for (int i = 0; i < result.size(); i++) {
nit: for (String filename : result) { ... }

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1316:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
Given that all deletes are done on the serial executor, would doing this on the
thread pool executor cause a race condition if cleanUp() was called?

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1333:
return BaseStateDirectoryHolder.sDirectory;
I'm not sure this change in logic is safe. This no longer getsOrCreates, it now
just gets for all but the first call to it.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1348:
private boolean isStateFile(String fileName) {
It might be more correct to move this to the policy interface, since it's the
one the names the file (or make the prefix a final static on the interface, and
make this function static).

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1349:
if (fileName == null) return false;
fileName should never be null. Better to NPE than to check it.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:59:
private static final Object MIGRATION_LOCK = new Object();
nit: These should be named like sMigrationLock I think.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:101:
return getOrCreateStateDirectory();
The comment in the interface doesn't mention creating. Probably this is what we
want though since it's what was happening before. Maybe update the method in the
interface to be called "getOrCreateStateDirectory()?

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:110:
public String getStateToBeMergedFileName() {
Does this make sense in a CCT world, where there are not just pairs of
selectors? Would it make more sense to remove this from the interface and have
the caller responsible for figuring out the filename of the other selector by
calling getStateFileName() on it?

[Ted C, 2016-08-24 04:27:43]

Thanks for taking an early look!  I'll work on addressing these first thing
tomorrow.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java:43:
*                             required, then this will be called synchronously.
On 2016/08/24 03:43:08, agrieve wrote:
> Conditionally calling callbacks synchronously adds complexity, since now you
> need to test both the case where it's called synchronously and the case where
> it's not. It'd be better to just always call it async. It'd also be good to
> document on which thread it'll be called on.

Yeah, I was a bit conflicted here as well.  I'm happy to make it always async
for clarity.

My rationale was that no migration was the norm and forcing a delay for
kicking off the prefetch of tab state wasn't the best.  That might be
premature optimization though. 

If I wanted to keep some async-ness to it, I could use:
https://developer.android.com/reference/android/os/Handler.html#postAtFrontOf...

But that again causes some oddness as the normal developer that posts tasks
doesn't really expect that to be called (correct or not).

Rambling over, will covert to async and comment that the callback will be
triggered on the UI thread.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java:78:
void cleanupUnusedFiles(Callback<List<String>> filesToDelete);
On 2016/08/24 03:43:08, agrieve wrote:
> This is a bit awkward. The policy does all of the migration logic, deletes
it's
> own thumbnails, but then delegates deleting files to this callback? Looking at
> the code, I can see why you didn't want to change this all around in this CL,
> but might be worth a TODO / mention that this could be improved.

Will do.

I was definitely running into some cyclical dependencies that I wanted to avoid,
so I opted for this less than elegant solution.  Will add a TODO.  In general, I
want
to take a more broader review of this whole code area as I think it's gotten
pretty
ungainly over the years.

I also this think api could simple be collectUnusedState files (or something).
The fact that this gets oddly mushed with the thumbnail deletion, despite this
class
not being the generator of those feel soooo very strange.

I think the right approach there would be to make an additional observer method
for when files are deleted and if it sees a tab state file deleted it should
kick of the
thumbnail deletion outside the scope of this class.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:402:
mPersistencePolicy.setMergeInProgress(true);
On 2016/08/24 03:43:08, agrieve wrote:
> Is this safe to change from a static to per-instance? I'd guess it was a
static
> because a merge involves multiple TabPersistentStores. Maybe instead it should
> be setting it to true for both involved stores?

Merging is very specific to the tabbed mode behavior.  Ideally, I'll find a
solution that pushes it entirely into that tabbed mode class and out of here. 

I moved it to a static within the tabbed mode policy as it should only interact
with them.  I believe that achieves the same behavior as having it be set within
the two persistence stores as the policies will share the static boolean.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:636:
* Deletes all files in the tab state directory.
On 2016/08/24 03:43:08, agrieve wrote:
> nit: Can you elaborate this comment a bit to point out that it deletes more
than
> just its own state file in the case where multiple selectors share a
directory?

will do.  potentially this should be a static instead of an instance method, but
will
start with updating the documentation.  This deletes all the things!!!  All tab
states
and all tab selectors.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:641:
new AsyncTask<Void, Void, Void>() {
On 2016/08/24 03:43:09, agrieve wrote:
> nit: no need for AsyncTask here.
> AsyncTask.SERIAL_EXECUTOR.execute(new Runnable() { ... });

nifty!  will do

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:938:
Queue<File> pendingFiles = new LinkedList<>();
On 2016/08/24 03:43:08, agrieve wrote:
> nit: you could avoid storing the list of files with an inner loop:
> 
> for each subdir:
>   for each file:

true...this is overly generic to handle nested folders and that isn't something
we support.

will simplify this to be more like what you said

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:946:
Pair<Integer, Boolean> tabStateInfo =
On 2016/08/24 03:43:08, agrieve wrote:
> Looks like this is new logic compared to the old version. Seems like a faster
> way to go, but is reading the state file also necessary if we're already
looking
> at the filenames?

this will actually be slightly slower.  Just seems more correct to get the max
of
all the tab states and selectors.  this would handle the case if we wanted to
pull
in webapps as they don't have a selector associated with it and just a tab
state.

I was hoping to actually just read the tab sates instead of the selector
metadata
but it turns out it will attempt to regenerate a tab if it is in the metadata
but has
not state based on the URL in the metadata file.  So I had to add back the
metadata
handling as well.

This just "seems" more correct to me.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1172:
for (int i = 0; i < result.size(); i++) {
On 2016/08/24 03:43:08, agrieve wrote:
> nit: for (String filename : result) { ... }

unless it is an array, we should never use that format in java.  that requires
an
iterator object allocation unnecessarily where the for (int = ... does not.

Granted, that is on the strong assumption we are using lists with fast lookup,
but we pretty much uniformly use arraylists, which is ok.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1316:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2016/08/24 03:43:08, agrieve wrote:
> Given that all deletes are done on the serial executor, would doing this on
the
> thread pool executor cause a race condition if cleanUp() was called?

I described this in the change commit message, but I don't believe so.

It conditionally used the thread pool executor already if there was no
migration.  Now that
these don't start till after migration has fully completed, I think it negates
the need for that
to be the enforcer of ordering.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1333:
return BaseStateDirectoryHolder.sDirectory;
On 2016/08/24 03:43:08, agrieve wrote:
> I'm not sure this change in logic is safe. This no longer getsOrCreates, it
now
> just gets for all but the first call to it. 

I thought I looked through the usage and it looked ok, but you're right that
this is a change in behavior.  I'm honestly not sure the interaction between
this and BaseStateDirectoryHolder.sDirectory was ever that safe.  If it is
deleted outside the scope of this class, the static would never be reset and all
references would fail.

The safest thing would to be just get rid of the caching, but that would
probably cause StrictMode violations.  I'll take a look more tomorrow, but I
"think" this is probably not any worse than it was but now the naming isn't good
(I'd be tempted to drop Create from the name).

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1348:
private boolean isStateFile(String fileName) {
On 2016/08/24 03:43:08, agrieve wrote:
> It might be more correct to move this to the policy interface, since it's the
> one the names the file (or make the prefix a final static on the interface,
and
> make this function static).

Sounds reasonable.  will do

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1349:
if (fileName == null) return false;
On 2016/08/24 03:43:08, agrieve wrote:
> fileName should never be null. Better to NPE than to check it.

Will fix

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:59:
private static final Object MIGRATION_LOCK = new Object();
On 2016/08/24 03:43:09, agrieve wrote:
> nit: These should be named like sMigrationLock I think.

Well, well, well...you're right here.  I was thinking that final was the thing
that forced it to be uppercase, but it is actually the combination of public and
final.
http://source.android.com/source/code-style.html#follow-field-naming-conventions

Buuuut...I think the uppercase formatting is probably more in line with what we
do in chrome where we treat all static finals as these type of constants
regardless of scope.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:110:
public String getStateToBeMergedFileName() {
On 2016/08/24 03:43:09, agrieve wrote:
> Does this make sense in a CCT world, where there are not just pairs of
> selectors? Would it make more sense to remove this from the interface and have
> the caller responsible for figuring out the filename of the other selector by
> calling getStateFileName() on it?

My thinking that this would be null in CCT land.

I'm not happy with the split of merge logic across the two since there are
multiple
merge codepaths it gets a bit trickier to do correctly.  I'll take another look,
but this
seemed like the "least" terrible short term solution until I can figure out a
better plan
for merging.

[agrieve, 2016-08-24 13:20:50]

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1316:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > Given that all deletes are done on the serial executor, would doing this on
> the
> > thread pool executor cause a race condition if cleanUp() was called?
> 
> I described this in the change commit message, but I don't believe so.
> 
> It conditionally used the thread pool executor already if there was no
> migration.  Now that
> these don't start till after migration has fully completed, I think it negates
> the need for that
> to be the enforcer of ordering.

clearState() could also cause issues since that deletes async.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1333:
return BaseStateDirectoryHolder.sDirectory;
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > I'm not sure this change in logic is safe. This no longer getsOrCreates, it
> now
> > just gets for all but the first call to it. 
> 
> I thought I looked through the usage and it looked ok, but you're right that
> this is a change in behavior.  I'm honestly not sure the interaction between
> this and BaseStateDirectoryHolder.sDirectory was ever that safe.  If it is
> deleted outside the scope of this class, the static would never be reset and
all
> references would fail.
> 
> The safest thing would to be just get rid of the caching, but that would
> probably cause StrictMode violations.  I'll take a look more tomorrow, but I
> "think" this is probably not any worse than it was but now the naming isn't
good
> (I'd be tempted to drop Create from the name).

clearState() would mess it up I think. It now deletes the directory, and
subsequent calls will no longer bring it back.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:59:
private static final Object MIGRATION_LOCK = new Object();
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:09, agrieve wrote:
> > nit: These should be named like sMigrationLock I think.
> 
> Well, well, well...you're right here.  I was thinking that final was the thing
> that forced it to be uppercase, but it is actually the combination of public
and
> final.
>
http://source.android.com/source/code-style.html#follow-field-naming-conventions
> 
> Buuuut...I think the uppercase formatting is probably more in line with what
we
> do in chrome where we treat all static finals as these type of constants
> regardless of scope.

Hrm, yeah I don't actually know which way is correct. Just noticed they were
different from sMergeInProgress.

[Ted C, 2016-08-24 14:12:57]

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1316:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2016/08/24 13:20:50, agrieve wrote:
> On 2016/08/24 04:27:43, Ted C wrote:
> > On 2016/08/24 03:43:08, agrieve wrote:
> > > Given that all deletes are done on the serial executor, would doing this
on
> > the
> > > thread pool executor cause a race condition if cleanUp() was called?
> > 
> > I described this in the change commit message, but I don't believe so.
> > 
> > It conditionally used the thread pool executor already if there was no
> > migration.  Now that
> > these don't start till after migration has fully completed, I think it
negates
> > the need for that
> > to be the enforcer of ordering.
> 
> clearState() could also cause issues since that deletes async.

That is correct, but I still argue that I'm not introducing any behavioral
changes to what existed before.

This prefetchexecutor existed to enforce that the prefetch tasks happen after
the migration code.  Now that the migration code has a callback for completion,
we don't need the executor to be the one enforcing the ordering.

So in the common non-migration case, this would previously have the likelihood
of interacting badly.  I'm not saying this is good, but I don't think I made it
worse (potentially for the migration case, but I think clearState did not have
that in mind).

Instead of trying to fix clearState in this patch, I think it's better for
another followup that makes clearState block every interaction in this file,
clear pending queues for restore, etc...  Since it is only called in the case
where you're going through FRE but somehow have file data, I don't think it is
the most critical path to address (since I believe it has been broken in similar
ways for probably ever).

But really, we only want clearState to be supported during startup.  Maybe the
right thing is to ensure it always happens before calls to loadState().  I'll
have to dig in more and figure out a way to do this, but I still think it is out
of scope of this change.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1333:
return BaseStateDirectoryHolder.sDirectory;
On 2016/08/24 13:20:50, agrieve wrote:
> On 2016/08/24 04:27:43, Ted C wrote:
> > On 2016/08/24 03:43:08, agrieve wrote:
> > > I'm not sure this change in logic is safe. This no longer getsOrCreates,
it
> > now
> > > just gets for all but the first call to it. 
> > 
> > I thought I looked through the usage and it looked ok, but you're right that
> > this is a change in behavior.  I'm honestly not sure the interaction between
> > this and BaseStateDirectoryHolder.sDirectory was ever that safe.  If it is
> > deleted outside the scope of this class, the static would never be reset and
> all
> > references would fail.
> > 
> > The safest thing would to be just get rid of the caching, but that would
> > probably cause StrictMode violations.  I'll take a look more tomorrow, but I
> > "think" this is probably not any worse than it was but now the naming isn't
> good
> > (I'd be tempted to drop Create from the name).
> 
> clearState() would mess it up I think. It now deletes the directory, and
> subsequent calls will no longer bring it back.

Nice catch...that wasn't my intended behavior.  I was thinking that it was only
deleting the files underneath it, but now I see the documentation in FileUtils
that it will delete the directory itself.  Will fix that by just iterating over
the subfiles in the base directory and deleting them.

[Ted C, 2016-08-24 14:37:50]

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1316:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2016/08/24 14:12:57, Ted C wrote:
> On 2016/08/24 13:20:50, agrieve wrote:
> > On 2016/08/24 04:27:43, Ted C wrote:
> > > On 2016/08/24 03:43:08, agrieve wrote:
> > > > Given that all deletes are done on the serial executor, would doing this
> on
> > > the
> > > > thread pool executor cause a race condition if cleanUp() was called?
> > > 
> > > I described this in the change commit message, but I don't believe so.
> > > 
> > > It conditionally used the thread pool executor already if there was no
> > > migration.  Now that
> > > these don't start till after migration has fully completed, I think it
> negates
> > > the need for that
> > > to be the enforcer of ordering.
> > 
> > clearState() could also cause issues since that deletes async.
> 
> That is correct, but I still argue that I'm not introducing any behavioral
> changes to what existed before.
> 
> This prefetchexecutor existed to enforce that the prefetch tasks happen after
> the migration code.  Now that the migration code has a callback for
completion,
> we don't need the executor to be the one enforcing the ordering.
> 
> So in the common non-migration case, this would previously have the likelihood
> of interacting badly.  I'm not saying this is good, but I don't think I made
it
> worse (potentially for the migration case, but I think clearState did not have
> that in mind).
> 
> Instead of trying to fix clearState in this patch, I think it's better for
> another followup that makes clearState block every interaction in this file,
> clear pending queues for restore, etc...  Since it is only called in the case
> where you're going through FRE but somehow have file data, I don't think it is
> the most critical path to address (since I believe it has been broken in
similar
> ways for probably ever).
> 
> But really, we only want clearState to be supported during startup.  Maybe the
> right thing is to ensure it always happens before calls to loadState().  I'll
> have to dig in more and figure out a way to do this, but I still think it is
out
> of scope of this change.

I'm should also state that I'm fine bringing back the different executors based
on whether a migration happened, but I'm personally not convinced that is worth
it.  But if you do, then I don't believe strongly that it is a bad thing either.

[Ted C, 2016-08-24 17:10:45]

First round of revisions are up

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java:43:
*                             required, then this will be called synchronously.
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > Conditionally calling callbacks synchronously adds complexity, since now you
> > need to test both the case where it's called synchronously and the case
where
> > it's not. It'd be better to just always call it async. It'd also be good to
> > document on which thread it'll be called on.
> 
> Yeah, I was a bit conflicted here as well.  I'm happy to make it always async
> for clarity.
> 
> My rationale was that no migration was the norm and forcing a delay for
> kicking off the prefetch of tab state wasn't the best.  That might be
> premature optimization though. 
> 
> If I wanted to keep some async-ness to it, I could use:
>
https://developer.android.com/reference/android/os/Handler.html#postAtFrontOf...
> 
> But that again causes some oddness as the normal developer that posts tasks
> doesn't really expect that to be called (correct or not).
> 
> Rambling over, will covert to async and comment that the callback will be
> triggered on the UI thread.

Done.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java:78:
void cleanupUnusedFiles(Callback<List<String>> filesToDelete);
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > This is a bit awkward. The policy does all of the migration logic, deletes
> it's
> > own thumbnails, but then delegates deleting files to this callback? Looking
at
> > the code, I can see why you didn't want to change this all around in this
CL,
> > but might be worth a TODO / mention that this could be improved.
> 
> Will do.
> 
> I was definitely running into some cyclical dependencies that I wanted to
avoid,
> so I opted for this less than elegant solution.  Will add a TODO.  In general,
I
> want
> to take a more broader review of this whole code area as I think it's gotten
> pretty
> ungainly over the years.
> 
> I also this think api could simple be collectUnusedState files (or something).
> The fact that this gets oddly mushed with the thumbnail deletion, despite this
> class
> not being the generator of those feel soooo very strange.
> 
> I think the right approach there would be to make an additional observer
method
> for when files are deleted and if it sees a tab state file deleted it should
> kick of the
> thumbnail deletion outside the scope of this class.

Done.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:636:
* Deletes all files in the tab state directory.
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > nit: Can you elaborate this comment a bit to point out that it deletes more
> than
> > just its own state file in the case where multiple selectors share a
> directory?
> 
> will do.  potentially this should be a static instead of an instance method,
but
> will
> start with updating the documentation.  This deletes all the things!!!  All
tab
> states
> and all tab selectors.

Done.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:641:
new AsyncTask<Void, Void, Void>() {
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:09, agrieve wrote:
> > nit: no need for AsyncTask here.
> > AsyncTask.SERIAL_EXECUTOR.execute(new Runnable() { ... });
> 
> nifty!  will do

Done.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:938:
Queue<File> pendingFiles = new LinkedList<>();
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > nit: you could avoid storing the list of files with an inner loop:
> > 
> > for each subdir:
> >   for each file:
> 
> true...this is overly generic to handle nested folders and that isn't
something
> we support.
> 
> will simplify this to be more like what you said

Done.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1348:
private boolean isStateFile(String fileName) {
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > It might be more correct to move this to the policy interface, since it's
the
> > one the names the file (or make the prefix a final static on the interface,
> and
> > make this function static).
> 
> Sounds reasonable.  will do

Moved to the interface and made this a static.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1349:
if (fileName == null) return false;
On 2016/08/24 04:27:43, Ted C wrote:
> On 2016/08/24 03:43:08, agrieve wrote:
> > fileName should never be null. Better to NPE than to check it.
> 
> Will fix

Done.

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

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:59:
private static final Object MIGRATION_LOCK = new Object();
On 2016/08/24 13:20:50, agrieve wrote:
> On 2016/08/24 04:27:43, Ted C wrote:
> > On 2016/08/24 03:43:09, agrieve wrote:
> > > nit: These should be named like sMigrationLock I think.
> > 
> > Well, well, well...you're right here.  I was thinking that final was the
thing
> > that forced it to be uppercase, but it is actually the combination of public
> and
> > final.
> >
>
http://source.android.com/source/code-style.html#follow-field-naming-conventions
> > 
> > Buuuut...I think the uppercase formatting is probably more in line with what
> we
> > do in chrome where we treat all static finals as these type of constants
> > regardless of scope.
> 
> Hrm, yeah I don't actually know which way is correct. Just noticed they were
> different from sMergeInProgress.

The capital case is more common in Chrome by a mile, so I went with that.

https://codereview.chromium.org/2277603002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java:101:
return getOrCreateStateDirectory();
On 2016/08/24 03:43:09, agrieve wrote:
> The comment in the interface doesn't mention creating. Probably this is what
we
> want though since it's what was happening before. Maybe update the method in
the
> interface to be called "getOrCreateStateDirectory()?

Done.

[Theresa, 2016-08-24 20:16:58]

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

https://codereview.chromium.org/2277603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:701:
// files aren't created before the state files are cleared.
We're not blocking on the clean up task anywhere, right? Is it possible to end
up interleaving the clean up task w/ other operations (e.g. creating tabs &
writing out the metadata file)

e.g. (not sure if this is actually feasible... but maybe?)
1) FRE is launched, ChromeTabbedActivity:970 calls #clearState()
2) Kick off clean up task running asynchronously on serial executor
3) TabPersistentStore calls #onStateLoaded() on observers
4) TabModelSelectorImpl marks the tab state as initialized
5) FRE finishes and user starts creating tabs
6) User goes to Android recents, we write the metadata out synchronously in
TabPersistentStore#saveState()
7) The async cleanup task finally gets around to deleting the metadata file

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

https://codereview.chromium.org/2277603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:639:
File baseStateDir = getOrCreateBaseStateDirectory();
I think this can be simplified to FileUtils.recurisvelyDeleteFile(baseStateDir)
since that method will check if the current file is a directory and recursively
call itself on files inside the directory.

[Ted C, 2016-08-24 20:30:34]

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

https://codereview.chromium.org/2277603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:701:
// files aren't created before the state files are cleared.
On 2016/08/24 20:16:58, Theresa Wellington wrote:
> We're not blocking on the clean up task anywhere, right? Is it possible to end
> up interleaving the clean up task w/ other operations (e.g. creating tabs &
> writing out the metadata file)
> 
> e.g. (not sure if this is actually feasible... but maybe?)
> 1) FRE is launched, ChromeTabbedActivity:970 calls #clearState()
> 2) Kick off clean up task running asynchronously on serial executor
> 3) TabPersistentStore calls #onStateLoaded() on observers
> 4) TabModelSelectorImpl marks the tab state as initialized
> 5) FRE finishes and user starts creating tabs
> 6) User goes to Android recents, we write the metadata out synchronously in
> TabPersistentStore#saveState()
> 7) The async cleanup task finally gets around to deleting the metadata file

Totally possible, but I don't think this made it any worse.  It was deleting the
files asyncly before and it still is (granted in one big block now).

I think clearState is broken and needs to be fixed, but I think it was broken
before hand and isn't more broken now.

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

https://codereview.chromium.org/2277603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:639:
File baseStateDir = getOrCreateBaseStateDirectory();
On 2016/08/24 20:16:58, Theresa Wellington wrote:
> I think this can be simplified to
FileUtils.recurisvelyDeleteFile(baseStateDir)
> since that method will check if the current file is a directory and
recursively
> call itself on files inside the directory.

The problem with recursivelyDeleteFile is that it deletes the directory as well.
 I want to leave the directories alone.  I ended up changing this more in the
next patch where I don't use the recursivelyDeleteFile at all since I don't want
any directories to be deleted just the files they contain.

[gone, 2016-08-25 00:44:22]

lgtm

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

https://chromiumcodereview.appspot.com/2277603002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:66:
* @param selectorIndex The index this selector represents in the list of
selectors.
This arg seems to be dead now.

https://chromiumcodereview.appspot.com/2277603002/diff/40001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java
(right):

https://chromiumcodereview.appspot.com/2277603002/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java:74:
private TabPersistentStore buildTabPersistenceStore(
I know you want it to be a thing, but it's the TabPersistentStore and it should
be buildTabPersistentStore().

[Ted C, 2016-08-25 02:27:51]

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

[commit-bot: I haz the power, 2016-08-25 02:28:33]

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

[Ted C, 2016-08-25 02:29:21]

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

https://codereview.chromium.org/2277603002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:66:
* @param selectorIndex The index this selector represents in the list of
selectors.
On 2016/08/25 00:44:22, dfalcantara wrote:
> This arg seems to be dead now.

Removed

https://codereview.chromium.org/2277603002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java
(right):

https://codereview.chromium.org/2277603002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java:74:
private TabPersistentStore buildTabPersistenceStore(
On 2016/08/25 00:44:22, dfalcantara wrote:
> I know you want it to be a thing, but it's the TabPersistentStore and it
should
> be buildTabPersistentStore().

Sigh...you'll always be a persistence store to me!

[commit-bot: I haz the power, 2016-08-25 03:43:27]

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

[commit-bot: I haz the power, 2016-08-25 03:43:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Theresa, 2016-08-25 05:33:21]

Just 1 more comment + test failures

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

https://codereview.chromium.org/2277603002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:641:
: "Only directories should exist below the base state directory";
In the olden days we used to store files in the base state directory instead of
sub-directories. Instead of asserting false should we just remove the files?

[Ted C, 2016-08-25 14:01:47]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-08-25 14:01:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2277603002/#ps100001
(title: "Fix migration task initialization")

[commit-bot: I haz the power, 2016-08-25 14:02:17]

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

[Ted C, 2016-08-25 14:08:02]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-08-25 14:08:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2277603002/#ps120001
(title: "Fix file deletion")

[commit-bot: I haz the power, 2016-08-25 14:08:23]

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

[Ted C, 2016-08-25 14:16:03]

And to fix the test failures, I needed to undo my changes to how I queued up
callbacks from migration.

The issue is that loadState calls waitForMigrationToFinish, which calls
sMigrationTask.get().  AsyncTask#get by necessity waits only for Result from
doInBackground and does not synchronously run the onPostExecute.

In my earlier patches, I was only initializing the prefetch tasks in
onPostExecute and that was being called after loadState finished (thus load
state did nothing).  I switched back to the previous model of initializing the
prefetch tasks and queuing them on the serial executor.

This seems to just be unraveling a thread that makes me really not like the
requirements that all this loading be done synchronously.  loadState should take
a callback and notify you when it is done.  It shouldn't need to do any stream
reading in place.  But that has huge implications on the startup path in Chrome
that will just fall over in certain places as we expect tabs to be initialized.

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

https://codereview.chromium.org/2277603002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:641:
: "Only directories should exist below the base state directory";
On 2016/08/25 05:33:21, Theresa Wellington wrote:
> In the olden days we used to store files in the base state directory instead
of
> sub-directories. Instead of asserting false should we just remove the files?

Done.

[Theresa, 2016-08-25 14:45:00]

lgtm

[commit-bot: I haz the power, 2016-08-25 15:09:17]

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

[commit-bot: I haz the power, 2016-08-25 15:09:18]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ted C, 2016-08-25 15:36:21]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2016-08-25 15:36:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2277603002/#ps140001
(title: "Fix findbugs")

[commit-bot: I haz the power, 2016-08-25 15:36:41]

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

[Ted C, 2016-08-25 15:42:18]

Description was changed from

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration now uses a callback to kick of the additional tasks
    (should be functionally the same as using the serial executor
    as before).
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Both prefetching tasks are now using the THREAD_POOL_EXECUTOR.
    Now that we block triggering them on migration completing, it
    seems to be the same desired behavior as before.  From what I
    can decipher, we only used SERIAL for both prefetch to prevent
    them from happening in parallel with migration, but we don't
    care if they execute at the same time.
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

to

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Both prefetching tasks are now using the THREAD_POOL_EXECUTOR.
    Now that we block triggering them on migration completing, it
    seems to be the same desired behavior as before.  From what I
    can decipher, we only used SERIAL for both prefetch to prevent
    them from happening in parallel with migration, but we don't
    care if they execute at the same time.
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

[Ted C, 2016-08-25 15:42:46]

Description was changed from

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Both prefetching tasks are now using the THREAD_POOL_EXECUTOR.
    Now that we block triggering them on migration completing, it
    seems to be the same desired behavior as before.  From what I
    can decipher, we only used SERIAL for both prefetch to prevent
    them from happening in parallel with migration, but we don't
    care if they execute at the same time.
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

to

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Nothing to see here
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

[Yusuf, 2016-08-25 16:18:25]

yusufo@chromium.org changed reviewers:
+ yusufo@chromium.org

[commit-bot: I haz the power, 2016-08-25 17:59:23]

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

[commit-bot: I haz the power, 2016-08-25 17:59:24]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ted C, 2016-08-25 18:23:40]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2016-08-25 18:25:00]

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-25 19:36:56]

Description was changed from

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Nothing to see here
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

to

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Nothing to see here
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

[commit-bot: I haz the power, 2016-08-25 19:37:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-25 19:40:20]

Description was changed from

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Nothing to see here
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

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

to

==========
Extract tabbed mode specific logic from the TabPersistenceStore.

This adds the concept of a persistence policy that handles
the type specific logic of the various tab models interactions.
This paves the way for CCT tab persisting.

Should mainly be a reshuffling of files with minimal logic changes.

Logic changes:
1.) Migration remains exactly as it was...sadly.
2.) Calculating the max tab id now iterates through all files
    under the root tabs dir (looks at individual tab states and
    metadata files to calculate the max ID).
3.) Nothing to see here
4.) Clear all now just recursively deletes the files under the
    base state directory.  That seems easier now that we migrated
    the tab state to a distinct subdirectory.

BUG=606513

Committed: https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461
Cr-Commit-Position: refs/heads/master@{#414510}
==========

[commit-bot: I haz the power, 2016-08-25 19:40:21]

Patchset 8 (id:??) landed as
https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461
Cr-Commit-Position: refs/heads/master@{#414510}