Migrate TabPersistentStore to one shared directory for multi-instance

Migrate TabPersistentStore to one shared directory for multi-instance

Rather than using a sub-directory per ChromeTabbedActivity instance,
use one shared director for all instances.

BUG=622143
Committed: https://crrev.com/b79781bcb77c31e9abbf68f8f5316856b2f0eaa4
Cr-Commit-Position: refs/heads/master@{#403229}

[Theresa, 2016-06-22 20:12:08]

twellington@chromium.org changed reviewers:
+ wnwen@chromium.org

[Theresa, 2016-06-22 20:12:09]

ptal

[Peter Wen, 2016-06-23 14:42:06]

How does the Feature("TPS") work? I've always just run tests with filter based
on test class name. :o

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
(right):

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:192:
* @return The list of window indexes that were processed.
nit: remove

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

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:149:
@VisibleForTesting
Please replace with // Not final for tests.

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

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:236:
public static File getOrCreateStateDirectory() {
Now that this is independent from the tab model selector index, we can finally
combine this call and getStateDirectory so that static callers also get the
benefit of caching. Yay!

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:829:
File[] folders = BaseStateDirectoryHolder.sDirectory.listFiles();
Since this is only called in loadState, which happens after migration is done,
rather than reading the various folders, this will need to read the state
directory (getStateDirectory) and the new saved state files for all the possible
selectors.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:989:
if (mCleanupTask != null) mCleanupTask.cancel(true);
This check can be moved into cleanUpPersistentData.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1038:
}.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
Now that we may end up deleting a lot of state files at once, do you think this
could use thread pool instead (or getPrefetchExecutor)?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1061:
if (!mDeleteAllFiles) getTabsFromOtherStateFiles(otherTabIds);
This needs to happen in doInBackground, as it reads disk. onPostExecute runs on
the UiThread after doInBackground.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1084:
if (i == mSelectorIndex) continue;
Seems avoiding re-reading the current metadata file is not that much more
complexity. Assuming users keep the vast majority of their tabs in their primary
instance, this makes a lot of sense. Thanks. :)

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1150:
if (FeatureUtilities.isDocumentMode(ContextUtils.getApplicationContext())) {
Do you know when this migration by the assassin occurs? Is it possible that by
the time TPS is created this is never true anymore?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1180:
}.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
Good catch! This has to be done before the prewarming tasks.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1187:
*/
Add @WorkerThread

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1219:
*/
Add @WorkerThread

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1238:
for (int i = 1; i < numDirs; i++) {
nit: combine these two for-loops and have the comments inline?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java
(right):

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:36:
ContextUtils.initApplicationContextForTests(mAppContext);
Thanks for using the new ContextUtils correctly!

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:44:
FeatureUtilities.setDocumentModeEnabled(false);
Seems this requires native to be loaded? Might be easier to rely on prefs. Since
the current default is opted out of document mode if no prefs is set, shouldn't
need to do anything (see DocumentModeAssassin#isOptedOutOfDocumentMode).

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:53:
ApplicationData.clearAppData(mAppContext);
Move to setUp?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:122:
ApplicationData.clearAppData(mAppContext);
Move to tearDown?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:141:
File otherFile = new File(stateDirs[1], "other.file");
Do we delete the other folder if there's still a file there?

[Theresa, 2016-06-23 21:49:36]

Features used to be run with -A Feature=FeatureName ... I can't get it to work
right now, but I'm optimistic that I'll figure it out and the annotation will be
useful in the future.
https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins...


Made changes from your comments and also added some code to deal with duplicate
tab# files that can result from moving tabs between windows.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
(right):

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:192:
* @return The list of window indexes that were processed.
On 2016/06/23 14:42:05, Peter Wen wrote:
> nit: remove

Done.

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

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:149:
@VisibleForTesting
On 2016/06/23 14:42:05, Peter Wen wrote:
> Please replace with // Not final for tests.

Done.

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

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:236:
public static File getOrCreateStateDirectory() {
On 2016/06/23 14:42:06, Peter Wen wrote:
> Now that this is independent from the tab model selector index, we can finally
> combine this call and getStateDirectory so that static callers also get the
> benefit of caching. Yay!

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:829:
File[] folders = BaseStateDirectoryHolder.sDirectory.listFiles();
On 2016/06/23 14:42:05, Peter Wen wrote:
> Since this is only called in loadState, which happens after migration is done,
> rather than reading the various folders, this will need to read the state
> directory (getStateDirectory) and the new saved state files for all the
possible
> selectors.

Right, sorry I missed this the first time around! I rewrote this method.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:989:
if (mCleanupTask != null) mCleanupTask.cancel(true);
On 2016/06/23 14:42:06, Peter Wen wrote:
> This check can be moved into cleanUpPersistentData.

Done.

I moved this check to cleanUpPersistentData but left the other cancel() calls
alone. I also want to cancel in these two scenarios:
1. Before trying to clear all state since that method deletes the state files
serially before calling cleanUpPersistentData
2. At the beginning of loadState(). If a second Chrome instance is created, I'm
concerned that the tab model for that instance could change without getting
written out to disk while clean up is running in the background on the thread
pool.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1038:
}.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
On 2016/06/23 14:42:05, Peter Wen wrote:
> Now that we may end up deleting a lot of state files at once, do you think
this
> could use thread pool instead (or getPrefetchExecutor)?

I think we need to keep the deletion serialized. The tab_state files definitely
need to be deleted serially so that any new writes occur after the deletion. I
think tab# file deletion needs to be serialized too in the clearState() case.
I'm not sure how realistic this scenario is, but I think we could run into
something like this: clear the state and delete files in the thread pool -
there's a lot of state, so this takes a while. While that's running, a new tab
is created with an id matching a file that hasn't been deleted yet. I think for
this to occur, the max_id pref would have to get cleared without the tab# files
getting deleted.... so maybe it's an invalid concern?

I don't anticipate that the change in directory structure will result in a
significant increase in the # of files we're deleting at once. It should only
affect people who used multi-instance and killed the second instance of Chrome
before tab closures were committed to disk.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1061:
if (!mDeleteAllFiles) getTabsFromOtherStateFiles(otherTabIds);
On 2016/06/23 14:42:05, Peter Wen wrote:
> This needs to happen in doInBackground, as it reads disk. onPostExecute runs
on
> the UiThread after doInBackground.

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1084:
if (i == mSelectorIndex) continue;
On 2016/06/23 14:42:05, Peter Wen wrote:
> Seems avoiding re-reading the current metadata file is not that much more
> complexity. Assuming users keep the vast majority of their tabs in their
primary
> instance, this makes a lot of sense. Thanks. :)

I decided to pull tabs from the current model rather than the state file for the
current model in case the set of tabs changes while this is happening in the
background. Do you think that's actually possible?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1150:
if (FeatureUtilities.isDocumentMode(ContextUtils.getApplicationContext())) {
On 2016/06/23 14:42:06, Peter Wen wrote:
> Do you know when this migration by the assassin occurs? Is it possible that by
> the time TPS is created this is never true anymore?

Just discussed with Dan. If a migration from document mode is required, we block
launching Chrome until the UpgradeActivity is done, so no, this shouldn't be
possible. Removed the check.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1187:
*/
On 2016/06/23 14:42:05, Peter Wen wrote:
> Add @WorkerThread

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1219:
*/
On 2016/06/23 14:42:05, Peter Wen wrote:
> Add @WorkerThread

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1238:
for (int i = 1; i < numDirs; i++) {
On 2016/06/23 14:42:05, Peter Wen wrote:
> nit: combine these two for-loops and have the comments inline?

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java
(right):

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:44:
FeatureUtilities.setDocumentModeEnabled(false);
On 2016/06/23 14:42:06, Peter Wen wrote:
> Seems this requires native to be loaded? Might be easier to rely on prefs.
Since
> the current default is opted out of document mode if no prefs is set,
shouldn't
> need to do anything (see DocumentModeAssassin#isOptedOutOfDocumentMode).

Dan says that no bots run document mode anymore, so I removed this line.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:53:
ApplicationData.clearAppData(mAppContext);
On 2016/06/23 14:42:06, Peter Wen wrote:
> Move to setUp?

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:122:
ApplicationData.clearAppData(mAppContext);
On 2016/06/23 14:42:06, Peter Wen wrote:
> Move to tearDown?

Done.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:141:
File otherFile = new File(stateDirs[1], "other.file");
On 2016/06/23 14:42:06, Peter Wen wrote:
> Do we delete the other folder if there's still a file there?

We try, but the deletion fails because you can't delete directories that still
have a file in them. I chose not to explicitly check that the directory still
existed because checking that the file still exists covers that, but I can add a
line asserting that the directory still exists if that's more clear.

[Peter Wen, 2016-06-27 16:23:47]

Thanks for waiting.

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

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:989:
if (mCleanupTask != null) mCleanupTask.cancel(true);
On 2016/06/23 21:49:35, Theresa Wellington wrote:
> On 2016/06/23 14:42:06, Peter Wen wrote:
> > This check can be moved into cleanUpPersistentData.
> 
> Done.
> 
> I moved this check to cleanUpPersistentData but left the other cancel() calls
> alone. I also want to cancel in these two scenarios:
> 1. Before trying to clear all state since that method deletes the state files
> serially before calling cleanUpPersistentData
> 2. At the beginning of loadState(). If a second Chrome instance is created,
I'm
> concerned that the tab model for that instance could change without getting
> written out to disk while clean up is running in the background on the thread
> pool.

Makes sense, now that it's a static task it's quite clear. Not sure if we'll
have synchronization issues though, but since cancel and assignment are both
idempotent, should be okay.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1038:
}.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
On 2016/06/23 21:49:35, Theresa Wellington wrote:
> On 2016/06/23 14:42:05, Peter Wen wrote:
> > Now that we may end up deleting a lot of state files at once, do you think
> this
> > could use thread pool instead (or getPrefetchExecutor)?
> 
> I think we need to keep the deletion serialized. The tab_state files
definitely
> need to be deleted serially so that any new writes occur after the deletion. I
> think tab# file deletion needs to be serialized too in the clearState() case.
> I'm not sure how realistic this scenario is, but I think we could run into
> something like this: clear the state and delete files in the thread pool -
> there's a lot of state, so this takes a while. While that's running, a new tab
> is created with an id matching a file that hasn't been deleted yet. I think
for
> this to occur, the max_id pref would have to get cleared without the tab#
files
> getting deleted.... so maybe it's an invalid concern?
> 
> I don't anticipate that the change in directory structure will result in a
> significant increase in the # of files we're deleting at once. It should only
> affect people who used multi-instance and killed the second instance of Chrome
> before tab closures were committed to disk.

Yeah, if we lose max_id then we have bigger problems, but you are right, we
won't change the number of files to delete significantly. Later we may want to
separate serial deletion for tab list files but thread pool deletion for tab
state files. Since file deletion is I/O bound, having a lot of it blocking the
one serial thread isn't ideal, but no rush as it's not much worse than we have
now.

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1084:
if (i == mSelectorIndex) continue;
On 2016/06/23 21:49:35, Theresa Wellington wrote:
> On 2016/06/23 14:42:05, Peter Wen wrote:
> > Seems avoiding re-reading the current metadata file is not that much more
> > complexity. Assuming users keep the vast majority of their tabs in their
> primary
> > instance, this makes a lot of sense. Thanks. :)
> 
> I decided to pull tabs from the current model rather than the state file for
the
> current model in case the set of tabs changes while this is happening in the
> background. Do you think that's actually possible?

Yes, the tab list file for the current selector is written out regularly, but it
can be out of date, so it's better this way. It may be possible for the list
file for the instance in the other window to be out of date too. What do you
think?

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java
(right):

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:141:
File otherFile = new File(stateDirs[1], "other.file");
On 2016/06/23 21:49:36, Theresa Wellington wrote:
> On 2016/06/23 14:42:06, Peter Wen wrote:
> > Do we delete the other folder if there's still a file there?
> 
> We try, but the deletion fails because you can't delete directories that still
> have a file in them. I chose not to explicitly check that the directory still
> existed because checking that the file still exists covers that, but I can add
a
> line asserting that the directory still exists if that's more clear.

Yeah, I think this is fine as is. I just wanted to make sure this was
intentional that we don't want to force delete the other directory.

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

https://codereview.chromium.org/2087893003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:224:
protected File getStateDirectory() {
Just FYI: We'll eventually get rid of this as the super-class separation was
only useful for document mode.

[Theresa, 2016-06-27 19:54:59]

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

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1038:
}.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
On 2016/06/27 16:23:47, Peter Wen wrote:
> On 2016/06/23 21:49:35, Theresa Wellington wrote:
> > On 2016/06/23 14:42:05, Peter Wen wrote:
> > > Now that we may end up deleting a lot of state files at once, do you think
> > this
> > > could use thread pool instead (or getPrefetchExecutor)?
> > 
> > I think we need to keep the deletion serialized. The tab_state files
> definitely
> > need to be deleted serially so that any new writes occur after the deletion.
I
> > think tab# file deletion needs to be serialized too in the clearState()
case.
> > I'm not sure how realistic this scenario is, but I think we could run into
> > something like this: clear the state and delete files in the thread pool -
> > there's a lot of state, so this takes a while. While that's running, a new
tab
> > is created with an id matching a file that hasn't been deleted yet. I think
> for
> > this to occur, the max_id pref would have to get cleared without the tab#
> files
> > getting deleted.... so maybe it's an invalid concern?
> > 
> > I don't anticipate that the change in directory structure will result in a
> > significant increase in the # of files we're deleting at once. It should
only
> > affect people who used multi-instance and killed the second instance of
Chrome
> > before tab closures were committed to disk.
> 
> Yeah, if we lose max_id then we have bigger problems, but you are right, we
> won't change the number of files to delete significantly. Later we may want to
> separate serial deletion for tab list files but thread pool deletion for tab
> state files. Since file deletion is I/O bound, having a lot of it blocking the
> one serial thread isn't ideal, but no rush as it's not much worse than we have
> now.

I added a TODO note :)

https://codereview.chromium.org/2087893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1084:
if (i == mSelectorIndex) continue;
On 2016/06/27 16:23:47, Peter Wen wrote:
> On 2016/06/23 21:49:35, Theresa Wellington wrote:
> > On 2016/06/23 14:42:05, Peter Wen wrote:
> > > Seems avoiding re-reading the current metadata file is not that much more
> > > complexity. Assuming users keep the vast majority of their tabs in their
> > primary
> > > instance, this makes a lot of sense. Thanks. :)
> > 
> > I decided to pull tabs from the current model rather than the state file for
> the
> > current model in case the set of tabs changes while this is happening in the
> > background. Do you think that's actually possible?
> 
> Yes, the tab list file for the current selector is written out regularly, but
it
> can be out of date, so it's better this way. It may be possible for the list
> file for the instance in the other window to be out of date too. What do you
> think?

It can be, yes, which is why I only want to run the cleanup when a single
instance of ChromeTabbedActivity is running and cancel it if a second one is
created. That seemed easier than sometimes checking multiple models.

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

https://codereview.chromium.org/2087893003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:224:
protected File getStateDirectory() {
On 2016/06/27 16:23:47, Peter Wen wrote:
> Just FYI: We'll eventually get rid of this as the super-class separation was
> only useful for document mode.

sgtm. Do you want me to add a todo note or is there already a bug tracking it
somewhere?

[Peter Wen, 2016-06-27 20:05:44]

lgtm

Thanks for your patience and ample tests!

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

https://codereview.chromium.org/2087893003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:224:
protected File getStateDirectory() {
On 2016/06/27 19:54:58, Theresa Wellington wrote:
> On 2016/06/27 16:23:47, Peter Wen wrote:
> > Just FYI: We'll eventually get rid of this as the super-class separation was
> > only useful for document mode.
> 
> sgtm. Do you want me to add a todo note or is there already a bug tracking it
> somewhere?

A TODO would be great!

[Theresa, 2016-06-27 20:10:38]

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

https://codereview.chromium.org/2087893003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:224:
protected File getStateDirectory() {
On 2016/06/27 20:05:44, Peter Wen wrote:
> On 2016/06/27 19:54:58, Theresa Wellington wrote:
> > On 2016/06/27 16:23:47, Peter Wen wrote:
> > > Just FYI: We'll eventually get rid of this as the super-class separation
was
> > > only useful for document mode.
> > 
> > sgtm. Do you want me to add a todo note or is there already a bug tracking
it
> > somewhere?
> 
> A TODO would be great!

Done.

[Theresa, 2016-06-27 20:17:26]

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

[Theresa, 2016-06-27 20:17:27]

+dfalcantara@ for everything
+tedchoc@ for IncognitoNotificationService*

[gone, 2016-06-29 16:04:12]

Overall looks good, but do we have to worry about Custom Tabs screwing any of
this up?  They _all_ intentionally use index -1 so that they don't mess up real
tab state directories.

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
(right):

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:345:
TabPersistentStore.saveListToFile(getTabbedDataDirectory(), TAB_MODEL_INDEX,
nit: put all arguments on the newline?

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:546:
/** @return Where tabbed mode data is stored for the main {@link TabModelImpl}.
*/
remove for and everything after it.

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
(right):

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1011:
sCleanupTask =
nit:
sCleanupTask = new CleanUpTabStateDataTask(deleteAllFiles);
sCleanupTask.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);?

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1051:
private class CleanUpTabStateDataTask extends AsyncTask<Void, Void,
CleanUpTabStateDataInfo> {
I know this is private, but can you add a javadoc anyway (for clarity)?

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1089:
* Reads tab state files for indexes other than {@code mSelectorIndex}.
Gets the IDs of all tabs in TabModelSelectors other than the currently selected
one?

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java
(right):

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:90:
// Check that all state files moved.
metadata files?

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:105:
// Check that tabs 2-5 did moved.
did move

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:167:
// Check that tab 1 did moved.
grammar

https://chromiumcodereview.appspot.com/2087893003/diff/140001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:189:
// it overwrites tab0_).
tab0_?

[Theresa, 2016-06-29 21:51:51]

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
(right):

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:345:
TabPersistentStore.saveListToFile(getTabbedDataDirectory(), TAB_MODEL_INDEX,
On 2016/06/29 16:04:12, dfalcantara wrote:
> nit: put all arguments on the newline?

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java:546:
/** @return Where tabbed mode data is stored for the main {@link TabModelImpl}.
*/
On 2016/06/29 16:04:12, dfalcantara wrote:
> remove for and everything after it.

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
(right):

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1011:
sCleanupTask =
On 2016/06/29 16:04:12, dfalcantara wrote:
> nit:
> sCleanupTask = new CleanUpTabStateDataTask(deleteAllFiles);
> sCleanupTask.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);?

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1051:
private class CleanUpTabStateDataTask extends AsyncTask<Void, Void,
CleanUpTabStateDataInfo> {
On 2016/06/29 16:04:12, dfalcantara wrote:
> I know this is private, but can you add a javadoc anyway (for clarity)?

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1089:
* Reads tab state files for indexes other than {@code mSelectorIndex}.
On 2016/06/29 16:04:12, dfalcantara wrote:
> Gets the IDs of all tabs in TabModelSelectors other than the currently
selected
> one?

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java
(right):

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:90:
// Check that all state files moved.
On 2016/06/29 16:04:12, dfalcantara wrote:
> metadata files?

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:105:
// Check that tabs 2-5 did moved.
On 2016/06/29 16:04:12, dfalcantara wrote:
> did move

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:167:
// Check that tab 1 did moved.
On 2016/06/29 16:04:12, dfalcantara wrote:
> grammar

Done.

https://codereview.chromium.org/2087893003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java:189:
// it overwrites tab0_).
On 2016/06/29 16:04:12, dfalcantara wrote:
> tab0_?

Done.

[Ted C, 2016-06-29 23:47:23]

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
(right):

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:86:
clearedIncognito &= deleteIncognitoStateFilesInDirectory(
this can just be = instead of &= since there are no longer any joining
conditions

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:194:
private Void closeIncognitoTabsInRunningTabbedActivities() {
this should be lowercase void now

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:195:
return ThreadUtils.runOnUiThreadBlockingNoException(
remove the return and instead of Callable, use a Runnable

[Theresa, 2016-06-30 00:02:52]

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
(right):

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:86:
clearedIncognito &= deleteIncognitoStateFilesInDirectory(
On 2016/06/29 23:47:23, Ted C wrote:
> this can just be = instead of &= since there are no longer any joining
> conditions

Done.

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:194:
private Void closeIncognitoTabsInRunningTabbedActivities() {
On 2016/06/29 23:47:23, Ted C wrote:
> this should be lowercase void now

Done.

https://codereview.chromium.org/2087893003/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:195:
return ThreadUtils.runOnUiThreadBlockingNoException(
On 2016/06/29 23:47:23, Ted C wrote:
> remove the return and instead of Callable, use a Runnable

Done.

[gone, 2016-06-30 00:06:20]

lgtm

https://codereview.chromium.org/2087893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
(right):

https://codereview.chromium.org/2087893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:98:
* incorrectly interleaving create/run/cancel on the task. */
eh, just use a regular javadoc thing here

/**
 * Prevents two blah
 */

[Ted C, 2016-06-30 00:07:46]

lgtm

[Theresa, 2016-06-30 17:24:31]

https://codereview.chromium.org/2087893003/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
(right):

https://codereview.chromium.org/2087893003/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:98:
* incorrectly interleaving create/run/cancel on the task. */
On 2016/06/30 00:06:19, dfalcantara wrote:
> eh, just use a regular javadoc thing here
> 
> /**
>  * Prevents two blah
>  */

Done.

[Theresa, 2016-06-30 17:24:36]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-06-30 17:24:37]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
tedchoc@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2087893003/#ps260001
(title: "Comment styling")

[commit-bot: I haz the power, 2016-06-30 17:25:12]

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-06-30 18:33:48]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-30 18:34:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 18:35:08]

Description was changed from

==========
Migrate TabPersistentStore to one shared directory for multi-instance

Rather than using a sub-directory per ChromeTabbedActivity instance,
use one shared director for all instances.

BUG=622143
==========

to

==========
Migrate TabPersistentStore to one shared directory for multi-instance

Rather than using a sub-directory per ChromeTabbedActivity instance,
use one shared director for all instances.

BUG=622143

Committed: https://crrev.com/b79781bcb77c31e9abbf68f8f5316856b2f0eaa4
Cr-Commit-Position: refs/heads/master@{#403229}
==========

[commit-bot: I haz the power, 2016-06-30 18:35:10]

Patchset 14 (id:??) landed as
https://crrev.com/b79781bcb77c31e9abbf68f8f5316856b2f0eaa4
Cr-Commit-Position: refs/heads/master@{#403229}