Add Browser Actions tab model selector and open a tab through it if ChromeTabbedActivity is not ava…

Add Browser Actions tab model selector and open a tab through it if ChromeTabbedActivity is not available

This CL adds a new tab model selector for Browser Actions. The Browser
Actions tab model selector creates a tab without need of ChromeActivity
and TabContentMananger. It also has it's own TabPersistencePolicy. The
CL also implements how to save and restore the tab model in case that
Chrome crashes. When ChromeTabbedActivity is not available, Browser
Actions will create a Tab from the Browser Actions tab model selector.

This CL also changes the "Open in new Chrome tab" to be executed in a
foreground service.

BUG=730305

[ltian, 2017-07-17 19:04:00]

ltian@chromium.org changed reviewers:
+ tedchoc@chromium.org, yusufo@chromium.org

[ltian, 2017-07-17 19:04:01]

Sorry the CL is a little big. My main concern is whether I add the Browser
Actions tab model selector in the right way. Can you take a look of the changes?

Thanks!

[Yusuf, 2017-07-18 22:59:48]

Some initial comments/questions. I will have to make another pass at the
service.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:685:
if (BrowserActionsTabCreationService.toggleOverviewByBrowserActions(
can we rename this to shouldToggleOverview()?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreatorManager.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreatorManager.java:17:
* The mananger returns a {@link BrowserActionsTabCreator} to create Tabs for
Browser Actions.
manager typo

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreatorManager.java:42:
mTabModel.addTab(tab, -1, TabLaunchType.FROM_BROWSER_ACTIONS);
if we are using FROM_BROWSER_ACTIONS for this, when are we using
FROM_DETACHED_USER_ACTION?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabPersistencePolicy.java:35:
public File getOrCreateStateDirectory() {
so the only current behavior change from the parent is the directory. What next
to add here?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabPersistencePolicy.java:45:
public boolean shouldMergeOnStartup() {
so we are not merging the tabs back in yet? What happens to the tabs in the
tabmodelselector when user opens back Chrome?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:37:
public class BrowsreActionsTabModelSelector
name typo

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:37:
public class BrowsreActionsTabModelSelector
I wonder if we should just move this to tabmodel/ as well to avoid making all
those package protected classes public. That would keep the tabmodel boundary
nicely.

tedchoc@ what do you think?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:53:
// This flag signifies the object has gotten an onNativeReady callback and
/** as well

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:77:
persistencePolicy, this, mTabCreatorManager, persistentStoreObserver);
if we are going to just provide a new one in getInstance, why not create this
policy instance inside the constructor?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:103:
TabModelImpl model = (TabModelImpl) getModel(false);
override getModel return value explicitly instead of casting here.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:112:
public void initializeSelector() {
javadoc

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:120:
TabModel incognitoModel =
we should make sure at least in first implementation, we avoid creating
incognito tabs in the background. it is fine to have the functionality ready,
but we should have safeguards at some point in the codepath.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1682:
nativeInitWebContents(mNativeTabAndroid, mIncognito, mIsHistoryDetached,
This param is for isBackgroundTab, right? Is that only related with history? I
guess I don't see how detached tab in speculation mode is different from a
detached browser actions tab. Both are first navigations, so should not have
history. The only difference is one is user initiated like you mention in launch
type, but I guess I dont see how that relates to the isBackgroundTab param.

And I guess we need another name for this new boolean (if it is necessary) that
makes it clear how these are all linked to the isBackgroundTab check in
tab_android.cc

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

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:74:
FROM_DETACHED_USER_ACTION,
how is this different from the new launch type we added above for browser
actions? Is it a difference of detached_browser_action and
frozen_browser_actions? I wonder how relevant those will be in things that use
TabLaunchType.

Can we not just use FROM_DETACHED and FROM_RESTORE? Or can we not get away with
adding only one new FROM_BROWSER_ACTIONS?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorBase.java
(left):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorBase.java:32:
// Only normal and incognito supported for now.
is this not the case anymore?

[Yusuf, 2017-07-19 00:13:01]

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:29:
public class BrowserActionsTabCreationService extends Service {
BrowserActionsService is cleaner I think.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:30:
public static final String ACTION_TAB_CREATION_START =
Some name clarification here:

TAB_CREATION_START is more like initialize
TAB_CREATION_FINISH is more like on_tab_load_finished
TAB_CREATION_UPDATE is more like on_tab_created

does that sound accurate?

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:61:
public int onStartCommand(Intent intent, int flags, int startId) {
Note that onStartCommand is on the main thread and in a way you are using this
as a way to get multiple messages which might be problematic long term depending
on what operations you want to carry out below.

The right way to handle this is probably making this an IntentService which is a
started service but creates async task for handling each message on its own
behind the scenes.

Or we can also do a started service that gives out an aidl interface on bind but
I dont think that is needed here.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:163:
public static Intent getTabCreationIntent(String action) {
this can handle all that is necessary in itself, taking the action, creating the
intent and then calling startService with that. I think that will avoid
repetition and be cleaner.

[ltian, 2017-08-07 23:24:12]

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:685:
if (BrowserActionsTabCreationService.toggleOverviewByBrowserActions(
On 2017/07/18 22:59:47, Yusuf wrote:
> can we rename this to shouldToggleOverview()?

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:29:
public class BrowserActionsTabCreationService extends Service {
On 2017/07/19 00:13:00, Yusuf wrote:
> BrowserActionsService is cleaner I think.

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:30:
public static final String ACTION_TAB_CREATION_START =
On 2017/07/19 00:13:00, Yusuf wrote:
> Some name clarification here:
> 
> TAB_CREATION_START is more like initialize
> TAB_CREATION_FINISH is more like on_tab_load_finished
> TAB_CREATION_UPDATE is more like on_tab_created
> 
> does that sound accurate?

Yes, TAB_CREATION_START means start creating a tab. But at that time, tab id is
not available. TAB_CREATION_UPDATE means tab id is available, we need to update
it in the notification because for a single tab notification, clicking it will
directly opens the tab, otherwise shows the overview. TAB_CREATION_FINISH means
tab is fully loaded.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:61:
public int onStartCommand(Intent intent, int flags, int startId) {
On 2017/07/19 00:13:00, Yusuf wrote:
> Note that onStartCommand is on the main thread and in a way you are using this
> as a way to get multiple messages which might be problematic long term
depending
> on what operations you want to carry out below.
> 
> The right way to handle this is probably making this an IntentService which is
a
> started service but creates async task for handling each message on its own
> behind the scenes.
> 
> Or we can also do a started service that gives out an aidl interface on bind
but
> I dont think that is needed here.

The problem for IntentService is it is not a foreground service. The reason we
have this foreground service is because we want the chrome process to be
foreground until a Tab is created. I think onStartCommand in here only deals
with creating notification to force the process to be foreground.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreationService.java:163:
public static Intent getTabCreationIntent(String action) {
On 2017/07/19 00:13:00, Yusuf wrote:
> this can handle all that is necessary in itself, taking the action, creating
the
> intent and then calling startService with that. I think that will avoid
> repetition and be cleaner.

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreatorManager.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreatorManager.java:17:
* The mananger returns a {@link BrowserActionsTabCreator} to create Tabs for
Browser Actions.
On 2017/07/18 22:59:47, Yusuf wrote:
> manager typo

Ops, sorry for that!

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabCreatorManager.java:42:
mTabModel.addTab(tab, -1, TabLaunchType.FROM_BROWSER_ACTIONS);
On 2017/07/18 22:59:47, Yusuf wrote:
> if we are using FROM_BROWSER_ACTIONS for this, when are we using
> FROM_DETACHED_USER_ACTION?

New plan will opens all the tabs in lazy load mode for now, so
Tab.createDetached() should not be used in this CL and FROM_BROWSER_ACTIONS is
enough.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabPersistencePolicy.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabPersistencePolicy.java:35:
public File getOrCreateStateDirectory() {
On 2017/07/18 22:59:47, Yusuf wrote:
> so the only current behavior change from the parent is the directory. What
next
> to add here?

To merge the Browser Actions tabs into normal Chrome tab model, we might have
somthing similar to detachAndStartReparenting() which binds the tab with
ChromeActivity and TabContentMananger. This files will have changes about clean
up the merged files.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabPersistencePolicy.java:45:
public boolean shouldMergeOnStartup() {
On 2017/07/18 22:59:47, Yusuf wrote:
> so we are not merging the tabs back in yet? What happens to the tabs in the
> tabmodelselector when user opens back Chrome?

Yes, this CL does not deal with tab merging. So if user opens the Chrome, newly
added tabs will not be added to the Chrome tab model.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java
(right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:37:
public class BrowsreActionsTabModelSelector
On 2017/07/18 22:59:47, Yusuf wrote:
> name typo

Ops, sorry.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:37:
public class BrowsreActionsTabModelSelector
On 2017/07/18 22:59:47, Yusuf wrote:
> I wonder if we should just move this to tabmodel/ as well to avoid making all
> those package protected classes public. That would keep the tabmodel boundary
> nicely.
> 
> tedchoc@ what do you think?

Looks like the only thing need to change to public is the TabModelSelectorUma
which I think we could pass null for now and later there should be a new class
for Browser Actions. If so, we can keep this in browseractions.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:53:
// This flag signifies the object has gotten an onNativeReady callback and
On 2017/07/18 22:59:47, Yusuf wrote:
> /** as well

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:77:
persistencePolicy, this, mTabCreatorManager, persistentStoreObserver);
On 2017/07/18 22:59:47, Yusuf wrote:
> if we are going to just provide a new one in getInstance, why not create this
> policy instance inside the constructor?

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:103:
TabModelImpl model = (TabModelImpl) getModel(false);
On 2017/07/18 22:59:47, Yusuf wrote:
> override getModel return value explicitly instead of casting here.

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:112:
public void initializeSelector() {
On 2017/07/18 22:59:47, Yusuf wrote:
> javadoc

Done.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowsreActionsTabModelSelector.java:120:
TabModel incognitoModel =
On 2017/07/18 22:59:47, Yusuf wrote:
> we should make sure at least in first implementation, we avoid creating
> incognito tabs in the background. it is fine to have the functionality ready,
> but we should have safeguards at some point in the codepath.

This implementation initializes the incognito tab model but forces the
getModel() only returns the normal model. However, adding safeguards will crash
Chrome because when restore, TabPersistenceStore initializes and checks both
models.

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1682:
nativeInitWebContents(mNativeTabAndroid, mIncognito, mIsHistoryDetached,
On 2017/07/18 22:59:47, Yusuf wrote:
> This param is for isBackgroundTab, right? Is that only related with history? I
> guess I don't see how detached tab in speculation mode is different from a
> detached browser actions tab. Both are first navigations, so should not have
> history. The only difference is one is user initiated like you mention in
launch
> type, but I guess I dont see how that relates to the isBackgroundTab param.
> 
> And I guess we need another name for this new boolean (if it is necessary)
that
> makes it clear how these are all linked to the isBackgroundTab check in
> tab_android.cc

New design will open all tabs in lazy load mode so this change is not needed.

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

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:74:
FROM_DETACHED_USER_ACTION,
On 2017/07/18 22:59:47, Yusuf wrote:
> how is this different from the new launch type we added above for browser
> actions? Is it a difference of detached_browser_action and
> frozen_browser_actions? I wonder how relevant those will be in things that use
> TabLaunchType.
> 
> Can we not just use FROM_DETACHED and FROM_RESTORE? Or can we not get away
with
> adding only one new FROM_BROWSER_ACTIONS?

Revert it back since new design creates all tabs in lazy load mode and does not
use Tab.createDetached().

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorBase.java
(left):

https://codereview.chromium.org/2984453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorBase.java:32:
// Only normal and incognito supported for now.
On 2017/07/18 22:59:47, Yusuf wrote:
> is this not the case anymore?

Sorry, it must be something I accidentally deleted. It is still the same now.

[Yusuf, 2017-08-18 00:55:05]

Just as an initial comment so that you can move forward, following the previous
comments from Ted, should we split this a bit?

Can you pull out the notification Service related bits for example? I feel like
that might clean up quite a bit.