New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 606513 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

Add tab persistence for Chrome Custom Tabs

Project Member Reported by tedc...@chromium.org, Apr 25 2016

Issue description

Currently, CCTs do not store/restore their tab state, so if you navigate away in Android and return it will resume the launching page instead of the last one.

We should add some sort of tab persistence for these entries.

Easiest repro steps:

1.) Enable Elderberrry herb prototype
  build/android/adb_chrome_public_command_line --tab-management-experiment-type-elderberry

2.) Launch standalone CCT
  adb shell am start -a android.intent.action.VIEW -d "https://www.yahoo.com"

3.) Navigate some pages down

4.) Hit android home button

5.) Kill browser process via ADB (adb shell ps | grep chrome)

6.) Go to recents and click on previously launched intent

-----------

Expected: Should be on the same page you were last viewing

Actual: Showing the initial url instead.
 
Status: Started (was: Assigned)
Cc: ian...@chromium.org yus...@chromium.org
Random thoughts:

1) CCT uses (and mostly disables) the TabPersistentStore, assigning all CCTs a TabModelSelector index of -1.  If we plan to use the TabPersistentStore like this, we'll need to assign a unique index to each CCT or (at the very least) separate their tabs out into a special app_cct_tabs directory instead of overwriting the app_tabs directory files like when CCTs first started using TabModelSelectorImpls.

2) CCTs can have multiple tabs opened, but only displays one of them.  This means that we'll have to save out data for all of them and track the TabModel metadata as well to be completely accurate.

3) The last time we were considering this, we thought storing data out to the Bundle/SavedInstanceState would be viable.  I'm not sure how viable this is because there is some cap on how big persisted Bundles can be.

4) CCTs will rarely get the chance to clean up after themselves, and will more likely be killed by the user while Clank isn't running.  This means we'll need some special cleanup mechanism if we persist files to storage.

5) At the very least, we could store the last URL that was displayed by the CCT instead of the full TabState.  This wouldn't work well for the case where there were multiple tabs in the TabModel.

Added Ian and Yusuf in case they can think of any other problems.

Comment 3 by ian...@chromium.org, Apr 26 2016

We had a similar discussion before in crbug.com/567719. At that time we thought it was a low priority thing. Now since our new tab management uses CCT more, we should make it happen. :P
6) We should make a decision around whether the customization will also be persisted. (My personal opionion is yes it should) and then find a way to serialize that as well. If we are "bringing back" the custom tab, it should come back in the same customized way it was launched from the host app, since this activity will still be in the same activity. It would be confusing to get a default looking CCT on top of any client app.

The easiest way to do this would be making sure we don't lose the intent.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1382473e75f7ae9fc20ce45356e1c94855ede461

commit 1382473e75f7ae9fc20ce45356e1c94855ede461
Author: tedchoc <tedchoc@chromium.org>
Date: Thu Aug 25 19:36:34 2016

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 

Review-Url: https://codereview.chromium.org/2277603002
Cr-Commit-Position: refs/heads/master@{#414510}

[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[add] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
[add] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/java_sources.gni
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/ContextMenuLoadUrlParamsTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassinTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/MultiInstanceMigrationTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelMergingTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorTabObserverTest.java
[modify] https://crrev.com/1382473e75f7ae9fc20ce45356e1c94855ede461/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java

Cc: dfalcant...@chromium.org
Owner: tedc...@chromium.org
Labels: M-54
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bd10cbe206cb17700d56b92fdf07190437866ff

commit 1bd10cbe206cb17700d56b92fdf07190437866ff
Author: tedchoc <tedchoc@chromium.org>
Date: Wed Aug 31 22:34:48 2016

Add tab persistence support for CustomTabs.

BUG= 606513 

Review-Url: https://codereview.chromium.org/2296833002
Cr-Commit-Position: refs/heads/master@{#415786}

[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[add] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/java_sources.gni
[add] https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7f175a1a5ec859e918853143bfd37e29131e94c

commit e7f175a1a5ec859e918853143bfd37e29131e94c
Author: yhirano <yhirano@chromium.org>
Date: Thu Sep 01 04:06:53 2016

Revert of Add tab persistence support for CustomTabs. (patchset #6 id:100001 of https://codereview.chromium.org/2296833002/ )

Reason for revert:
The added tests are failing.
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/35991

Original issue's description:
> Add tab persistence support for CustomTabs.
>
> BUG= 606513 
>
> Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
> Cr-Commit-Position: refs/heads/master@{#415786}

TBR=dfalcantara@chromium.org,yusufo@chromium.org,twellington@chromium.org,nyquist@chromium.org,tedchoc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 606513 

Review-Url: https://codereview.chromium.org/2292083004
Cr-Commit-Position: refs/heads/master@{#415876}

[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[delete] https://crrev.com/cd2ad916a1f9d140b54492987e874de5a1944a0b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/e7f175a1a5ec859e918853143bfd37e29131e94c/chrome/android/java_sources.gni
[delete] https://crrev.com/cd2ad916a1f9d140b54492987e874de5a1944a0b/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5190ef576cbe6dadaaa36c634e3f0610093c6b43

commit 5190ef576cbe6dadaaa36c634e3f0610093c6b43
Author: tedchoc <tedchoc@chromium.org>
Date: Thu Sep 01 19:23:22 2016

Add tab persistence support for CustomTabs.

BUG= 606513 

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Review-Url: https://codereview.chromium.org/2296833002
Cr-Original-Commit-Position: refs/heads/master@{#415786}
Cr-Commit-Position: refs/heads/master@{#416015}

[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[add] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/java_sources.gni
[add] https://crrev.com/5190ef576cbe6dadaaa36c634e3f0610093c6b43/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java

Comment 11 by k...@chromium.org, Sep 6 2016

Labels: ReleaseBlock-Stable
Tab persistence is needed before we land Herb in stable as Herb will expand the scope of CCTs greatly and it is a bad experience for users to lose state of their CCTs when OOM especially on lower end devices where there is less memory.
Labels: Merge-Request-54
Per #11, merge request for https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff

Comment 13 by dimu@chromium.org, Sep 8 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 9 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d6ed09d8aeffd50bfad099c047785d8730dfefa

commit 0d6ed09d8aeffd50bfad099c047785d8730dfefa
Author: Ted Choc <tedchoc@google.com>
Date: Fri Sep 09 17:47:56 2016

Add tab persistence support for CustomTabs.

BUG= 606513 

Review URL: https://codereview.chromium.org/2322163004 .

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Review-Url: https://codereview.chromium.org/2296833002
Cr-Original-Original-Commit-Position: refs/heads/master@{#415786}
Cr-Original-Commit-Position: refs/heads/master@{#416015}
Cr-Commit-Position: refs/branch-heads/2840@{#275}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[add] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java_sources.gni
[add] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java

Status: Fixed (was: Started)
Marking this as fixed.

Here are two follow up bugs:
https://bugs.chromium.org/p/chromium/issues/detail?id=646595
https://bugs.chromium.org/p/chromium/issues/detail?id=646600
To test this:

1.) Open a custom tab, navigate a to a few pages (need to be on a different page from the initial one to help verify)
2.) Leave the CCT
3.) Kill Chrome
4.) Refocus the custom tab and ensure you are on the final page you navigated to

To achieve #3, there are two approaches that I can think of:
1.) Kill Chrome via the command line:
  # adb root
  # adb shell ps | grep chrome | tr -s ' ' | cut -d ' ' -f2 | xargs adb shell kill -9

2.) Turn on Android's don't keep activities.  I believe this should save the state and trigger a restore, but I haven't tried.
Status: Assigned (was: Fixed)
I just tried this on a Nexus 5X / MDA89E with Chrome Dev 54.0.2840.25 :

- The first page that was opened in the custom tab is still what gets loaded when the custom tab is refocused. The URL of the previous final page does appear momentarily on the toolbar, but is replaced by the first page when loading begins. 
- This happens for both approaches to killing Chrome, via the command line and the Android don't-keep-activities setting.

Video of the command line killing approach - http://go/chrome-androidlogs1/6/606513
Status: Started (was: Assigned)

Comment 19 by k...@chromium.org, Sep 14 2016

Cc: k...@chromium.org
Labels: Merge-Request-54
Merge request for #20 as well.

Comment 22 by dimu@chromium.org, Sep 15 2016

Labels: -Merge-Request-54 Merge-Approved-54
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 23 by sheriffbot@chromium.org, Sep 16 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 24 by bugdroid1@chromium.org, Sep 16 2016

Status: Fixed (was: Started)
I think the best way to test is with don't keep activities set in Android.  Killing the process will not aways work for CCTs vs Normal Chrome as we don't restore them as aggressively in CCT land.
Status: Verified (was: Fixed)
Thank you tedchoc@ ! Verified on 54.0.2840.40 with the "Don't keep activities" Android setting.
Project Member

Comment 27 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d6ed09d8aeffd50bfad099c047785d8730dfefa

commit 0d6ed09d8aeffd50bfad099c047785d8730dfefa
Author: Ted Choc <tedchoc@google.com>
Date: Fri Sep 09 17:47:56 2016

Add tab persistence support for CustomTabs.

BUG= 606513 

Review URL: https://codereview.chromium.org/2322163004 .

Committed: https://crrev.com/1bd10cbe206cb17700d56b92fdf07190437866ff
Review-Url: https://codereview.chromium.org/2296833002
Cr-Original-Original-Commit-Position: refs/heads/master@{#415786}
Cr-Original-Commit-Position: refs/heads/master@{#416015}
Cr-Commit-Position: refs/branch-heads/2840@{#275}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[add] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistencePolicy.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/java_sources.gni
[add] https://crrev.com/0d6ed09d8aeffd50bfad099c047785d8730dfefa/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java

Sign in to add a comment