1108426 - Extend RemoteTab to support URL history

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Richard Newman [:rnewman]]

Android has always included URL history. We now get it from desktop, too. Let's include it in RemoteTab and use it in the tabs panel.

Comment 1

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

I was thinking about this last night.  Do you mean surfacing a tab's history for incoming tabs sent to or synced to the Android device?  Can you improve the "friendliness" of this mentored bug and be more explicit about what you want to see?  Code pointers to the relevant places would help too.

Comment 2

[Richard Newman [:rnewman]]

Synced to, not sent to. Unfortunately we can't include any history in send tab commands.

We download 'tabs' records from the Sync server, storing them in TabsProvider.

Those records have an array of URLs, which historically for desktop has only contained a single entry. This is split into the TabsProvider schema as two fields:

                       Tabs.URL + " TEXT," +
                       Tabs.HISTORY + " TEXT," +

and in Tab.java:

  public ContentValues toContentValues(String clientGUID, int position) {
    ContentValues out = new ContentValues();
...
    out.put(BrowserContract.Tabs.URL,       (String) this.history.get(0));
    out.put(BrowserContract.Tabs.HISTORY,   this.history.toJSONString());


The database now will contain full history for both desktop and mobile. The UI doesn't reflect that, though.

Fixing this bug involves threading the full history into each RemoteTab instance (TabsAccessor.getClientsFromCursor), then updating the RemoteTabs* UI code to be more sophisticated than this:

RemoteTabsExpandableListFragment.java#136:

                // This item is a TwoLinePageRow, so we allow switch-to-tab.
                mUrlOpenListener.onUrlOpen(tab.url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));


The end result is that you can open a URL from your desktop, and hit 'back' to navigate through the list of pages that got you there on the other device.

Comment 3

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

> The end result is that you can open a URL from your desktop, and hit 'back'
> to navigate through the list of pages that got you there on the other device.

I'm really not convinced we want this.  It's strange to go from one context (Remote Tabs panel) to another (the opened URL) and have "back" not do what you would expect, which is to return to your old context.  We struggle with this in many places and I'm not aware of any over-arching UX approach.  (Would love to be corrected there.)

I think there's value in making sure that "onUrlOpen" and friends can handle URL histories, but I don't support doing this when following links from a home panel.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #3)

> I'm really not convinced we want this.  It's strange to go from one context
> (Remote Tabs panel) to another (the opened URL) and have "back" not do what
> you would expect, which is to return to your old context.  We struggle with
> this in many places and I'm not aware of any over-arching UX approach. 
> (Would love to be corrected there.)

I think that depends on the framing of remote tabs. If it's kinda like zooming into the tab that's open on your other device, and we present it as resuming a back stack -- even with animation -- I think it makes sense. If we frame it as "open this current URL", then sure, it would be confusing.

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: 1108426.patch

Tabs from synced tabs panel supports history.
* converted history json string from remote tabs to the format supported by sessionstore.js

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I am not so sure we ever want to display the session history of a Tab in the Synced Tabs UI. That's a lot of data. Also, we're not actually capable of showing the true session history WRT back/forward, since we don't send the "index" of the URL currently displayed in the Tab. Unless what's really sent via the TabsProvider is the "back" list of URLs, but even then, it's too much for people to grasp and is just going to cause confusion.

If we want to try to auto-populate those URLs into the session history of a Synced Tab when opened, that's a different story. It also fits our model better: An open Tab can have history.

Comment 7

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> I am not so sure we ever want to display the session history of a Tab in the
> Synced Tabs UI. That's a lot of data. Also, we're not actually capable of
> showing the true session history WRT back/forward, since we don't send the
> "index" of the URL currently displayed in the Tab. Unless what's really sent
> via the TabsProvider is the "back" list of URLs, but even then, it's too
> much for people to grasp and is just going to cause confusion.
> 
> If we want to try to auto-populate those URLs into the session history of a
> Synced Tab when opened, that's a different story. It also fits our model
> better: An open Tab can have history.

A wild mfinkle appears!  Your concern and suggestion are what we're aiming for.  This ticket tracks shuttling tab history into and out of Sync, and then making sure we configure the tab history when a remote tab is opened from the Remote Tabs panel.  It's not about displaying that history in the panel itself.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8550852 [details] [diff] [review]
1108426.patch

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

Vivek: this looks like a Part 1 for this bug. Is there a Part 2 coming?

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +182,5 @@
>          // children. Therefore, we must handle one case manually.
>          if (view instanceof TwoLinePageRow) {
> +            TwoLinePageRow twoLinePage = (TwoLinePageRow) view;
> +            twoLinePage.update(tab.title, tab.url);
> +            // Suppress switch to tab for this view.

Nit: newline before comment.

Comment 9

[Vivek Balakrishnan[:vivek]]

@rnewman: yes, I seem to have uploaded my initial patch. I'll update a new patch today after rebasing.

Comment 10

[Vivek Balakrishnan[:vivek]]

Attachment: 1108426.patch

New patch with following changes
* extracted Session:RestoreRecentTabs to an interface.
* Used the above interface to load remote tabs with history

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8556514 [details] [diff] [review]
1108426.patch

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

In the absence of lucasr or sriram, I'm going to punt this to the next two most culpable folks, Wes and Margaret, to take a look at parts of this.

Could one of you two assess the BrowserApp changes, naming, etc.?

::: mobile/android/base/BrowserApp.java
@@ +3181,5 @@
>              }
>          }).execute();
>      }
>  
> +    // HomePager.onRestoreTabsHistory

This comment should be "HomePager.OnRestoreTabsHistoryListener", if you're following that style. The method name is obvious!

::: mobile/android/base/home/HomeFragment.java
@@ +66,5 @@
>  
>      // Whether the fragment has loaded its content
>      private boolean mIsLoaded;
>  
> +    // On restore tabs history listener.

Remove this.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +133,5 @@
>                  }
>  
>                  Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);
>  
> +                restoreTabsHistoryListener.onRestoreTabsHistory(convertToSessionJson(tab.history));

s/Json/JSON

You probably want to error-check the result before calling the listener, too.

@@ +182,5 @@
>          registerForContextMenu(mList);
>      }
>  
> +    /**
> +     * converts array of URLs JSON to session restore JSON.

s/converts/Converts

@@ +184,5 @@
>  
> +    /**
> +     * converts array of URLs JSON to session restore JSON.
> +     *
> +     * The JSON format follows the format used in Gecko for session restore .

Nit: extra space.

@@ +189,5 @@
> +     *
> +     * @param urlJson array of history URLs from remote client as JSON string
> +     * @return list of JSON string
> +     */
> +    private List<String> convertToSessionJson(String urlJson) {

urlJSON (and change javadoc, too)

@@ +210,5 @@
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSON error", e);
> +        }
> +
> +        sessionJsonList.add(tabData.toString());

This seems like a startlingly wasteful transformation. Can we do better than building a new string -- e.g., passing the upstream format across the bridge, and mutating the JS object directly on the other side?

Comment 12

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

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html

Comment 13

[:Margaret Leibovic]

Comment on attachment 8556514 [details] [diff] [review]
1108426.patch

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

The overall approach here looks fine to me, but I am a bit confused about exactly what data is getting passed around in these methods. At the very least, we should document what's expected in these parameters.

::: mobile/android/base/BrowserApp.java
@@ +3191,5 @@
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSON error", e);
> +        }
> +
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Session:RestoreRecentTabs", json.toString()));

Nice, it's handy that we already have this message we can use.

::: mobile/android/base/home/HomePager.java
@@ +102,5 @@
>          public void onUrlOpenInBackground(String url, EnumSet<Flags> flags);
>      }
>  
> +    public interface OnRestoreTabsHistoryListener {
> +        public void onRestoreTabsHistory(List<String> historyList);

I would just call this 'OnRestoreTabsListener' and make the method 'onRestoreTabs'. Restoring a tab should always restore its session history, it's just a bug that we don't already do that :)

I think we should also rename the parameter here, since if I understand things correctly, this is a list of tab data strings, which includes session history among other things. The name 'historyList' makes it sound like this is just a list of session history URLs, which it's not.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8556514 [details] [diff] [review]
1108426.patch

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

I think margaret got to this for me. :)

Comment 15

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.