1146589 - Ensure that tab opened with "open now" action is selected and visible when fennec loads

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

When a user has added some tabs to the tab queue and then uses the "open now" action of the toast, the site that they wanted to "Open now" isn't currently visible when Fennec loads.

For example, adding sites A and B to the tab queue and then selecting "Open now" on the toast for site C will result in the tab order in the tabs panel of C, A, B with B being the selected and visible tab.  The expected loaded order of the tabs should be A, B, C with C being selected and visible

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Part 1: Add JavaScript callback

If we pass a performCallback value of true to the JavaScript, get it to call back to the Java with a message of Tabs:TabsOpened.  This is so that we can first load the tab queue tabs and then load the data passed in to our onNewIntent method later to preserve the correct display order of tabs

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Part 2: Pass callback flag to JavaScript and adjust Java method signature

Add a new parameter to the openQueuedUrls method to allow us to specify if we want a callback from the JavaScript.  Adjust the existing method calls to accommodate the new method signature.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Part 3: Adjust ACTION_VIEW code to process tab queue data before the intent data

Add a check to the onNewIntent method, when receiving an ACTION_VIEW intent, see if we have tabs queued and if so then delay opening the passed in url from the intent.

Comment 4

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

Comment on attachment 8582616 [details] [diff] [review]
Part 1: Add JavaScript callback

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

::: mobile/android/components/SessionStore.js
@@ +164,3 @@
>          this._openTabs(data);
> +
> +        if(data.performCallback) {

I think performCallback could be more specific as to what the callback does.

e.g. -> data.shouldNotifyTabsOpenedToJava

nit: `if (`

Comment 5

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

Comment on attachment 8582618 [details] [diff] [review]
Part 2: Pass callback flag to JavaScript and  adjust Java method signature

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

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +127,4 @@
>          }
>      }
>  
> +    static public void openQueuedUrls(final Context context, final GeckoProfile profile, final String filename, boolean performJavaScriptCallback) {

Just a tip: as a reviewer, it's easier if the micro commits also include more context - i.e. where is this new argument used? I'd personally prefer a larger patch that tells me why this method argument is being added so I don't have to bounce around too much.

Comment 6

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

Comment on attachment 8582622 [details] [diff] [review]
Part 3: Adjust ACTION_VIEW code to process tab  queue data before the intent data

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

Provided JS lets us, I think this would be more readable if we passed the final tab to JS too. We could either:
  * (preferred) Put it into the tab queue on the side necessary to have it opened and selected last
  * Pass it as a special argument in the JSON array to JS (e.g. data.tabToBeSelected)

r+ because this seems to work, but I'd prefer ^.

::: mobile/android/base/GeckoApp.java
@@ +1828,4 @@
>              String uri = intent.getDataString();
>              Tabs.getInstance().loadUrl(uri);
>          } else if (Intent.ACTION_VIEW.equals(action)) {
> +            // We need to ensure that if we receive a VIEW action and there are tabs queued then site loaded from the

nit: then the site loaded...

@@ +1831,5 @@
> +            // We need to ensure that if we receive a VIEW action and there are tabs queued then site loaded from the
> +            // intent is visible 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.
> +            if (AppConstants.NIGHTLY_BUILD && AppConstants.MOZ_ANDROID_TAB_QUEUE && TabQueueHelper.shouldOpenTabQueueUrls(this)) {

nit: This line seems crazy long. I like 100 characters per line in Java. It helps when putting two editor windows side-by-side and when reviewing code through Splinter.

@@ +1843,5 @@
> +                                                                    Tabs.LOADURL_USER_ENTERED |
> +                                                                    Tabs.LOADURL_EXTERNAL);
> +                        }
> +                    }
> +                }, "Tabs:TabsOpened");

nit: newline below this otherwise it's hard to pull out the event we're listening for.

Comment 7

[Martyn Haigh (:mhaigh)]

I've filed bug 1148380 to track the optimisation of the code to remove the callback - agreed it's a better way of doing it, but I'm eager to get code landed atm so will land in current state and come back to cleanup later.

Comment 8

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff02e43180eb

Comment 9

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf05536b2a8c

Comment 10

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58825b39277c

Comment 11

[Martyn Haigh (:mhaigh)]

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

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Samsung S5 (Android 4.4)
Build: Firefox for Android 40.0a1 (2015-04-22)

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

1. Nightly is opened and only about:home is displayed
=> The loaded order of the tabs is: about:home, gmail.com, youtube.com with google.com being selected and visible

2. Nightly is not opened
=> The loaded order of the tabs is: google.com. gmail.com and youtube.com being selected and visible

Comment 13

[Teodora Vermesan (:TeoVermesan)]

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

 1. Nightly 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. Nightly is not opened
=> The loaded order of the tabs is: 
1. google.com. 
2. gmail.com 
3. youtube.com being selected and visible

Is it expected the loaded order of the tabs to be this when Nightly is not openend?

Comment 14

[Martyn Haigh (:mhaigh)]

I've added https://bugzilla.mozilla.org/show_bug.cgi?id=1159752 to track the shown tab not being correct when loading tab queue tabs without Fx being open.