Closed Bug 1146325 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(3 files, 4 obsolete files)

Once tab queue data is loaded in fennec, convert to the required format and send to gecko to load all the tabs at once.
Add method to open multiple tabs from gecko and an observer to listen for events.
Attachment #8581687 - Flags: review?(margaret.leibovic)
Attached patch Part 2: cleanup code and imports (obsolete) — Splinter Review
A little bit of non-destructive housekeeping - nothing to see here, move on.
Attachment #8581689 - Flags: review?(margaret.leibovic)
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.
Attachment #8581692 - Flags: review?(margaret.leibovic)
Oops - submitted too early.  That last patch also sends an event to Gecko with the processed data.
Last Part 3 patch had the wrong JSONObject import, this fixes that.
Attachment #8581692 - Attachment is obsolete: true
Attachment #8581692 - Flags: review?(margaret.leibovic)
Attachment #8581876 - Flags: review?(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.
Attachment #8581687 - Flags: review?(margaret.leibovic) → feedback+
Attachment #8581689 - Flags: review?(margaret.leibovic) → review+
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 :)
Attachment #8581876 - Flags: review?(margaret.leibovic) → feedback+
Now hard-coding the additional params in JS and reworked the JS to reflect the simpler data structure.
Attachment #8581687 - Attachment is obsolete: true
Attachment #8583098 - Flags: review?(margaret.leibovic)
Some additional code cleanup - non-destructive still.
Attachment #8581689 - Attachment is obsolete: true
Attachment #8583100 - Flags: review?(margaret.leibovic)
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.
Attachment #8581876 - Attachment is obsolete: true
Attachment #8583101 - Flags: review?(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.
Attachment #8583100 - Flags: review?(margaret.leibovic) → review+
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 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.
Attachment #8583098 - Flags: review?(margaret.leibovic) → review+
Attachment #8583101 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/58b3c3a8e8a8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.