1146325 - Convert loaded tab queue data to the required format and send to gecko to open

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

Once tab queue data is loaded in fennec, convert to the required format and send to gecko to load all the tabs at once.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Part 1: Add JavaScript observer and method to open multiple tabs

Add method to open multiple tabs from gecko and an observer to listen for events.

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Part 2: cleanup code and imports

A little bit of non-destructive housekeeping - nothing to see here, move on.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Part 3: Process tab queue data and pass through to Gecko

Process the JSON data from the loaded tab queue file to get it in to the correct format as expected by our JavaScript _openTabs function.

Comment 4

[Martyn Haigh (:mhaigh)]

Oops - submitted too early.  That last patch also sends an event to Gecko with the processed data.

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Part 3: Process tab queue data and pass through to Gecko

Last Part 3 patch had the wrong JSONObject import, this fixes that.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8581687 [details] [diff] [review]
Part 1: Add JavaScript observer and method to  open multiple tabs

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

::: mobile/android/components/SessionStore.js
@@ +926,5 @@
> +        isPrivate: tabData.isPrivate,
> +        desktopMode: tabData.desktopMode,
> +      };
> +
> +      let tab = window.BrowserApp.addTab(tabData.url, params);

This logic looks very similar to this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#914

I think we should factor this out into a shared function.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8581876 [details] [diff] [review]
Part 3: Process tab queue data and pass through to Gecko

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

Looking good, but I think we can make this simpler.

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +145,5 @@
> +
> +            for (int i = 0; i < jsonArray.length(); i++) {
> +                String site;
> +                try {
> +                    site = jsonArray.getString(i);

Nit: Maybe call this `url` instead of `site`, since it looks like that's what it is.

@@ +156,5 @@
> +
> +                    try {
> +                        jsonObject.put("url", site);
> +                        jsonObject.put("isPrivate", false);
> +                        jsonObject.put("desktopMode", false);

I would assume these two states are the default, so we should be able to omit this from a message sent to JS. Or if not, we can specify them as the default on the JS side.

And if that's the case, we can get rid of this JSONObject business and just pass an array of URLs directly to JS.

This would mean that we actually wouldn't share the JS code from the first patch, but I'm okay with us having a separate function implementation if it's much simpler :)

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Part 1: Add JavaScript observer and method to open multiple tabs

Now hard-coding the additional params in JS and reworked the JS to reflect the simpler data structure.

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Part 2: cleanup code and imports

Some additional code cleanup - non-destructive still.

Comment 10

[Martyn Haigh (:mhaigh)]

Attachment: Part 3: Process tab queue data and pass through to Gecko

Simplify the data we're sending to gecko. Note that we're still using the JSONObject here, instead of just passing the JSONArray through because we'll rely on it for a future patch when we have to pass additional params through to the JS.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8583098 [details] [diff] [review]
Part 1: Add JavaScript observer and method to  open multiple tabs

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

::: mobile/android/components/SessionStore.js
@@ +919,5 @@
> +  // This function iterates through a list of urls opening a new tab for each.
> +  _openTabs: function ss_openTabs(aData) {
> +    let window = Services.wm.getMostRecentWindow("navigator:browser");
> +    for (let i = 0; i < aData.urls.length; i++) {
> +      let url = aData.urls[i];

I think we should just replace aData with an array of urls.

_openTabs: function(urls) {
 ...
}

Then we can just pass a JSONArray directly from Java in the "Tabs:OpenMutliple" message.

@@ +923,5 @@
> +      let url = aData.urls[i];
> +      let params = {
> +        selected: (i == aData.urls.length - 1),
> +        isPrivate: false,
> +        desktopMode: false,

We don't even need to include these, since the default is false for these:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3366
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3402

I don't see this default ever changing, so we don't need to worry about this.

@@ +926,5 @@
> +        isPrivate: false,
> +        desktopMode: false,
> +      };
> +
> +      let tab = window.BrowserApp.addTab(url, params);

Nit: No need to declare a local variable here, since it's not used.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8583101 [details] [diff] [review]
Part 3: Process tab queue data and pass through  to Gecko

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

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +131,5 @@
>          JSONArray jsonArray = profile.readJSONArrayFromFile(filename);
> +        if (jsonArray.length() > 0) {
> +            JSONObject data = new JSONObject();
> +            try {
> +                data.put("urls", jsonArray);

As I mentioned in my last comment, I don't think we even need to wrap this array in an object, we can just pass the array directly.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8583098 [details] [diff] [review]
Part 1: Add JavaScript observer and method to  open multiple tabs

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

Ah, just noticed comment 10, ignore my comments about directly passing a urls array.

r+ with other comments addressed.

Comment 14

[Martyn Haigh (:mhaigh)]

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

Comment 15

[Martyn Haigh (:mhaigh)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58b3c3a8e8a8