1359531 - Further optimise activity switching

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

After a friendly nudge during review, it turned out that bug 1352997 might allow simplifying the original activity switching approach from bug 1351739.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too.

We want to move the activity switching code into the activity (i.e. GeckoApp) itself and run it from its onTabChanged handler in response to the appropriate events (at the moment that's only SELECTED), so we need to ensure that this will actually be called within custom tabs/web apps as well.

Additionally, there's no need to separately register the tab events listener from the CustomTabs/WebAppActivity as well if our parent class already does it for us.

Review commit: https://reviewboard.mozilla.org/r/134854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134854/

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 2 - Move some tab switch functionality to the tab object.

These methods already take a tab object as their argument, so we just turn them into Tab member functions.

Review commit: https://reviewboard.mozilla.org/r/134866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134866/

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 3 - Creating new intents is a job for IntentHelper.

Review commit: https://reviewboard.mozilla.org/r/134868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134868/

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 4 - Import Tab intent extra key definitions.

Review commit: https://reviewboard.mozilla.org/r/134870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134870/

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 5 - Trigger activity switching from GeckoApp.

To avoid the need for tracking the currently active activity, we just respond from the activity itself by using GeckoApp's tab event listener.

As replacement for the "closeTabNoActivitySwitch()" function, we set a flag on the activity instead before closing the tab.

Review commit: https://reviewboard.mozilla.org/r/134856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134856/

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 6 - Remove manual setting of current activity.

... we hardly knew ye. We no longer need it for the tab switching functionality and while we still need to improve our application-background/foreground tracking (compare bug 1346413 and bug 1338673), an ActivityLifecycleCallbacks listener isn't necessarily the ideal solution, either [1]. Therefore, we'll remove it again.

[1] With it, we'd no longer have to manually call GeckoApplication.onActivityPause/Resume() from each activity, but if we want to synchronously do something in onPause (e.g. call our onPause()/application-background handlers in Gecko), we still need additional logic to track whether that onPause() call was just switching between internal activities or actually indicates that we're in background now.

Review commit: https://reviewboard.mozilla.org/r/134860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134860/

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN.

Normally, receiving a new intent means we want to switch tabs/open a new tab, so we need to turn off our "last selected tab in this activity" logic, however this is not true when we receive a simple Intent.ACTION_MAIN, e.g. when we're being brought into the foreground from a launcher icon (as opposed to the activity/task switcher).

Review commit: https://reviewboard.mozilla.org/r/136622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136622/

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1359531 - Part 8 - Handle tabs selected while all GeckoApps were in background.

Activity switching is now handled by the currently active GeckoApp instance, which obviously doesn't work if all of them are currently backgrounded.

To fix this, we track for each tab whether its selection was handled by a foregrounded GeckoApp instance. If it wasn't, we catch up with any possibly necessary activity switches during the next resume.

Review commit: https://reviewboard.mozilla.org/r/136624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136624/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865220 [details]
Bug 1359531 - Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too.

https://reviewboard.mozilla.org/r/134854/#review140042

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865221 [details]
Bug 1359531 - Part 2 - Move some tab switch functionality to the tab object.

https://reviewboard.mozilla.org/r/134866/#review140052

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865222 [details]
Bug 1359531 - Part 3 - Creating new intents is a job for IntentHelper.

https://reviewboard.mozilla.org/r/134868/#review141288

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865223 [details]
Bug 1359531 - Part 4 - Import Tab intent extra key definitions.

https://reviewboard.mozilla.org/r/134870/#review141346

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865224 [details]
Bug 1359531 - Part 5 - Trigger activity switching from GeckoApp.

https://reviewboard.mozilla.org/r/134856/#review141348

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865225 [details]
Bug 1359531 - Part 6 - Remove manual setting of current activity.

https://reviewboard.mozilla.org/r/134860/#review141350

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865226 [details]
Bug 1359531 - Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN.

https://reviewboard.mozilla.org/r/136622/#review141352

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8865227 [details]
Bug 1359531 - Part 8 - Handle tabs selected while all GeckoApps were in background.

https://reviewboard.mozilla.org/r/136624/#review141354

Comment 17

[Julian Chu [:walkingice]]

Comment on attachment 8865220 [details]
Bug 1359531 - Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too.

https://reviewboard.mozilla.org/r/134854/#review142322

Comment 18

[Jan Henning [:JanH]]

So in the mean time, over in bug 1363567 jchen has discovered the GeckoActivityMonitor and found it useful for a problem of his. Besides, despite what I've written in the commit message for part 6, I'm actually still undecided on whether the activity monitor might not after all be helpful in improving our application-background/foreground logic.

Comment 19

[Julian Chu [:walkingice]]

Comment on attachment 8865221 [details]
Bug 1359531 - Part 2 - Move some tab switch functionality to the tab object.

https://reviewboard.mozilla.org/r/134866/#review142324

Comment 20

[Julian Chu [:walkingice]]

Comment on attachment 8865222 [details]
Bug 1359531 - Part 3 - Creating new intents is a job for IntentHelper.

https://reviewboard.mozilla.org/r/134868/#review142326

Comment 21

[Julian Chu [:walkingice]]

Comment on attachment 8865223 [details]
Bug 1359531 - Part 4 - Import Tab intent extra key definitions.

https://reviewboard.mozilla.org/r/134870/#review142328

Comment 22

[Julian Chu [:walkingice]]

Comment on attachment 8865224 [details]
Bug 1359531 - Part 5 - Trigger activity switching from GeckoApp.

https://reviewboard.mozilla.org/r/134856/#review142330

Comment 23

[Julian Chu [:walkingice]]

Comment on attachment 8865225 [details]
Bug 1359531 - Part 6 - Remove manual setting of current activity.

https://reviewboard.mozilla.org/r/134860/#review142332

Comment 24

[Julian Chu [:walkingice]]

Comment on attachment 8865226 [details]
Bug 1359531 - Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN.

https://reviewboard.mozilla.org/r/136622/#review142334

Comment 25

[Julian Chu [:walkingice]]

Comment on attachment 8865227 [details]
Bug 1359531 - Part 8 - Handle tabs selected while all GeckoApps were in background.

https://reviewboard.mozilla.org/r/136624/#review142336

Comment 26

[Jan Henning [:JanH]]

And bug 1363567 is landing now, so part 6 needs updating to no longer rip out the GAM after all.

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8865220 [details]
Bug 1359531 - Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134854/diff/1-2/

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8865221 [details]
Bug 1359531 - Part 2 - Move some tab switch functionality to the tab object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134866/diff/1-2/

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8865222 [details]
Bug 1359531 - Part 3 - Creating new intents is a job for IntentHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134868/diff/1-2/

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8865223 [details]
Bug 1359531 - Part 4 - Import Tab intent extra key definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134870/diff/1-2/

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8865224 [details]
Bug 1359531 - Part 5 - Trigger activity switching from GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134856/diff/1-2/

Comment 32

[Jan Henning [:JanH]]

Comment on attachment 8865225 [details]
Bug 1359531 - Part 6 - Remove manual setting of current activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134860/diff/1-2/

Comment 33

[Jan Henning [:JanH]]

Comment on attachment 8865226 [details]
Bug 1359531 - Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136622/diff/1-2/

Comment 34

[Jan Henning [:JanH]]

Comment on attachment 8865227 [details]
Bug 1359531 - Part 8 - Handle tabs selected while all GeckoApps were in background.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136624/diff/1-2/

Comment 35

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #18)
> Besides,
> despite what I've written in the commit message for part 6, I'm actually
> still undecided on whether the activity monitor might not after all be
> helpful in improving our application-background/foreground logic.

So I've looked into it again and it seems my original assessment was correct and somehow along the way I got confused and misremembered things. If we want to do synchronous application-background handling from within on(Activity)Pause(d), we still need additional logic. All the GeckoActivityMonitor could buy us is no longer having to manually call our own onActivityPause/onActivityResume implementation in GeckoApplication.

Comment 36

[Jan Henning [:JanH]]

Comment on attachment 8865220 [details]
Bug 1359531 - Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134854/diff/2-3/

Comment 37

[Jan Henning [:JanH]]

Comment on attachment 8865221 [details]
Bug 1359531 - Part 2 - Move some tab switch functionality to the tab object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134866/diff/2-3/

Comment 38

[Jan Henning [:JanH]]

Comment on attachment 8865222 [details]
Bug 1359531 - Part 3 - Creating new intents is a job for IntentHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134868/diff/2-3/

Comment 39

[Jan Henning [:JanH]]

Comment on attachment 8865223 [details]
Bug 1359531 - Part 4 - Import Tab intent extra key definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134870/diff/2-3/

Comment 40

[Jan Henning [:JanH]]

Comment on attachment 8865224 [details]
Bug 1359531 - Part 5 - Trigger activity switching from GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134856/diff/2-3/

Comment 41

[Jan Henning [:JanH]]

Comment on attachment 8865225 [details]
Bug 1359531 - Part 6 - Remove manual setting of current activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134860/diff/2-3/

Comment 42

[Jan Henning [:JanH]]

Comment on attachment 8865226 [details]
Bug 1359531 - Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136622/diff/2-3/

Comment 43

[Jan Henning [:JanH]]

Comment on attachment 8865227 [details]
Bug 1359531 - Part 8 - Handle tabs selected while all GeckoApps were in background.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136624/diff/2-3/

Comment 44

[Jan Henning [:JanH]]

Comment on attachment 8865220 [details]
Bug 1359531 - Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134854/diff/3-4/

Comment 45

[Jan Henning [:JanH]]

Comment on attachment 8865221 [details]
Bug 1359531 - Part 2 - Move some tab switch functionality to the tab object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134866/diff/3-4/

Comment 46

[Jan Henning [:JanH]]

Comment on attachment 8865222 [details]
Bug 1359531 - Part 3 - Creating new intents is a job for IntentHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134868/diff/3-4/

Comment 47

[Jan Henning [:JanH]]

Comment on attachment 8865223 [details]
Bug 1359531 - Part 4 - Import Tab intent extra key definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134870/diff/3-4/

Comment 48

[Jan Henning [:JanH]]

Comment on attachment 8865224 [details]
Bug 1359531 - Part 5 - Trigger activity switching from GeckoApp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134856/diff/3-4/

Comment 49

[Jan Henning [:JanH]]

Comment on attachment 8865225 [details]
Bug 1359531 - Part 6 - Remove manual setting of current activity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134860/diff/3-4/

Comment 50

[Jan Henning [:JanH]]

Comment on attachment 8865226 [details]
Bug 1359531 - Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136622/diff/3-4/

Comment 51

[Jan Henning [:JanH]]

Comment on attachment 8865227 [details]
Bug 1359531 - Part 8 - Handle tabs selected while all GeckoApps were in background.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136624/diff/3-4/

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cf774f3e277
Part 1 - Call GeckoApp's tab event handler from web apps/custom tabs, too. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/df8187a1ecc1
Part 2 - Move some tab switch functionality to the tab object. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/8dbc1f7ec748
Part 3 - Creating new intents is a job for IntentHelper. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/46b245b80245
Part 4 - Import Tab intent extra key definitions. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/bd4d35fd55f4
Part 5 - Trigger activity switching from GeckoApp. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/e3decf7e8200
Part 6 - Remove manual setting of current activity. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/ff8fe34e775f
Part 7 - Don't ignore last selected tab for Intent.ACTION_MAIN. r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/b04dbc3924b9
Part 8 - Handle tabs selected while all GeckoApps were in background. r=sebastian,walkingice

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cf774f3e277
https://hg.mozilla.org/mozilla-central/rev/df8187a1ecc1
https://hg.mozilla.org/mozilla-central/rev/8dbc1f7ec748
https://hg.mozilla.org/mozilla-central/rev/46b245b80245
https://hg.mozilla.org/mozilla-central/rev/bd4d35fd55f4
https://hg.mozilla.org/mozilla-central/rev/e3decf7e8200
https://hg.mozilla.org/mozilla-central/rev/ff8fe34e775f
https://hg.mozilla.org/mozilla-central/rev/b04dbc3924b9