1159752 - Visible & selected tab incorrect when tab queue loaded and Fx isn't loaded

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1146589#c13 When Fx is not loaded and several tabs are queued, the visible and selected tab is incorrect.

Adding gmail.com and youtube.com to the tab queue and then selecting "Open
now" for google.com

 1. Fx is opened and only about:home is displayed
=> The loaded order of the tabs is: 
1. about:home (filled Bug 1159260 for this)
2. gmail.com
3. youtube.com with
4. google.com being selected and visible
 
2. Fx is not opened
=> The loaded order of the tabs is: 
1. google.com. 
2. gmail.com 
3. youtube.com being selected and visible

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Visible & selected tab incorrect when tab queue loaded and Fx isn't loaded

Upon closer inspection, we had a bit of a mess on our hands in terms of loading the tab queue on app start.  When Fennec starts, onResume is called before the initialize function in GeckoApp has had a chance to run, so when we were coming in to the app afresh we were trying to load Tab Queue tabs before Gecko had loaded, resulting in some weird behaviour, as described above.  This also explains why the Tabs Panel wasn't opening when there were multiple tabs being opened.

To combat this, I've made the Tab Queue loading in onResume conditional on the app having been initialized.  The way we handle incoming intents is if the app has not been initialized already then it's handled in the initialize method, otherwise new intents are handled in the onNewIntent method.  

I've added some code in to the initialize method in GeckoApp to handle processing the tab queue.  To accommodate this, the processTabQueue and openQueuedTabs functions have been moved verbatim from BrowserApp to GeckoApp.  

The code which opens the queued tabs has been refactored from the onNewIntent function in GeckoApp to accommodate also being called from the initialize function.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8604188 [details] [diff] [review]
Visible & selected tab incorrect when tab queue loaded and Fx isn't loaded

Review of attachment 8604188 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ nits & questions answered.

(In reply to Martyn Haigh (:mhaigh) from comment #1)
> To combat this, I've made the Tab Queue loading in onResume conditional on
> the app having been initialized.

This patch didn't change the onResume behavior. Shouldn't processTabQueue just be removed from onResume? I would think the initialize and onNewIntent case would cover it.

Also, I dislike that the tab queue code is being moved into GeckoApp even though it's specific to the browser - I don't suppose you could add an abstract method to do the handling instead?

That being said, everything else looks reasonable.

::: mobile/android/base/GeckoApp.java
@@ +1828,5 @@
>  
> +
> +    protected void processActionViewIntent(final Runnable openTabsRunnable) {
> +        // We need to ensure that if we receive a VIEW action and there are tabs queued then the
> +        // site loaded from the intent is op top (last loaded) and selected with all other tabs

op top?

@@ +1831,5 @@
> +        // We need to ensure that if we receive a VIEW action and there are tabs queued then the
> +        // site loaded from the intent is op top (last loaded) and selected with all other tabs
> +        // being opened behind it. We process the tab queue first and request a callback from the JS - the
> +        // listener will open the url from the intent as normal when the tab queue has been processed.
> +        ThreadUtils.postToBackgroundThread(new Runnable() {

Why is this on a background thread?

Comment 3

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/b98dbd42d4d3

Comment 4

[Martyn Haigh (:mhaigh)]

Oops - that was the wrong change, it's been backed out - please ignore!

Comment 5

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/c8d39e33cc50

Comment 6

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/a4a0e18e4940

Comment 7

[Martyn Haigh (:mhaigh)]

> 
> (In reply to Martyn Haigh (:mhaigh) from comment #1)
> > To combat this, I've made the Tab Queue loading in onResume conditional on
> > the app having been initialized.
> 
> This patch didn't change the onResume behavior. Shouldn't processTabQueue
> just be removed from onResume? I would think the initialize and onNewIntent
> case would cover it.

It adds a conditional to the processTabQueue called from onResume to depend on mInitialized, defined in GeckoApp.  This means that if we are in a cold start, onResume is called before initialize, so processTabQueue won't run and instead the TQ will be processed in the initialize method, however if Fx was in the background then onResume will successfully call processTabQueue.

> 
> Also, I dislike that the tab queue code is being moved into GeckoApp even
> though it's specific to the browser - I don't suppose you could add an
> abstract method to do the handling instead?

If we add them as abstract methods then WebappImp will have to implement them, which isn't great.  I've put them as empty methods which BrowserApp overrides.

> 
> @@ +1831,5 @@
> > +        // We need to ensure that if we receive a VIEW action and there are tabs queued then the
> > +        // site loaded from the intent is op top (last loaded) and selected with all other tabs
> > +        // being opened behind it. We process the tab queue first and request a callback from the JS - the
> > +        // listener will open the url from the intent as normal when the tab queue has been processed.
> > +        ThreadUtils.postToBackgroundThread(new Runnable() {
> 
> Why is this on a background thread?

Because we're doing file IO in the openQueuedUrls method at the end of that runnable.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a4a0e18e4940

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Martyn Haigh (:mhaigh) from comment #7)
> Because we're doing file IO in the openQueuedUrls method at the end of that
> runnable.

But only if tab queue is enabled and we actually need to unqueue tabs, right? So we should only use the runnable for openQueuedUrls to conserve resources. Follow-up?

Comment 10

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 41.0a1 (2015-05-17)

Verified as fixed using steps from description:
Add gmail.com and youtube.com to the tab queue and "Open now" for google.com

 1. Fx is opened and only about:home is displayed
=> The loaded order of the tabs is: 
1. about:home 
2. gmail.com
3. youtube.com 
4. google.com is selected and visible
 
2. Fx is not opened
=> The loaded order of the tabs is: 
1. gmail.com 
2. youtube.com 
3. google.com is selected and visible

Comment 11

[Martyn Haigh (:mhaigh)]

(In reply to Michael Comella (:mcomella) from comment #9)
> (In reply to Martyn Haigh (:mhaigh) from comment #7)
> > Because we're doing file IO in the openQueuedUrls method at the end of that
> > runnable.
> 
> But only if tab queue is enabled and we actually need to unqueue tabs,
> right? So we should only use the runnable for openQueuedUrls to conserve
> resources. Follow-up?

Actually we also need it for the shouldOpenTabQueueUrls method in the initial conditional statement - so I don't think this needs following up.