1004850 - List recently closed tabs in the UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This is an enhancement beyond bug 701725.

It looks like we keep track of recently closed tabs in session store, and I think it could be nice to show those somewhere in the UI, similarly to how desktop has a place in the menu for that. What about putting them somewhere in the history panel? Perhaps we can enhance the "tabs from last time" panel to also show recently closed tabs.

Comment 1

[Ian Barlow (:ibarlow)]

Yeah that would be nice. I wonder if Recent Tabs is a thing that might merit its own panel, with this increased functionality. So a list that might read something like:

RECENT TABS
---------------------------------------
Recently Closed
---------------------------------------
Tab
Tab
Tab
Open all
---------------------------------------
Tabs from last time
---------------------------------------
Tab
Tab
Tab
Open all

Comment 2

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

Note that there could be privacy implications around clearing history: most people aren't going to think to clear their history in two places, especially when the recent tabs page hard to discover.

Comment 3

[:Margaret Leibovic]

I imagine clearing history would also clear session history (or if it doesn't it should). This recent tabs list would just get pulled from session store, so it would hopefully be empty if the user cleared their history.

Comment 4

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

(In reply to :Margaret Leibovic from comment #3)
> I imagine clearing history would also clear session history (or if it
> doesn't it should). This recent tabs list would just get pulled from session
> store, so it would hopefully be empty if the user cleared their history.

SessionStore is cleared when the history is purged:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#144
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#94

Comment 5

[:Margaret Leibovic]

Tentatively taking for now.

Comment 6

[:Margaret Leibovic]

I have a whole series of patches here that adds a "Recent Tabs" panel to about:home, which lists recently closed tabs and tabs from last time.

There are a few more patches to write, but I wanted to start getting review on the ones I have. I'm thinking of splitting these issues into separate bugs, but I can land them together if we don't want to land this new panel with about these features. The things left to handle are:

* Add "Open all" items
* Listen for changes to the set of recently closed tabs (right now we just get the closed tabs once, when we load the panel view)
* Actually restore tabs, rather than just load URL (this is a problem with the current "Tabs from last time" panel, so I split this off into bug 1012739)

Comment 7

[:Margaret Leibovic]

Attachment: (Part 1) Replace HistoryPanel with MostRecentPanel

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2) Rename MostRecentPanel to HistoryPanel

Comment 9

[:Margaret Leibovic]

Attachment: (Part 3) Create RecentTabsPanel from existing LastTabsPanel

Comment 10

[:Margaret Leibovic]

Attachment: (Part 4) Update RecentTabsAdapter to be a MultiTypeCursorAdapter

Comment 11

[:Margaret Leibovic]

Attachment: (Part 5) Add recently closed tabs to RecentTabsPanel

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8424919 [details] [diff] [review]
(Part 1) Replace HistoryPanel with MostRecentPanel

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

Nice. I'd prefer MostRecentPanel to actually become HistoryPanel to keep things nice and tidy.

::: mobile/android/base/resources/layout-xlarge-v11/home_history_panel.xml
@@ -9,5 @@
> -              android:layout_height="fill_parent">
> -
> -    <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget"
> -                                            style="@style/Widget.Home.HistoryTabWidget"
> -                                            android:layout_width="@dimen/history_tab_widget_width"

Please audit this patch for other resources that become unused after this change. For example, I guess history_tab_widget_width/history_tab_widget_height are not used anywhere else.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8424920 [details] [diff] [review]
(Part 2) Rename MostRecentPanel to HistoryPanel

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

Ah, awesome, thanks :-)

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8424921 [details] [diff] [review]
(Part 3) Create RecentTabsPanel from existing LastTabsPanel

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

Great. One thing to investigate: could you change something in about:home settings (e.g. hide the reading list or something), then apply your patches and see if the 'RecentTabs' panel show up in Home settings? My guess is that it won't up. Maybe this will be our first change that will require a HomeConfig migration or sorts...

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8424922 [details] [diff] [review]
(Part 4) Update RecentTabsAdapter to be a MultiTypeCursorAdapter

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

Slightly mixed about how you're figuring out where to place the headers. Have you considered doing something similar to MostRecentPanel?

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8424923 [details] [diff] [review]
(Part 5) Add recently closed tabs to RecentTabsPanel

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

Looking pretty good. The way header handling is actually not too bad. Needs some thread-safety fixes before getting a r+.

::: mobile/android/app/mobile.js
@@ +120,5 @@
>  /* session store */
>  pref("browser.sessionstore.resume_session_once", false);
>  pref("browser.sessionstore.resume_from_crash", true);
>  pref("browser.sessionstore.interval", 10000); // milliseconds
> +pref("browser.sessionstore.max_tabs_undo", 10);

Any rationale for this number? Seems a bit too high. Maybe we should start with 5 or something? Up to you.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +67,5 @@
>      // On new tabs listener
>      private OnNewTabsListener mNewTabsListener;
>  
> +    // Recently closed tabs from gecko
> +    private static JSONArray mRecentlyClosedTabs;

This shouldn't be static.

@@ +192,5 @@
> +                mRecentlyClosedTabs = message.getJSONArray("tabs");
> +
> +                // Reload the cursor to show recently closed tabs.
> +                if (mRecentlyClosedTabs.length() > 0 && canLoad()) {
> +                    load();

I assume the reason you're not using loadIfVisible() here is that you want to force a load() call even if the fragment has already loaded its contents (i.e. mIsLoaded == true)?

This code is running in the Gecko thread, you should move it all to the UI thread as you're touching non-thread-safe bits of HomeFragment here. Also, you should only update mRecentlyClosedTabs in the UI thread to avoid the complexity from concurrency here.

@@ +239,5 @@
>                                                                     RecentTabs.URL,
>                                                                     RecentTabs.TITLE,
>                                                                     RecentTabs.TYPE });
>  
> +            if (mRecentlyClosedTabs != null) {

Maybe move the length check up here?

if (mRecentlyClosedTabs != null && mRecentlyClosedTabs.length() > 0) {
   ...
}

@@ +251,5 @@
> +                        final JSONObject tab = mRecentlyClosedTabs.getJSONObject(i);
> +                        final String url = tab.getString("url");
> +
> +                        // Don't show recent tabs for about:home
> +                        if (!AboutPages.isAboutHome(url)) {

Maybe exclude all about: pages?

Comment 17

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 8424919 [details] [diff] [review]
> (Part 1) Replace HistoryPanel with MostRecentPanel
> 
> Review of attachment 8424919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. I'd prefer MostRecentPanel to actually become HistoryPanel to keep
> things nice and tidy.

I did it in steps like this because I found it easier to follow the diff.

> ::: mobile/android/base/resources/layout-xlarge-v11/home_history_panel.xml
> @@ -9,5 @@
> > -              android:layout_height="fill_parent">
> > -
> > -    <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget"
> > -                                            style="@style/Widget.Home.HistoryTabWidget"
> > -                                            android:layout_width="@dimen/history_tab_widget_width"
> 
> Please audit this patch for other resources that become unused after this
> change. For example, I guess
> history_tab_widget_width/history_tab_widget_height are not used anywhere
> else.

Good catch, there's actually a decent amount more to remove... I'll post an updated patch.

Comment 18

[:Margaret Leibovic]

Attachment: (Part 1 v2) Replace HistoryPanel with MostRecentPanel

Comment 19

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Comment on attachment 8424921 [details] [diff] [review]
> (Part 3) Create RecentTabsPanel from existing LastTabsPanel
> 
> Review of attachment 8424921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great. One thing to investigate: could you change something in about:home
> settings (e.g. hide the reading list or something), then apply your patches
> and see if the 'RecentTabs' panel show up in Home settings? My guess is that
> it won't up. Maybe this will be our first change that will require a
> HomeConfig migration or sorts...

Good catch. Yes, you're right that the new panel doesn't show up, and it doesn't even show up as an option in settings. It will be interesting to figure out how we want to do this migration, since users could have rearranged the panels. Maybe we can just insert the recent tabs panel before or after the history panel (depending on phone/tablet), wherever it is in the order. Or always add it as the first panel on phones and the last panel on tablets.

Comment 20

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #16)
> Comment on attachment 8424923 [details] [diff] [review]
> (Part 5) Add recently closed tabs to RecentTabsPanel
> 
> Review of attachment 8424923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking pretty good. The way header handling is actually not too bad. Needs
> some thread-safety fixes before getting a r+.
> 
> ::: mobile/android/app/mobile.js
> @@ +120,5 @@
> >  /* session store */
> >  pref("browser.sessionstore.resume_session_once", false);
> >  pref("browser.sessionstore.resume_from_crash", true);
> >  pref("browser.sessionstore.interval", 10000); // milliseconds
> > +pref("browser.sessionstore.max_tabs_undo", 10);
> 
> Any rationale for this number? Seems a bit too high. Maybe we should start
> with 5 or something? Up to you.

It's 10 on desktop. We can start with 5, but I think holding onto the session history of 10 recently closed tabs wouldn't be so bad, since some users can have a much higher number of tabs than that. But yeah, probably best to watch out for any memory regressions.

> ::: mobile/android/base/home/RecentTabsPanel.java
> @@ +67,5 @@
> >      // On new tabs listener
> >      private OnNewTabsListener mNewTabsListener;
> >  
> > +    // Recently closed tabs from gecko
> > +    private static JSONArray mRecentlyClosedTabs;
> 
> This shouldn't be static.

This is probably a sign that I'm doing something wrong, but I couldn't access this member inside RecentTabsCursorLoader unless it was static. Perhaps storing this state on RecentTabsPanel isn't the right thing to do.

> @@ +192,5 @@
> > +                mRecentlyClosedTabs = message.getJSONArray("tabs");
> > +
> > +                // Reload the cursor to show recently closed tabs.
> > +                if (mRecentlyClosedTabs.length() > 0 && canLoad()) {
> > +                    load();
> 
> I assume the reason you're not using loadIfVisible() here is that you want
> to force a load() call even if the fragment has already loaded its contents
> (i.e. mIsLoaded == true)?

Yes, I was following the same pattern as the DynamicPanel panel layout loading logic.

> This code is running in the Gecko thread, you should move it all to the UI
> thread as you're touching non-thread-safe bits of HomeFragment here. Also,
> you should only update mRecentlyClosedTabs in the UI thread to avoid the
> complexity from concurrency here.

Good catch.

> @@ +251,5 @@
> > +                        final JSONObject tab = mRecentlyClosedTabs.getJSONObject(i);
> > +                        final String url = tab.getString("url");
> > +
> > +                        // Don't show recent tabs for about:home
> > +                        if (!AboutPages.isAboutHome(url)) {
> 
> Maybe exclude all about: pages?

I was just being consistent with out current logic. I think it makes sense to exclude about:home, since this UI is technically showing up on about:home, but a user might want to reopen another about: page. However, we do exclude them from history, so maybe this makes sense.

Comment 21

[:Margaret Leibovic]

Comment on attachment 8424923 [details] [diff] [review]
(Part 5) Add recently closed tabs to RecentTabsPanel

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

::: mobile/android/chrome/content/browser.js
@@ +8360,5 @@
> +          type: "RecentlyClosedTabs:Data",
> +          tabs: tabs
> +        });
> +        break;
> +      }

I just realized this logic could go in SessionStore.js. I think that would probably make more sense, and it could also allow us to send messages when the closed tabs list is updated.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8425863 [details] [diff] [review]
(Part 1 v2) Replace HistoryPanel with MostRecentPanel

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

Awesome, I love patches that remove a lot of code :-)

Comment 23

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

(In reply to :Margaret Leibovic from comment #20)

> > > +    // Recently closed tabs from gecko
> > > +    private static JSONArray mRecentlyClosedTabs;
> > 
> > This shouldn't be static.
> 
> This is probably a sign that I'm doing something wrong, but I couldn't
> access this member inside RecentTabsCursorLoader unless it was static.
> Perhaps storing this state on RecentTabsPanel isn't the right thing to do.

Can't we let SessionStore.js handle the state and we just send messages to it?

Comment 24

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #23)
> (In reply to :Margaret Leibovic from comment #20)
> 
> > > > +    // Recently closed tabs from gecko
> > > > +    private static JSONArray mRecentlyClosedTabs;
> > > 
> > > This shouldn't be static.
> > 
> > This is probably a sign that I'm doing something wrong, but I couldn't
> > access this member inside RecentTabsCursorLoader unless it was static.
> > Perhaps storing this state on RecentTabsPanel isn't the right thing to do.
> 
> Can't we let SessionStore.js handle the state and we just send messages to
> it?

Yes, that's what we're doing, but the cursor loader doesn't listen for messages itself, so that data needs some intermediary home that the cursor loader can read from. We also don't want to block on gecko to load the cursor during startup, since it can get tabs from last time without gecko running.

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #20)
> This is probably a sign that I'm doing something wrong, but I couldn't
> access this member inside RecentTabsCursorLoader unless it was static.
> Perhaps storing this state on RecentTabsPanel isn't the right thing to do.

Loaders cannot share state with the fragment/activity that owns it because of the way their life-cycle is managed (in the LoaderManager). But loader callbacks can share state with the loader owner. So, simply pass mRecentlyClosedTabs as a constructor argument in onCreateLoader().

Comment 26

[:Margaret Leibovic]

Attachment: (Part 5 v2) Add recently closed tabs to RecentTabsPanel

Also flagging bnicholson to look at the session restore stuff.

I updated this patch to listen for changes to the closedTabs array in Java. This is ready for the future when we actually support undoing closed tabs, and in fact a recently closed tab will disappear from the list if you use my undo closed tabs add-on (although I need to update that add-on to support there being more than one closed tab to undo, and it could actually flip this pref, too...).

Comment 27

[:Margaret Leibovic]

Attachment: (Part 6) Add HomeConfigPrefsBackend migration to add recent tabs panel

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 8427369 [details] [diff] [review]
(Part 5 v2) Add recently closed tabs to RecentTabsPanel

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

Looking good but needs some final tweaks. I'll leave the review of the SessionStore.js changes to bnicholson.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +183,5 @@
>      }
>  
>      @Override
>      protected void load() {
> +        getLoaderManager().restartLoader(LOADER_ID_RECENT_TABS, null, mCursorLoaderCallbacks);

This will force the loader to refresh its contents on device rotation which is not ideal. Leave load() as is and call restartLoader() only for the ClosedTabs:Data case.

That actually reminds me that you should override onConfigurationChanged() in RecentTabsPanel to force the UI to refresh for different orientations on tablets. The code should be same than we have in, say, ReadingListPanel, TopSitesPanel, etc.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 8427427 [details] [diff] [review]
(Part 6) Add HomeConfigPrefsBackend migration to add recent tabs panel

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

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +39,5 @@
> +    // Increment this to trigger a migration.
> +    private static final int VERSION = 1;
> +
> +    // The prefs key that holds the current migration
> +    private static final String VERSION_KEY = "home_config_prefs_backend_version";

I'd go with 'home_config_version'. This is the SharedPrefs backend after all, no need to over-prefix it.

@@ +88,5 @@
>  
>          return new State(panelConfigs, true);
>      }
>  
> +    private ArrayList<PanelConfig> maybePerformMigraion(ArrayList<PanelConfig> panelConfigs) {

The migration code should act on a HomeConfig.State instance (instead of the raw list of PanelConfigs) and use the HomeConfig.Editor API to make changes to it. This will reduce the chances of making invalid changes to HomeConfig as part of a migration.

@@ +90,5 @@
>      }
>  
> +    private ArrayList<PanelConfig> maybePerformMigraion(ArrayList<PanelConfig> panelConfigs) {
> +        final SharedPreferences prefs = getSharedPreferences();
> +        final int currentVersion = prefs.getInt(VERSION_KEY, 0);

The version handling looks right. As an extra optimization, maybe we should add a boolean 'migrationDone' member to avoid doing the version check every time we load HomeConfig. See how GeckoSharedPrefs does that, for example.

@@ +110,5 @@
> +                    if (HardwareUtils.isTablet()) {
> +                        panelConfigs.add(recentTabsEntry);
> +                    } else {
> +                        panelConfigs.add(0, recentTabsEntry);
> +                    }

Maybe this migration should be a little smarter in terms of how it adds the RecentTabs panel? For example, maybe we should install and disable the new RecentTabs panel if the user has explicitly disabled the existing History panel (where the last tabs feature current lives)? It might be annoying to see a new panel (for something you've explicitly disabled before) suddenly showing up in your homepage after an app upgrade.

On the other hand, we might want to force this new panel to show up after an app upgrade so that users know the new panel exists?

This is more of an UX question I guess. Maybe check with ibarlow?

@@ +117,5 @@
> +        }
> +
> +        final SharedPreferences.Editor editor = prefs.edit();
> +        editor.putInt(VERSION_KEY, VERSION);
> +        editor.commit();

The migration should probably result in the new migrated configuration being saved before returning. Otherwise it will return the non-migrated content on the next startup, after the migration has already been done.

@@ +146,5 @@
>                  Log.e(LOGTAG, "Exception loading PanelConfig from JSON", e);
>              }
>          }
>  
> +        maybePerformMigraion(panelConfigs);

Fix typo.

Comment 30

[Brian Nicholson (:bnicholson)]

Comment on attachment 8427369 [details] [diff] [review]
(Part 5 v2) Add recently closed tabs to RecentTabsPanel

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

As usual with dealing with session-related bugs, it's important to remember that Fennec can be killed at any time, then restored to that state when the user reopens the app.

Can you test that 1) SessionStore correctly writes the undo tab data to disk, 2) SessionStore reads the undo tab data from disk, and 3) the Recent Tabs panel still shows these closed tabs after the session is restored? If not, please file a follow-up to make sure this works properly.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +45,5 @@
>  /**
>   * Fragment that displays tabs from last session in a ListView.
>   */
> +public class RecentTabsPanel extends HomeFragment
> +                             implements GeckoEventListener {

GeckoEventListener is deprecated. New code should implement NativeEventListener instead.

::: mobile/android/components/SessionStore.js
@@ +892,5 @@
> +  },
> +
> +  _sendClosedTabsToJava: function ss_sendClosedTabsToJava() {
> +    let window = Services.wm.getMostRecentWindow("navigator:browser");
> +    let tabs = JSON.parse(this.getClosedTabData(window)).map(function (tab) {

getClosedTabData() currently returns the stringified JSON, which you them immediately unstringify here. Since getClosedTabData() isn't used anywhere else, you might as well adapt it to what you really need. Return this._windows[aWindow.__SSID].closedTabs directly, then avoid the JSON.parse() here.

Comment 31

[:Margaret Leibovic]

Attachment: (Part 5 v5) Add recently closed tabs to RecentTabsPanel

Updated to address comments. I also filed bug 1015516 about saving the closedTabs data to disk, because right now we don't do that.

Comment 32

[:Margaret Leibovic]

Comment on attachment 8428172 [details] [diff] [review]
(Part 5 v5) Add recently closed tabs to RecentTabsPanel

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

::: mobile/android/base/home/RecentTabsPanel.java
@@ +226,5 @@
> +            public void run() {
> +                mClosedTabs = closedTabs;
> +
> +                // Reload the cursor to show recently closed tabs.
> +                if (mClosedTabs.length > 0 && canLoad()) {

Oh, I just realized this is wrong... we'll want to update the UI to hide the recently closed tabs if there are no more to show. However, this won't really be an issue until we fix bug 732752.

Comment 33

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #29)

> Maybe this migration should be a little smarter in terms of how it adds the
> RecentTabs panel? For example, maybe we should install and disable the new
> RecentTabs panel if the user has explicitly disabled the existing History
> panel (where the last tabs feature current lives)? It might be annoying to
> see a new panel (for something you've explicitly disabled before) suddenly
> showing up in your homepage after an app upgrade.
> 
> On the other hand, we might want to force this new panel to show up after an
> app upgrade so that users know the new panel exists?
> 
> This is more of an UX question I guess. Maybe check with ibarlow?

ibarlow, can you help us decide what to do when migrating users who have already customized their home panels?

In my current patch, I just propose adding the new "Recent Tabs" panel as the first item (farthest left) on phones, and last item (farthest right) on tablets, regardless of current configuration. Alternately, we could add the panel in a disabled state if the user has their history panel disabled, although it seems like it would be nice to let users know we have this new functionality. Maybe we should only add it in a disabled state if they have all other panels disabled? It would probably annoy people who disabled everything to see a new panel magically appear when they update their browser.

Perhaps this could be a good time for a contextual hint. Like "Hey, there's a new panel!".

Comment 34

[Thomas Stache]

I think it shouldn't ask too much to default new panels to "on" for users on the Nightly channel, who test WIP functionality like the home panels. It would be different for Beta/Release, but how much implementation effort is it really worth for Nightly?

Comment 35

[:Margaret Leibovic]

(In reply to Thomas Stache from comment #34)
> I think it shouldn't ask too much to default new panels to "on" for users on
> the Nightly channel, who test WIP functionality like the home panels. It
> would be different for Beta/Release, but how much implementation effort is
> it really worth for Nightly?

I think we should land our planned migration code in Nightly and have it ride the trains, rather than try to do something different for Beta/Release. This really only affects users who have already customized their home panels, which I don't expect to be many. However, we can probably use UI telemetry to learn more if we need this data to inform our decision.

Comment 36

[:Margaret Leibovic]

Attachment: (Part 6 v2) Add HomeConfigPrefsBackend migration to add recent tabs panel

What do you think of this? The one issue with using the HomeConfig.Editor API like this is that it doesn't expect to be used for built-in panels, so we'll need to modify it for that. At the very least we need to remove this check:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeConfig.java#1258

But then we probably would want to add a dynamic panel check before sending the "HomePanels:Installed" message to gecko.

Comment 37

[Lucas Rocha (:lucasr)]

Comment on attachment 8428172 [details] [diff] [review]
(Part 5 v5) Add recently closed tabs to RecentTabsPanel

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

The UI/Java side of things looks good to me with the suggested tweaks.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +213,5 @@
> +    public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
> +        final NativeJSObject[] tabs = message.getObjectArray("tabs");
> +        final int length = tabs.length;
> +
> +        final ClosedTab[] closedTabs = new ClosedTab[length];

It's kinda neat that Java allows 'new something[0]' :-)

@@ +216,5 @@
> +
> +        final ClosedTab[] closedTabs = new ClosedTab[length];
> +        for (int i = 0; i < length; i++) {
> +            final NativeJSObject tab = tabs[i];
> +            closedTabs[i] = new ClosedTab(tab.getString("url"), tab.getString("title"));

Just wondering: does getString() throw if 'url' is not present? Is this an error you want to explicitly ignore or do something about?

@@ +226,5 @@
> +            public void run() {
> +                mClosedTabs = closedTabs;
> +
> +                // Reload the cursor to show recently closed tabs.
> +                if (mClosedTabs.length > 0 && canLoad()) {

Do you mean that we never update the list of closed tabs on the gecko side? Anything good reason for not restarting the loader independently of mClosedTabs's length?

@@ +251,5 @@
>          mNewTabsListener.onNewTabs(urls);
>      }
>  
>      private static class RecentTabsCursorLoader extends SimpleCursorLoader {
> +        private final ClosedTab[] closedTabs;

nit: avoid mixing old and new styles in the same file i.e. use mClosedTabs here.

@@ +278,5 @@
>  
> +            if (closedTabs != null && closedTabs.length > 0) {
> +                addRow(c, null, context.getString(R.string.home_closed_tabs_title), RecentTabs.TYPE_HEADER);
> +
> +                final int length = closedTabs.length;

nit: given that there's method call to get length, do you really need to assign to a temp variable before using it in the loop?

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 8428920 [details] [diff] [review]
(Part 6 v2) Add HomeConfigPrefsBackend migration to add recent tabs panel

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

Looks good but this patch made me wonder if this migration code is being applied to the right abstraction level of our backend's 'schema'. When I recommended using State as the migration input, I did not account for other possible types of migrations that we might need in the future. For example, if we ever add a new required PanelConfig property to our JSON 'schema' (for a new panel feature or something), we'll have to migrate the existing panels at a JSON level, before the PanelConfig instances are even created. Otherwise the PanelConfig's validate() calls will fail for the old JSON representation. The apparent need to remove dynamic/built panel checks from the Editor API looks a bit smelly too. It's probably a sign that we're thinking about migrations in the wrong abstraction level.

I remember mfinkle suggested having schema versions embedded in the JSON to reduce dependencies on external sources to figure out the schema version of the current state. This might be something we want to explore while at it.

So, here's a rough suggested plan:
- Change the maybePerformMigration() to take a JSON string instead.
- Load the JSONArray from the JSON string
- On migration 1, create built-in panel add it to JSON array (use toJSON() to get its JSON representation).
- If we decide to match the current History panel's disabled state, look for it in the list and set the appropriate JSON key ('hidden' I think) in the new built-in panel.
- Save the new JSON string and return the new JSONArray.

If we go with storing the schema version in the JSON string itself, we'll have to turn the JSON array representation into a JSON object that has a 'version' and a 'panels' attribute. You'll have to refactor the code to account for the JSONArray -> JSONObject-with-a-JSONArray transition too.

Thoughts?

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +90,5 @@
>  
>          return new State(panelConfigs, true);
>      }
>  
> +    private State maybePerformMigration(State state) {

This method should be static and synchronized to avoid concurrent migrations. Keep in mind that there might be multiple instances of this backend in memory at the same time.

@@ +127,5 @@
> +        }
> +
> +        final SharedPreferences.Editor prefsEditor = prefs.edit();
> +        prefsEditor.putInt(VERSION_KEY, VERSION);
> +        prefsEditor.commit();

Maybe just do:

prefs.edit().putInt(VERSION_KEY, VERSION).commit()

Up to you.

Comment 39

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #33)
 
> In my current patch, I just propose adding the new "Recent Tabs" panel as
> the first item (farthest left) on phones, and last item (farthest right) on
> tablets, regardless of current configuration. 

I think we should start with this, 

> Maybe we should only add it in a disabled state if they have
> all other panels disabled? 

... and this. I wouldn't get too much more clever than that.

> Perhaps this could be a good time for a contextual hint. Like "Hey, there's
> a new panel!".

Let's wait on this part for now. Not sure if teasing apart tabs and history warrants a tip of its own.

Comment 40

[:Margaret Leibovic]

I want this to track 33, it would be nice to have this at the same time as bug 817716.

Comment 41

[:Margaret Leibovic]

Finally looping back around to working on this bug...

(In reply to Lucas Rocha (:lucasr) from comment #38)
> Comment on attachment 8428920 [details] [diff] [review]
> (Part 6 v2) Add HomeConfigPrefsBackend migration to add recent tabs panel
> 
> Review of attachment 8428920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but this patch made me wonder if this migration code is being
> applied to the right abstraction level of our backend's 'schema'. When I
> recommended using State as the migration input, I did not account for other
> possible types of migrations that we might need in the future. For example,
> if we ever add a new required PanelConfig property to our JSON 'schema' (for
> a new panel feature or something), we'll have to migrate the existing panels
> at a JSON level, before the PanelConfig instances are even created.
> Otherwise the PanelConfig's validate() calls will fail for the old JSON
> representation. The apparent need to remove dynamic/built panel checks from
> the Editor API looks a bit smelly too. It's probably a sign that we're
> thinking about migrations in the wrong abstraction level.
> 
> I remember mfinkle suggested having schema versions embedded in the JSON to
> reduce dependencies on external sources to figure out the schema version of
> the current state. This might be something we want to explore while at it.
> 
> So, here's a rough suggested plan:
> - Change the maybePerformMigration() to take a JSON string instead.
> - Load the JSONArray from the JSON string
> - On migration 1, create built-in panel add it to JSON array (use toJSON()
> to get its JSON representation).
> - If we decide to match the current History panel's disabled state, look for
> it in the list and set the appropriate JSON key ('hidden' I think) in the
> new built-in panel.
> - Save the new JSON string and return the new JSONArray.
> 
> If we go with storing the schema version in the JSON string itself, we'll
> have to turn the JSON array representation into a JSON object that has a
> 'version' and a 'panels' attribute. You'll have to refactor the code to
> account for the JSONArray -> JSONObject-with-a-JSONArray transition too.
> 
> Thoughts?

I like the idea of keeping the migration at the JSON level for all the reasons you mentioned above. Storing the version number with the JSON data seems a bit annoying for needing to change the way we handle the JSON, but that does seem like a good way to prevent version errors, since the version number and the data would all get saved at once.

I'll work on updating my patch, and hopefully we can land the patches in this bug some time this week (it looks like the only one that still needs review is this last migration patch).

Comment 42

[:Margaret Leibovic]

Attachment: (Part 6 v3) Add HomeConfigPrefsBackend migration to add recent tabs panel

Not sure I'm 100% happy with the way this fits in, but what do you think so far?

Comment 43

[Lucas Rocha (:lucasr)]

Comment on attachment 8441427 [details] [diff] [review]
(Part 6 v3) Add HomeConfigPrefsBackend migration to add recent tabs panel

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

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +97,5 @@
> +     * @param jsonString string currently stored in preferences.
> +     *
> +     * @return JSONArray array representing new set of panel configs.
> +     */
> +    private JSONArray maybePerformMigration(String jsonString) throws JSONException {

This needs to be synchronized on a class level to avoid any concurrent migrations across different backend instances. In other words, this method should be static and synchronized.

@@ +98,5 @@
> +     *
> +     * @return JSONArray array representing new set of panel configs.
> +     */
> +    private JSONArray maybePerformMigration(String jsonString) throws JSONException {
> +        final JSONArray originalJsonPanelConfigs = new JSONArray(jsonString);

Maybe just originalJson?

@@ +114,5 @@
> +        }
> +
> +        Log.d(LOGTAG, "Performing migration");
> +
> +        final JSONArray newJsonPanelConfigs = new JSONArray();

newJson?

@@ +139,5 @@
> +                    }
> +
> +                    // Add the new panel to the end of the array on tablets.
> +                    if (HardwareUtils.isTablet()) {
> +                        newJsonPanelConfigs.put(jsonRecentTabsConfig);

You can do this instead:

// Copy the original panel configs.
final int count = originalJsonPanelConfigs.length();
for (int i = 0; i < count; i++) {
    final JSONObject jsonPanelConfig = originalJsonPanelConfigs.getJSONObject(i);
    newJsonPanelConfigs.put(jsonPanelConfig);
}

if (HardwareUtils.isTablet()) {
    newJsonPanelConfigs.put(jsonRecentTabsConfig);
} else {
    newJsonPanelConfigs.put(0, jsonRecentTabsConfig);
}

@@ +148,5 @@
> +
> +        // Save the new panel config and the new version number.
> +        final SharedPreferences.Editor prefsEditor = prefs.edit();
> +        prefsEditor.putString(PREFS_CONFIG_KEY, newJsonPanelConfigs.toString());
> +        prefsEditor.putInt(VERSION_KEY, VERSION);

If we decide to store the version in the json itself, you can do something like this:

final JSONArray originalJson
final int version;
try {
    originalJson = new JSONArray(jsonString);
    version = 0;
} catch (JSONException e) {
    JSONObject json = new JSONObject(jsonString);
    originalJson = json.getJSONArray("panels");
    version = json.getInt("version");
}

...

The drawback being that we'll always have to try to parse as JSONArray first (even after we have already done the migration). Another option is changing the key name for the new format. This way we'd simply do a SharedPreferences.contains() check on the old key first to see if we have already done the migration to the new JSONObject format. Something like this:

final String key = (prefs.contains(OLD_KEY) ? OLD_KEY : NEW_KEY);
final String jsonString = prefs.getString(key);
if (TextUtils.isEmpty(jsonString)) {
    return loadDefaultConfig();
}

final JSONArray originalJson
final int version;
if (prefs.contains(OLD_KEY)) {
    originalJson = new JSONArray(jsonString);
    version = 0;
} else {
    JSONObject json = new JSONObject(jsonString);
    originalJson = json.getJSONArray(JSON_KEY_PANELS);
    version = json.getInt(JSON_KEY_VERSION);
}

...then do stuff and save the migrated JSON to NEW_KEY.

Comment 44

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #43)

Thanks for the feedback!
 
> You can do this instead:
> 
> // Copy the original panel configs.
> final int count = originalJsonPanelConfigs.length();
> for (int i = 0; i < count; i++) {
>     final JSONObject jsonPanelConfig =
> originalJsonPanelConfigs.getJSONObject(i);
>     newJsonPanelConfigs.put(jsonPanelConfig);
> }
> 
> if (HardwareUtils.isTablet()) {
>     newJsonPanelConfigs.put(jsonRecentTabsConfig);
> } else {
>     newJsonPanelConfigs.put(0, jsonRecentTabsConfig);
> }

I assumed this would replace the panelConfig at index 0:
http://developer.android.com/reference/org/json/JSONArray.html#put%28int,%20int%29

But I agree the two different if statements are unpleasant, it would be nice if we could do an if/else like this.

> @@ +148,5 @@
> > +
> > +        // Save the new panel config and the new version number.
> > +        final SharedPreferences.Editor prefsEditor = prefs.edit();
> > +        prefsEditor.putString(PREFS_CONFIG_KEY, newJsonPanelConfigs.toString());
> > +        prefsEditor.putInt(VERSION_KEY, VERSION);
> 
> If we decide to store the version in the json itself, you can do something
> like this:
> 
> final JSONArray originalJson
> final int version;
> try {
>     originalJson = new JSONArray(jsonString);
>     version = 0;
> } catch (JSONException e) {
>     JSONObject json = new JSONObject(jsonString);
>     originalJson = json.getJSONArray("panels");
>     version = json.getInt("version");
> }
> 
> ...
> 
> The drawback being that we'll always have to try to parse as JSONArray first
> (even after we have already done the migration). Another option is changing
> the key name for the new format. This way we'd simply do a
> SharedPreferences.contains() check on the old key first to see if we have
> already done the migration to the new JSONObject format. Something like this:
> 
> final String key = (prefs.contains(OLD_KEY) ? OLD_KEY : NEW_KEY);
> final String jsonString = prefs.getString(key);
> if (TextUtils.isEmpty(jsonString)) {
>     return loadDefaultConfig();
> }
> 
> final JSONArray originalJson
> final int version;
> if (prefs.contains(OLD_KEY)) {
>     originalJson = new JSONArray(jsonString);
>     version = 0;
> } else {
>     JSONObject json = new JSONObject(jsonString);
>     originalJson = json.getJSONArray(JSON_KEY_PANELS);
>     version = json.getInt(JSON_KEY_VERSION);
> }
> 
> ...then do stuff and save the migrated JSON to NEW_KEY.

This key name change is an interesting idea, I can play around with that.

Comment 45

[:Margaret Leibovic]

Thinking about the key name change a bit more, this seems like it could become burdensome for future migrations. In order to account for the situation where a saved config is more than one version behind the latest version, we would have to keep every old key around and iterate through them.

I can experiment with storing the version number with the JSON array, but I'm starting to think I like the way this patch currently stores the version in a separate pref, since I feel like it make the logic more straightforward.

Comment 46

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #45)
> Thinking about the key name change a bit more, this seems like it could
> become burdensome for future migrations. In order to account for the
> situation where a saved config is more than one version behind the latest
> version, we would have to keep every old key around and iterate through them.
> 
> I can experiment with storing the version number with the JSON array, but
> I'm starting to think I like the way this patch currently stores the version
> in a separate pref, since I feel like it make the logic more straightforward.

Just to be clear, what I suggested was to use a new key name just for the initial upgrade from version 0 i.e. JSONArray -> JSONObject. This initial migration would remove the old key and only use the new key for future migrations. Is that what you're trying to implement?

Comment 47

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #43)
> Comment on attachment 8441427 [details] [diff] [review]
> (Part 6 v3) Add HomeConfigPrefsBackend migration to add recent tabs panel
> 
> Review of attachment 8441427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +97,5 @@
> > +     * @param jsonString string currently stored in preferences.
> > +     *
> > +     * @return JSONArray array representing new set of panel configs.
> > +     */
> > +    private JSONArray maybePerformMigration(String jsonString) throws JSONException {
> 
> This needs to be synchronized on a class level to avoid any concurrent
> migrations across different backend instances. In other words, this method
> should be static and synchronized.

If we make this static, how should we deal with the fact that this method requires access to a Context, both to get the shared preferences and to create the new built-in panel? As a workaround, could we just make the migrationDone flag static, so that we'll only try to perform a migration once per app run? (I updated the patch to move the call to set that flag to true above the version check, since we only need to do that version check once.)

Comment 48

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #46)
> (In reply to :Margaret Leibovic from comment #45)
> > Thinking about the key name change a bit more, this seems like it could
> > become burdensome for future migrations. In order to account for the
> > situation where a saved config is more than one version behind the latest
> > version, we would have to keep every old key around and iterate through them.
> > 
> > I can experiment with storing the version number with the JSON array, but
> > I'm starting to think I like the way this patch currently stores the version
> > in a separate pref, since I feel like it make the logic more straightforward.
> 
> Just to be clear, what I suggested was to use a new key name just for the
> initial upgrade from version 0 i.e. JSONArray -> JSONObject. This initial
> migration would remove the old key and only use the new key for future
> migrations. Is that what you're trying to implement?

Oh, sorry, I was confused then. I thought you were talking about storing the version information as part of the key itself.

Comment 49

[:Margaret Leibovic]

Attachment: (Part 6 v4) Add HomeConfigPrefsBackend migration to add recent tabs panel

This patch integrates the ideas from recent comments.

I tested and confirmed this works as expected.

Comment 50

[Lucas Rocha (:lucasr)]

Comment on attachment 8442156 [details] [diff] [review]
(Part 6 v4) Add HomeConfigPrefsBackend migration to add recent tabs panel

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

Looks good with the suggested changes.

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +59,5 @@
>      private final Context mContext;
>      private ReloadBroadcastReceiver mReloadBroadcastReceiver;
>      private OnReloadListener mReloadListener;
>  
> +    private static boolean mMigrationDone = false;

Should be sMigrationDone.

@@ +118,5 @@
> +        // Make sure we only do this version check once.
> +        mMigrationDone = true;
> +
> +        JSONArray originalJsonPanels;
> +        int version;

both should be final

@@ +121,5 @@
> +        JSONArray originalJsonPanels;
> +        int version;
> +
> +        final SharedPreferences prefs = GeckoSharedPrefs.forProfile(context);
> +        if (prefs.contains(PREFS_CONFIG_KEY_OLD)) {

Add a comment about why this is considered version 0.

@@ +162,5 @@
> +
> +                    // Add the new panel to the end of the array on tablets.
> +                    if (HardwareUtils.isTablet()) {
> +                        newJsonPanels.put(jsonRecentTabsConfig);
> +                    }

This migration should remove the value of old key. Otherwise the contains(OLD_KEY) will be true on the next startup.

Comment 51

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #50)

> @@ +162,5 @@
> > +
> > +                    // Add the new panel to the end of the array on tablets.
> > +                    if (HardwareUtils.isTablet()) {
> > +                        newJsonPanels.put(jsonRecentTabsConfig);
> > +                    }
> 
> This migration should remove the value of old key. Otherwise the
> contains(OLD_KEY) will be true on the next startup.

Good catch!

I'm going to pound on this a bit before landing it, to make sure we won't run into any migration follow-ups.

Comment 52

[:Margaret Leibovic]

I realized I have to update the save implementation to save the JSON in the new format with the version number. I did that and verified that customizing the set of home panels still works.

I also realized that this will probably break about:home tests that expect the current set of panels. Here's a try run:
https://tbpl.mozilla.org/?tree=Try&rev=0cb094237403

Comment 53

[:Margaret Leibovic]

This was orange on 2.3 (phone UI) because we added a new panel at the beginning of the home pager. I made a patch to update AbotuHomeComponent to account for this new panel, let's see if that works (I'll post a patch for review if it does):
https://tbpl.mozilla.org/?tree=Try&rev=f304c180e177

Comment 55

[:Margaret Leibovic]

Attachment: (Part 7) Update robocop tests to account for new panel

My previous attempts failed, but I'm hoping this will work. Try run here:
https://tbpl.mozilla.org/?tree=Try&rev=0e4868be80ba

Luckily getting rid of the most recent/tabs from last time sections in the history panel makes some of this simpler.

Comment 56

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

Comment on attachment 8442983 [details] [diff] [review]
(Part 7) Update robocop tests to account for new panel

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

nit: Add a TODO to testAboutHomePageNavigation to adjust the test for the changed panels.

r+ with nits, assuming there are no more embedded panels (e.g. the old Tabs from Last Time) and the cleanup in AboutHomeTest thus make sense.

::: mobile/android/base/tests/AboutHomeTest.java
@@ +25,5 @@
>   * The purpose of this class is to collect all the logically connected methods that deal with about:home
>   * To use any of these methods in your test make sure it extends AboutHomeTest instead of BaseTest
>   */
>  abstract class AboutHomeTest extends PixelTest {
> +    protected enum AboutHomeTabs { RECENT_TABS, HISTORY, TOP_SITES, BOOKMARKS, READING_LIST };

nit: I'd prefer if this was vertical:

protected enum AboutHomeTabs {
    RECENT_TABS,
    ...
}

::: mobile/android/base/tests/testHistory.java
@@ +25,2 @@
>  
> +        final ListView hList = findListViewWithTag("history");

nit: You should grab this String from HomePager. Make the access public if you need to.

::: mobile/android/base/tests/testReaderMode.java
@@ +106,5 @@
>          mAsserter.ok(mSolo.waitForText("Robocop Text Page"), "Verify if the page is added to your Reading List", "The page is present in your Reading List");
>  
>          // Check if the page is added in History tab like a Reading List item
> +        openAboutHomeTab(AboutHomeTabs.HISTORY);
> +        list = findListViewWithTag("history");

nit: Again from HomePager.

::: mobile/android/base/tests/testShareLink.java
@@ +124,2 @@
>  
> +        ListView mostRecentList = findListViewWithTag("history");

nit: via HomePager.

Comment 57

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #56)
> Comment on attachment 8442983 [details] [diff] [review]
> (Part 7) Update robocop tests to account for new panel
> 
> Review of attachment 8442983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nit: Add a TODO to testAboutHomePageNavigation to adjust the test for the
> changed panels.

Filed bug 1028727 and left a TODO comment pointing to that.

> r+ with nits, assuming there are no more embedded panels (e.g. the old Tabs
> from Last Time) and the cleanup in AboutHomeTest thus make sense.

Yup, no more embedded panels, so this test is simpler.

> ::: mobile/android/base/tests/testHistory.java
> @@ +25,2 @@
> >  
> > +        final ListView hList = findListViewWithTag("history");
> 
> nit: You should grab this String from HomePager. Make the access public if
> you need to.

I filed bug 1028728 about this, since fixing this is outside the scope of updating these tests (it's a pre-existing issue).

Comment 58

[:Margaret Leibovic]

This was green on try, so pushing to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/127f582403b0
https://hg.mozilla.org/integration/fx-team/rev/b781abf699fb
https://hg.mozilla.org/integration/fx-team/rev/0477f20e297c
https://hg.mozilla.org/integration/fx-team/rev/ef5f5360951e
https://hg.mozilla.org/integration/fx-team/rev/0d67da636065
https://hg.mozilla.org/integration/fx-team/rev/defbfb9a7e15
https://hg.mozilla.org/integration/fx-team/rev/9801180cd3b7

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/127f582403b0
https://hg.mozilla.org/mozilla-central/rev/b781abf699fb
https://hg.mozilla.org/mozilla-central/rev/0477f20e297c
https://hg.mozilla.org/mozilla-central/rev/ef5f5360951e
https://hg.mozilla.org/mozilla-central/rev/0d67da636065
https://hg.mozilla.org/mozilla-central/rev/defbfb9a7e15
https://hg.mozilla.org/mozilla-central/rev/9801180cd3b7

Comment 60

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes with the wording "List recently closed tabs"