At the moment that is between BrowserApp, CustomTabActivity and WebAppActivity. We'd probably want that when: - The foreground tab changes its tab type (bug 1348686) - We open a new foreground tab whose tab type doesn't match the currently active GeckoApp activity - We select a tab whose tab type doesn't match the currently active GeckoApp activity The initial idea would be that calling the appropriate methods on the Tab/the Tabs instance should be enough to then automatically trigger an activity switch under the above conditions.

I also need to incorporate the changes from bug 1347605 so as to use the correct WebAppActivity when switching *to* a WebApp tab. After a quick look, this mainly seems to involve getting the tab's manifest path and then querying the WebAppIndexer for the correct activity.

A quick status update: Basic activity switching is working fine, that is pressing "Switch" on the new tab snackbar switches to the full UI and likewise going back from there handles the case where the parent tab was a custom tab or web app. It still needs a little polishing to get into a reviewable state, but other than that and in conjunction with bug 1352997 I think the functionality is good enough for landing and then continue working from there.

Jan and Sebastian, This is a major work blocking both custom tab and standalone mode, which we all target to ship in version 55. The patches seems huge so I would like to know what the ETA will be?

Comment on attachment 8856158 [details] Bug 1351739 - Part -1 - Housekeeping. https://reviewboard.mozilla.org/r/127232/#review131980

Comment on attachment 8856160 [details] Bug 1351739 - Part 0 - Use INVALID_TAB_ID more. https://reviewboard.mozilla.org/r/127706/#review131982

Just FYI, later today I'm going to push an updated version with one or two small fixes I've since discovered and that's also been rebased for the recent CustomTabsActivity/WebAppActivity changes that landed at the beginning of this week.

Comment on attachment 8856162 [details] Bug 1351739 - Part 2 - Convert CustomTabsActivity to SafeIntents. https://reviewboard.mozilla.org/r/125834/#review132474 ::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:78 (Diff revision 2) > @Override > public void onCreate(Bundle savedInstanceState) { > super.onCreate(savedInstanceState); > > if (savedInstanceState != null) { > - startIntent = savedInstanceState.getParcelable(SAVED_START_INTENT); > + Intent restoredIntent = savedInstanceState.getParcelable(SAVED_START_INTENT); nit: final

Ugh, mozreview reset all the additional review requests...

Comment on attachment 8856162 [details] Bug 1351739 - Part 2 - Convert CustomTabsActivity to SafeIntents. https://reviewboard.mozilla.org/r/125834/#review133698

Comment on attachment 8856161 [details] Bug 1351739 - Part 1 - Track the currently active activity. https://reviewboard.mozilla.org/r/125830/#review132026 This and the following code introduces quite some complexity to automatically switch activities when a different tab type is selected. However I was wondering whether we are actually going to need this? My naive assumption was that when this happens then the initiating activity knows about that and can do the activity switch too (e.g. when moving a tab from CustomTabActivity to BrowserActivity). But I guess while implementing and researching this you came up for a reason why this should be handled in a more generic way by the tab switching code? Going forward it seems to be a good idea to document this architecture somewhere outside of the code (mobile/android/docs or the wiki).

Comment on attachment 8856161 [details] Bug 1351739 - Part 1 - Track the currently active activity. https://reviewboard.mozilla.org/r/125830/#review132026 > My naive assumption was that when this happens then the initiating activity knows about that and can do the activity switch too That could be possible, but in principle an activity switch would have to be triggered from a number of places, some of which might not even be under our immediate control (add-ons). At the moment we've got at least * the context menu, which is controlled by Gecko and at the moment not even aware of the tab type/activity its opening in * add-ons opening new tabs (e.g. via the context menu or whatever), which most definitively won't be aware that you'll need to switch activities when opening a new tab from within a custom tab/standalone mode web app * Tabs.java itself when closing a tab and selecting its parent tab, which might belong to a different activity So I think it would still be preferable to provide at least some common infrastructure for this to avoid duplicating code all over the place as far as possible. Going one step further and linking this to the tab type and automatically triggering activity switches however has the advantage that things mostly "just work" for existing callers - we don't have to teach JS or add-ons how to switch activities and parent tab selection works out of the box, too. The only real new special casing I had to introduce was [Bug 1352997 - Part 7](https://reviewboard.mozilla.org/r/130308/), because when exiting Firefox an automatic activity switch is indeed undesired, as it conflicts with us disappearing into the background. Also, I think that part of the complexity comes from other sources, e.g. the need to track the correct selected tab for each activity, to have a fall back from tab switch intents (if the tab switch intent ends up in the Android task switcher and then Gecko is killed, thereby invalidating all tab references) or to store extra intent data for custom tabs (all customisations)/web apps (the manifest path).

Comment on attachment 8856158 [details] Bug 1351739 - Part -1 - Housekeeping. https://reviewboard.mozilla.org/r/127232/#review135650 ::: commit-message-67162:1 (Diff revision 4) > +Bug 1351739 - Part -1 - Housekeeping. r?sebastian,nechen Part -1 LOL :D

Comment on attachment 8856160 [details] Bug 1351739 - Part 0 - Use INVALID_TAB_ID more. https://reviewboard.mozilla.org/r/127706/#review135652

Comment on attachment 8856161 [details] Bug 1351739 - Part 1 - Track the currently active activity. https://reviewboard.mozilla.org/r/125830/#review135656 ::: mobile/android/base/java/org/mozilla/gecko/GeckoActivityMonitor.java:18 (Diff revision 4) > +public class GeckoActivityMonitor implements Application.ActivityLifecycleCallbacks { > + private static final String LOGTAG = "GeckoActivityMonitor"; > + > + @SuppressLint("StaticFieldLeak") > + // We only hold a reference to the currently running activity - when this activity pauses, > + // the reference is released or else overwritten by the next activity. I think put comments before annotation might be better, how do you think? ::: mobile/android/base/java/org/mozilla/gecko/GeckoActivityMonitor.java:38 (Diff revision 4) > + @Override > + public void onActivityCreated(Activity activity, Bundle savedInstanceState) { > + currentActivity = activity; > + } > + > + // onNewIntent happens inbetween a pause/resume cycle, which means that we wouldn't have in-between? ::: mobile/android/base/java/org/mozilla/gecko/GeckoActivityMonitor.java:58 (Diff revision 4) > + currentActivity = activity; > + } > + > + /** > + * Intended to be used if the current activity is required to be up-to-date for code that > + * executes in onCreate/onstart/... before calling the corresponding superclass method. onstart -> onStart :-)

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review135696 ::: mobile/android/base/java/org/mozilla/gecko/Tab.java:62 (Diff revision 4) > > private IconRequestBuilder mIconRequestBuilder; > private Future<IconResponse> mRunningIconRequest; > > private boolean mHasFeeds; > + private SafeIntent mCustomTabIntent; I am wondering should Tab persist a member for CustomTab? Because Tab is used by normal-browser and WebpAppActivity, not only for CustomTab. ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:323 (Diff revision 4) > > + if (oldTab != null && oldTab.getType() != tab.getType() && > + !currentActivityMatchesTab(tab)) { > + // We're in the wrong activity for this kind of tab, so launch the correct one > + // and then try again. > + launchActivityForTab(tab); I am not sure whether it is a good idea that "Tabs.java understands what is Activity". IMHO to interactions between Activities(ie. to launch an activity) to be kept in Activity itself might be better. If CustomTabsActivity wants to 'open a tab in Browser', then launching Activity should be done by CustomTabs itself, not in Tabs. ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:355 (Diff revision 4) > + > + if (currentActivity == null) { > + return false; > + } > + String currentActivityName = currentActivity.getClass().getName(); > + return currentActivityName.equals(getClassNameForTab(tab)); Multiple 3rd-party apps will launch the same Activity. It is likely keeping return true for each different tabs. Will it cause any confusion?

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review135696 > I am not sure whether it is a good idea that "Tabs.java understands what is Activity". IMHO to interactions between Activities(ie. to launch an activity) to be kept in Activity itself might be better. > > If CustomTabsActivity wants to 'open a tab in Browser', then launching Activity should be done by CustomTabs itself, not in Tabs. Because activity switches could be triggered by a number of things both on the Gecko and Java-side, I'd still want to handle this centrally somehow. I think I originally settled on this approach because our current "last selected tab within this activity"-tracking automatically runs in onPause - hence I wanted to defer tab selection to happen only after the activity switch (and therefore onPause for the previous activity) had already taken place, so we wouldn't record the wrong tab in the wrong activity in onPause. Therefore this dance of launching a new activity, returning early from tab selection and then completing the process by re-running tab selection from the new activity. In the end, my hopes for that didn't turn out as expected, hence the addition of bug 1352997 to completely overhaul the system for tracking which tab should be selected in which activity. With bug 1352997 in place, I think it could be possible to let Tabs.java select tabs normally again and just react to the Tab.SELECTED event in GeckoApp to hand off to a different activity if necessary. The activity monitoring would also probably become unnecessary again because an activity would of course know whether it was currently running or not when the SELECTED event arrived. However: Unless there are some fundamental problems with the current patch (as opposed to merely "philosophical" concerns about where the activity switching code should live in the end), I'd rather go ahead with the current approach to get it into the tree and then move the activity switching code elsewhere as a follow-up where I can take the combined result of this and bug 1352997 as a baseline for further refactoring. > Multiple 3rd-party apps will launch the same Activity. It is likely keeping return true for each different tabs. Will it cause any confusion? I'm afraid I can't quite follow you. Could you please explain the scenario you've got in mind in a bit more detail?

Comment on attachment 8856161 [details] Bug 1351739 - Part 1 - Track the currently active activity. https://reviewboard.mozilla.org/r/125830/#review135656 > I think put comments before annotation might be better, how do you think? I agree (ditto for the other suggestions.)

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review135696 > I am wondering should Tab persist a member for CustomTab? Because Tab is used by normal-browser and WebpAppActivity, not only for CustomTab. On the one hand I see what you mean, on the other hand, if we want to be able to flexibly move tabs between activities (e.g. the idea that was floating around to display web apps as a custom tab when outside the actual scope of the web app), then potentially any tab could eventually be required to store a custom tab intent. We do have a separate tab type (derived from the base tab type) for private tabs, but there, the separation is stricter - we never turn private mode tabs into normal tabs or vice versa.

(In reply to Jan Henning [:JanH] from comment #49) > With bug 1352997 in place, I think it could be possible to let Tabs.java > select tabs normally again and just react to the Tab.SELECTED event in > GeckoApp to hand off to a different activity if necessary. The activity > monitoring would also probably become unnecessary again because an activity > would of course know whether it was currently running or not when the > SELECTED event arrived. This sounds like a way nicer architecture and less complexity! > However: Unless there are some fundamental problems with the current patch > (as opposed to merely "philosophical" concerns about where the activity > switching code should live in the end), I'd rather go ahead with the current > approach to get it into the tree and then move the activity switching code > elsewhere as a follow-up where I can take the combined result of this and > bug 1352997 as a baseline for further refactoring. How fast can this follow-up refactoring happen? There might not be a fundamental problem but if we could avoid the complexity of the current approach (and therefore the risk of temporary regressions) then this sounds like something worth doing?

Comment on attachment 8856161 [details] Bug 1351739 - Part 1 - Track the currently active activity. https://reviewboard.mozilla.org/r/125830/#review136218 I'm not really in favor of tracking this (There are multiple ways this can go wrong). But as you said there's the opportunity to get rid of this again - so let's move forward.

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review136222

Comment on attachment 8856164 [details] Bug 1351739 - Part 4 - Handle selected tab temporarily being undefined. https://reviewboard.mozilla.org/r/125826/#review136224 Reading the reason for that I'm a bit afraid that we are going to accidentally switch activities at some point. :-)

Comment on attachment 8856165 [details] Bug 1351739 - Part 5 - Implement activity switching for web apps. https://reviewboard.mozilla.org/r/125842/#review136226 ::: mobile/android/base/java/org/mozilla/gecko/Tab.java:311 (Diff revision 4) > + public String getManifestPath() { > + return mManifestPath; > + } This is the path of the manifest in the local file system, right? Can you add this as javadoc (or in the name of the method if you have a good idea how). ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:378 (Diff revision 4) > intent.setData(Uri.parse(tab.getURL())); > } > break; > + case WEBAPP: > + intent = new Intent(GeckoApp.ACTION_WEBAPP); > + String manifestPath = tab.getManifestPath(); nit: final ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:399 (Diff revision 4) > + // TODO: When things have settled down a bit, we should split this and everything similar > + // TODO: in the WebAppActivity into a dedicated WebAppManifest class (bug 1353868). This is a good idea! Can you file a bug for that?

Comment on attachment 8856166 [details] Bug 1351739 - Part 6 - Finish the WebAppActivity when closing via onDone. https://reviewboard.mozilla.org/r/125828/#review136228

(In reply to Sebastian Kaspari (:sebastian) from comment #52) > How fast can this follow-up refactoring happen? There might not be a > fundamental problem but if we could avoid the complexity of the current > approach (and therefore the risk of temporary regressions) then this sounds > like something worth doing? Maybe two week(-end)s + review cycles? Of course it also depends on how many unexpected side issues I run into (viz. this bug), although I don't expect anything as major as bug 1352997. I'd still prefer to clear the current work off my plate though, so I don't have to untangle bug 1352997 from this one beforehand and continue maintaining this (or especially bug 1352997) separately against other changes - although thankfully development tempo on custom tabs and web apps seems to have slowed down a bit recently. While I agree that compared to the outlined approach after refactoring the current implementation introduces a little bit of additional risk when an activity switch is triggered by a tab closing (because unlike before and presumably afterwards, we now close the tab before officially selecting its replacement), I don't think it is too terribly large. Also, on the other hand landing the current state also means that the other parts of activity switching can receive feedback earlier.

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review135696 > Because activity switches could be triggered by a number of things both on the Gecko and Java-side, I'd still want to handle this centrally somehow. > > I think I originally settled on this approach because our current "last selected tab within this activity"-tracking automatically runs in onPause - hence I wanted to defer tab selection to happen only after the activity switch (and therefore onPause for the previous activity) had already taken place, so we wouldn't record the wrong tab in the wrong activity in onPause. Therefore this dance of launching a new activity, returning early from tab selection and then completing the process by re-running tab selection from the new activity. In the end, my hopes for that didn't turn out as expected, hence the addition of bug 1352997 to completely overhaul the system for tracking which tab should be selected in which activity. > > With bug 1352997 in place, I think it could be possible to let Tabs.java select tabs normally again and just react to the Tab.SELECTED event in GeckoApp to hand off to a different activity if necessary. The activity monitoring would also probably become unnecessary again because an activity would of course know whether it was currently running or not when the SELECTED event arrived. > > However: Unless there are some fundamental problems with the current patch (as opposed to merely "philosophical" concerns about where the activity switching code should live in the end), I'd rather go ahead with the current approach to get it into the tree and then move the activity switching code elsewhere as a follow-up where I can take the combined result of this and bug 1352997 as a baseline for further refactoring. Opened bug 1359531 to track the follow-up work which I'll get to as soon as this here is out of the way.

Comment on attachment 8856164 [details] Bug 1351739 - Part 4 - Handle selected tab temporarily being undefined. https://reviewboard.mozilla.org/r/125826/#review136224 Well, *whistling*, that's what prompted [bug 1352997 - part 7](https://reviewboard.mozilla.org/r/130308/)... It can only happen for custom tabs/web apps, though, because there the default tab selection logic will fall back to normal tabs if no tabs of the same type are available.

Comment on attachment 8856165 [details] Bug 1351739 - Part 5 - Implement activity switching for web apps. https://reviewboard.mozilla.org/r/125842/#review136226 > This is a good idea! Can you file a bug for that? I already did, didn't I?

(In reply to Jan Henning [:JanH] from comment #49) > > Multiple 3rd-party apps will launch the same Activity. It is likely keeping return true for each different tabs. Will it cause any confusion? > > I'm afraid I can't quite follow you. Could you please explain the scenario > you've got in mind in a bit more detail? Sorry I didn't explain it well. For example, both of Google+ app and Gmail app could use CustomTabs. So there might be two CustomTabsActivity instance meanwhile and they have the same class name. For Tab-for-Gmail and current-activity-is-for-googleplus, this method still return true. Curious will it cause any issue? (maybe this won't happen so it will be safe).

Comment on attachment 8856164 [details] Bug 1351739 - Part 4 - Handle selected tab temporarily being undefined. https://reviewboard.mozilla.org/r/125826/#review136728

Comment on attachment 8856165 [details] Bug 1351739 - Part 5 - Implement activity switching for web apps. https://reviewboard.mozilla.org/r/125842/#review136780

Comment on attachment 8856166 [details] Bug 1351739 - Part 6 - Finish the WebAppActivity when closing via onDone. https://reviewboard.mozilla.org/r/125828/#review136782

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review135696 > I'm afraid I can't quite follow you. Could you please explain the scenario you've got in mind in a bit more detail? (In reply to Julian Chu [:walkingice] from comment #70) > (In reply to Jan Henning [:JanH] from comment #49) > > > Multiple 3rd-party apps will launch the same Activity. It is likely keeping return true for each different tabs. Will it cause any confusion? > > > > I'm afraid I can't quite follow you. Could you please explain the scenario > > you've got in mind in a bit more detail? > > Sorry I didn't explain it well. For example, both of Google+ app and Gmail > app could use CustomTabs. So there might be two CustomTabsActivity instance > meanwhile and they have the same class name. For Tab-for-Gmail and > current-activity-is-for-googleplus, this method still return true. Curious > will it cause any issue? (maybe this won't happen so it will be safe). Ah, right. Yes, we only care about matching up tab types with their corresponding activity, so while in a custom tab, it is possible to select a different custom tab without triggering an activity switch. So far I think we don't do this by default (i.e. no add-ons, which could potentially do all sorts of weird stuff), except when closing the tab - which we only do when leaving the activity, in which case it won't matter anyway.

Comment on attachment 8856163 [details] Bug 1351739 - Part 3 - Switch activities when a custom tab is selected/unselected. https://reviewboard.mozilla.org/r/125836/#review137168

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s ce12e3556ae5 -d e919535c7872: rebasing 392119:ce12e3556ae5 "Bug 1351739 - Part -1 - Housekeeping. r=sebastian,walkingice" merging mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java rebasing 392120:6db9242b469e "Bug 1351739 - Part 0 - Use INVALID_TAB_ID more. r=sebastian,walkingice" rebasing 392121:fbe7d9c42370 "Bug 1351739 - Part 1 - Track the currently active activity. r=sebastian,walkingice" rebasing 392122:ad2ab9f45b2b "Bug 1351739 - Part 2 - Convert CustomTabsActivity to SafeIntents. r=sebastian,walkingice" merging mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java merging mobile/android/base/java/org/mozilla/gecko/customtabs/IntentUtil.java warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by mozilla@buttercookie.de: https://hg.mozilla.org/integration/autoland/rev/afc8f7514251 Part -1 - Housekeeping. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/732aac065f98 Part 0 - Use INVALID_TAB_ID more. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/3d9e6fe1d43a Part 1 - Track the currently active activity. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/cec16c86471f Part 2 - Convert CustomTabsActivity to SafeIntents. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/6881d81cd2a3 Part 3 - Switch activities when a custom tab is selected/unselected. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/d8afa0011e06 Part 4 - Handle selected tab temporarily being undefined. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/2b2265a57f4c Part 5 - Implement activity switching for web apps. r=sebastian,walkingice https://hg.mozilla.org/integration/autoland/rev/f4258202a88d Part 6 - Finish the WebAppActivity when closing via onDone. r=sebastian,walkingice

https://hg.mozilla.org/mozilla-central/rev/afc8f7514251 https://hg.mozilla.org/mozilla-central/rev/732aac065f98 https://hg.mozilla.org/mozilla-central/rev/3d9e6fe1d43a https://hg.mozilla.org/mozilla-central/rev/cec16c86471f https://hg.mozilla.org/mozilla-central/rev/6881d81cd2a3 https://hg.mozilla.org/mozilla-central/rev/d8afa0011e06 https://hg.mozilla.org/mozilla-central/rev/2b2265a57f4c https://hg.mozilla.org/mozilla-central/rev/f4258202a88d