1251362 - Combine Recent Tabs into History tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Splitting this off from bug 1220928.

Comment 2

[Chenxia Liu [:liuche]]

Hi Jan, if you're looking for something to work on, would you like to take over this small project? This involves adding a "Recently closed" smartfolder to the History panel that handles "recently closed" tabs. There won't be a separate section for "tabs from last time," but if a user doesn't have "restore tabs from last time" enabled, any tabs that had been "closed" when killing Fennec should be included in "recently closed".

Everything else about the items should be the same (context menu, styling), and the smartfolder should only be visible if there are items in it. You can look at bug 1261527 or bug 1220928 for references.

To start, take a look at home/CombinedHistoryPanel.java and home/CombinedHistoryRecyclerView.java, and you'll probably want to make another adapter like home/CombinedHistoryAdapter.java or home/ClientsAdapter.java, which will listen for closing tabs.

Let me know if you have any questions!

Comment 3

[Jan Henning [:JanH]]

If it's not too urgent then yes, I can try taking a stab at it.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.

Review commit: https://reviewboard.mozilla.org/r/52896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52896/

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

Review commit: https://reviewboard.mozilla.org/r/52898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52898/

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review commit: https://reviewboard.mozilla.org/r/52900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52900/

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review commit: https://reviewboard.mozilla.org/r/52902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52902/

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review commit: https://reviewboard.mozilla.org/r/52904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52904/

Comment 9

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review commit: https://reviewboard.mozilla.org/r/52906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52906/

Comment 10

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review commit: https://reviewboard.mozilla.org/r/52908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52908/

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review commit: https://reviewboard.mozilla.org/r/52910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52910/

Comment 12

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1251362 - Part 13 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche

Review commit: https://reviewboard.mozilla.org/r/52912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52912/

Comment 13

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review commit: https://reviewboard.mozilla.org/r/52914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52914/

Comment 14

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

Review commit: https://reviewboard.mozilla.org/r/52916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52916/

Comment 15

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review commit: https://reviewboard.mozilla.org/r/52918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52918/

Comment 16

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review commit: https://reviewboard.mozilla.org/r/52920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52920/

Comment 17

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1251362 - Part 14 - Migrate (customised) home panel configurations

Review commit: https://reviewboard.mozilla.org/r/52922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52922/

Comment 18

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review commit: https://reviewboard.mozilla.org/r/52924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52924/

Comment 19

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Normally, this is only used by the crash loop protection (see bug 1263110).

Review commit: https://reviewboard.mozilla.org/r/52926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52926/

Comment 20

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

This adds telemetry for clicking on a closed tab or one of the "Open all" buttons. Methods and extras strings are the same as those used for the old Recent Tabs panel.

Review commit: https://reviewboard.mozilla.org/r/54438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54438/

Comment 21

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

By passing the panel types to be removed/set as new default panel as arguments instead of hard coding them, we can reuse that function for our own home panel config migration.

Review commit: https://reviewboard.mozilla.org/r/54440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54440/

Comment 22

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

For people with customised home panels, we need to explicitly remove the Recent Tabs panel. We also unhide the Combined History panel if it was previously hidden and additionally turn it into the default panel if the Recent Tabs panel was the previous default panel.

Review commit: https://reviewboard.mozilla.org/r/54442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54442/

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8752994 [details]
Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52896/diff/1-2/

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8752995 [details]
Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52898/diff/1-2/

Comment 25

[Jan Henning [:JanH]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52900/diff/1-2/

Comment 26

[Jan Henning [:JanH]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52902/diff/1-2/

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52904/diff/1-2/

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52906/diff/1-2/

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52908/diff/1-2/

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52910/diff/1-2/

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8753002 [details]
MozReview Request: Bug 1251362 - Part 13 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52912/diff/1-2/

Comment 32

[Jan Henning [:JanH]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52914/diff/1-2/

Comment 33

[Jan Henning [:JanH]]

Comment on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52916/diff/1-2/

Comment 34

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52918/diff/1-2/

Comment 35

[Jan Henning [:JanH]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52920/diff/1-2/

Comment 36

[Jan Henning [:JanH]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52924/diff/1-2/

Comment 37

[Jan Henning [:JanH]]

Comment on attachment 8753009 [details]
Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52926/diff/1-2/

Comment 38

[Chenxia Liu [:liuche]]

Comment on attachment 8752994 [details]
Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.

https://reviewboard.mozilla.org/r/52896/#review51372

::: mobile/android/base/resources/layout/home_combined_history_panel.xml:15
(Diff revision 2)
>  
>      <android.support.v4.widget.SwipeRefreshLayout
>              android:id="@+id/refresh_layout"
>              android:layout_width="match_parent"
>              android:layout_height="0dp"
> -            android:layout_weight="1">
> +            android:layout_weight="1.2">

Eek, yeah - when we fix bug 1267884 we can properly use wrap_content for the RecyclerView and don't need to do this hack in order to have the empty views show up properly.

Comment 39

[Chenxia Liu [:liuche]]

Comment on attachment 8752995 [details]
Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

https://reviewboard.mozilla.org/r/52898/#review51376

For this, ideally we would just not show "Recently Closed" if there are no items.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:393
(Diff revision 2)
>      }
>  
>      private void updateEmptyView() {
>          boolean showEmptyHistoryView = false;
>          boolean showEmptyClientsView = false;
> +        boolean showEmptyRecentTabsView = false;

I mentioned in a comment (but not documented anywhere in the mocks) that if there are no recently closed tabs, we should hide this smartfolder. We can use the empty view right now, but ideally we can remove that and just not need to handle this state.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:15
(Diff revision 2)
> +import android.view.View;
> +import android.view.ViewGroup;
> +
> +import org.mozilla.gecko.R;
> +
> +import static org.mozilla.gecko.home.CombinedHistoryItem.ItemType.*;

Nit: Don't use wildcard imports.

Comment 40

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52896/#review51372

> Eek, yeah - when we fix bug 1267884 we can properly use wrap_content for the RecyclerView and don't need to do this hack in order to have the empty views show up properly.

Ah right, I wasn't entirely happy about this, either, but I think I had tried using wrap_content and it didn't work - as you've described [here](https://bugzilla.mozilla.org/show_bug.cgi?id=1267884#c0), the refresh layout simply expanded to take up the whole panel height and pushed the empty views off the screen.

Comment 41

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52898/#review51376

> I mentioned in a comment (but not documented anywhere in the mocks) that if there are no recently closed tabs, we should hide this smartfolder. We can use the empty view right now, but ideally we can remove that and just not need to handle this state.

Partially this is an artefact of how I went about implementing this - since the only other smart folder in the history panel (synced tabs) doesn't use dynamic visibility, I wanted to make sure this new folder was properly working before doing anything fancy like hiding it when empty. Hiding of this folder is (currently) implemented in [part 9](https://reviewboard.mozilla.org/r/52912/diff/#index_header).

The other thing is that in [part 13](https://reviewboard.mozilla.org/r/52918/diff/#index_header) I've redirected direct loads of the old Recent Tabs panel about:home?panel=... URL (i.e. [this](https://dxr.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1430)) to directly jump to the Recent Tabs folder of the new Combined History panel. 
Currently this jump is unconditional, i.e. it doesn't depend on whether we have any closed tabs or not (we'd have to wait for Gecko to be up and running before we could decide this), meaning the empty view might still be necessary if we
- don't want to display a completely empty panel
- don't want to delay the jump to the Recent Tabs folder until Gecko has started up

> Nit: Don't use wildcard imports.

I was taking my cue from [the ClientsAdapter](https://dxr.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf/mobile/android/base/java/org/mozilla/gecko/home/ClientsAdapter.java#31), but sure, I'll import them indvidually.

Comment 42

[Jan Henning [:JanH]]

Comment on attachment 8752995 [details]
Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52898/diff/2-3/

Comment 43

[Jan Henning [:JanH]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52900/diff/2-3/

Comment 44

[Jan Henning [:JanH]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52902/diff/2-3/

Comment 45

[Jan Henning [:JanH]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52904/diff/2-3/

Comment 46

[Jan Henning [:JanH]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52906/diff/2-3/

Comment 47

[Jan Henning [:JanH]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52908/diff/2-3/

Comment 48

[Jan Henning [:JanH]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52910/diff/2-3/

Comment 49

[Jan Henning [:JanH]]

Comment on attachment 8753002 [details]
MozReview Request: Bug 1251362 - Part 13 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52912/diff/2-3/

Comment 50

[Jan Henning [:JanH]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52914/diff/2-3/

Comment 51

[Jan Henning [:JanH]]

Comment on attachment 8755197 [details]
Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54438/diff/1-2/

Comment 52

[Jan Henning [:JanH]]

Comment on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52916/diff/2-3/

Comment 53

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52918/diff/2-3/

Comment 54

[Jan Henning [:JanH]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52920/diff/2-3/

Comment 55

[Jan Henning [:JanH]]

Comment on attachment 8755198 [details]
Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54440/diff/1-2/

Comment 56

[Jan Henning [:JanH]]

Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54442/diff/1-2/

Comment 57

[Jan Henning [:JanH]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52924/diff/2-3/

Comment 58

[Jan Henning [:JanH]]

Comment on attachment 8753009 [details]
Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52926/diff/2-3/

Comment 59

[Chenxia Liu [:liuche]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

https://reviewboard.mozilla.org/r/52900/#review51378

Some comments - I'm going to flag anthony for some questions regarding the "Open all" button and just the behavior/mock in general when there are no items. Good point on switching back to the view from an intermediate state being a case where we might need an empty state for this sub-panel.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:583
(Diff revision 3)
>              mRefreshLayout.setRefreshing(false);
>          }
>      }
>  
>      @Override
> +    public void onDestroyView() {

Are views destroyed under memory pressure? This should be fine, but I'm wondering if this listener should be in onCreate/onDestroy instead. (I understand that this is what the previous code did, but I'm just wondering if the alternative would be better.)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryRecyclerView.java:129
(Diff revision 3)
>                  break;
>  
>              case CLOSED_TAB:
>                  break;
> +
> +            case OPEN_ALL_TABS:

We could also reuse the footer button (that is currently only "Clear History" right now). We should check with Anthony for mocks, but if that does change, we can do it in this bug, or in a separate one.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:33
(Diff revision 3)
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +import java.util.ArrayList;
> +import java.util.List;
>  
>  import static org.mozilla.gecko.home.CombinedHistoryItem.ItemType.CLOSED_TAB;

I meant, is it possible to just import ItemType, and then use ItemType.CLOSED_TAB, etc?

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:43
(Diff revision 3)
> -public class RecentTabsAdapter extends RecyclerView.Adapter<CombinedHistoryItem> implements CombinedHistoryRecyclerView.AdapterContextMenuBuilder {
> +public class RecentTabsAdapter extends RecyclerView.Adapter<CombinedHistoryItem>
> +                               implements CombinedHistoryRecyclerView.AdapterContextMenuBuilder, NativeEventListener {
>      private static final String LOGTAG = "GeckoRecentTabsAdapter";
>  
> +    private static final int NAVIGATION_BACK_BUTTON_INDEX = 0;
> +    private static final int BUTTON_LENGTH = 1;

This variable name should either be more specific or commented - maybe num_header_buttons or just num_buttons.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:99
(Diff revision 3)
> +        // Only modify mRecentlyClosedTabs on the UI thread.
> +        ThreadUtils.postToUiThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                mRecentlyClosedTabs = closedTabs;
> +                notifyDataSetChanged();

We can be a little more clever about this notifyDataSetChanged with RecyclerView.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:140
(Diff revision 3)
> +                // Only modify mLastSessionTabs on the UI thread.
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        mLastSessionTabs = lastSessionTabs;
> +                        notifyDataSetChanged();

Same, we can probably update this cleverly (though it's probably less important than for synced tabs/history, because users won't usually see these animations).

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:204
(Diff revision 3)
> +        itemCount += mRecentlyClosedTabs.length + mLastSessionTabs.length;
> +
> +        if (isOpenAllRecentTabsButtonVisible()) {
> +            itemCount += 1;
> +        }
> +        if (isLastSessionTabsSectionHeaderVisible()) {

We don't want to separate last section tabs from recent tabs (visually at least). They should all be in the same place. I think you asked me about this, but I think I must not have been very clear about answering - what I meant was, if we keep them separate (programmatically) we can have different telemetry for them, but visually, they should not be separated into different sections.

Tabs that get closed manually should be the same as tabs that were closed as a result of closing your session.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:227
(Diff revision 3)
> +
> +        if (isLastSessionTabsSectionHeaderVisible() && position == getLastSessionTabsSectionHeaderIndex()) {
> +            return SECTION_HEADER;
> +        }
> +
> +        if (isOpenAllLastSessionTabsButtonVisible() && position == getOpenAllLastSessionTabsButtonIndex()) {

These should be the same button I think, but we can double check with Anthony.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:271
(Diff revision 3)
> +
> +    private int getFirstLastSessionTabIndex() {
> +        return getLastSessionTabsSectionHeaderIndex() + BUTTON_LENGTH;
> +    }
> +
> +    private boolean isOpenAllLastSessionTabsButtonVisible() {

I'd like to get antlam's feedback on this, but I think it'd be easier to handle this case if we didn't need to also keep track of a completely unrelated item in the adapter, and could just have the "footer button" for this job.

Comment 60

[Chenxia Liu [:liuche]]

antlam, some questions for you:

- Can you provide a mock with what the "Recently Closed" subpanel should look like? Should there be a header (and if so, what string), and how should the "Open all" button look? Can that be the footer button that we had before (or do we not want it at all anymore)?
- Do you want something different from the existing Recent Tabs empty view for this new Recently Closed subpanel? I know that we want to hide the smartfolder when there are no items, but is there a case where the user is in the folder, then clears the tabs or something? We might want an empty state for that (but we could also do that in a later bug).
- Are there any different actions you'd like for the context menu choices? (Open normal/private, copy link, share, Add to home Screen)

Comment 61

[Chenxia Liu [:liuche]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

https://reviewboard.mozilla.org/r/52902/#review51654

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:114
(Diff revision 3)
>      public void onCreate(Bundle savedInstance) {
>          super.onCreate(savedInstance);
>  
>          mHistoryAdapter = new CombinedHistoryAdapter(getResources());
>          mClientsAdapter = new ClientsAdapter(getContext());
> -        mRecentTabsAdapter = new RecentTabsAdapter(getContext());
> +        mRecentTabsAdapter = new RecentTabsAdapter(getContext(), getPanelStateUpdateHandler());

Add a comment explaining why this is different from the others - the explanation you gave in the comment is good, let's put it in the code too.

Comment 62

[Chenxia Liu [:liuche]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

https://reviewboard.mozilla.org/r/52904/#review51658

Comment 63

[Jan Henning [:JanH]]

antlam, liuche: To explain my reasoning for keeping "Recently closed tabs" and "Tabs from last time" somewhat separate, these two types of closed tabs have somewhat differing lifecycles, so just mixing the two together might potentially be slightly confusing:
"Recently closed tabs" are updated live and on-the-go - as soon as you close a tab, it appears on the Recent Tabs list. However we only keep track of a limited number of recently closed tabs (5 by default, I think), so once you start exceeding that number, your least recently closed tabs are getting pushed off that list.
The number of "Tabs from last time" on the other hand can be arbitrarily large (as large as your last session was) and they're only updated during application startup.
So if we'd simply throw the two together in one list, we'd get into a situation where some closed tabs are constantly appearing and then later disappearing (as new closed tabs come in) from one half of the list, while all the other tabs remain unchanged as long as the browsing session lasts. Explicitly labelling that other half as "Tabs from last time" makes this difference in behaviour clearer to my eyes.

About the Open all button, how relevant would the following scenario be (for a user who doesn't automatically restore her or his tabs):
- You start Firefox by launching it from a link in an external app, i.e. you jump directly into a new tab.
- You eventually close that tab and decide to resume your last session.

If there's only one Open All button, there's no easy way to resume your last session without mixing in any other tabs you might have already closed during the current browsing session.

Comment 64

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52898/#review51376

> Partially this is an artefact of how I went about implementing this - since the only other smart folder in the history panel (synced tabs) doesn't use dynamic visibility, I wanted to make sure this new folder was properly working before doing anything fancy like hiding it when empty. Hiding of this folder is (currently) implemented in [part 9](https://reviewboard.mozilla.org/r/52912/diff/#index_header).
> 
> The other thing is that in [part 13](https://reviewboard.mozilla.org/r/52918/diff/#index_header) I've redirected direct loads of the old Recent Tabs panel about:home?panel=... URL (i.e. [this](https://dxr.mozilla.org/mozilla-central/rev/46fe2115d46a5bb40523b8466341d8f9a26e1bdf/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1430)) to directly jump to the Recent Tabs folder of the new Combined History panel. 
> Currently this jump is unconditional, i.e. it doesn't depend on whether we have any closed tabs or not (we'd have to wait for Gecko to be up and running before we could decide this), meaning the empty view might still be necessary if we
> - don't want to display a completely empty panel
> - don't want to delay the jump to the Recent Tabs folder until Gecko has started up

Thinking some more about it, in the above use case we're only really interested in the "Tabs from last time" and reading the sessionstore.bak file doesn't depend on Gecko being up and running, however from a more general perspective, we'd still have to wait for the JS session store to be running before we know for certain whether we've got any closed tabs to display or not.

Comment 65

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52900/#review51378

> Are views destroyed under memory pressure? This should be fine, but I'm wondering if this listener should be in onCreate/onDestroy instead. (I understand that this is what the previous code did, but I'm just wondering if the alternative would be better.)

I don't have much experience with Android's lifecycles, but speaking empirically, both on my phone and on an emulator the CombinedHistoryPanel gets destroyed as soon as about:home is no longer visible and I hope I can assume that the view wouldn't get destroyed while the panel was still visible? In which case it probably wouldn't really matter which pair of functions to use, especially as the session store sends out full updates, anyway.

> We could also reuse the footer button (that is currently only "Clear History" right now). We should check with Anthony for mocks, but if that does change, we can do it in this bug, or in a separate one.

Yep, let's wait for Anthony, although as mentioned, if there's only one button for both types, resuming the last session (if you're not restoring automatically) becomes slighty more cumbersome if you decide to resume only after having already closed some tabs in the current session.

> I meant, is it possible to just import ItemType, and then use ItemType.CLOSED_TAB, etc?

Right, I understand now :-), will change it again where necessary.

> This variable name should either be more specific or commented - maybe num_header_buttons or just num_buttons.

Hmm, this was actually supposed to signify the space occupied by a non-data item, i.e. one button/section header/... takes up one item in the list. Maybe in this case *not* using a named constant might be clearer?

> We don't want to separate last section tabs from recent tabs (visually at least). They should all be in the same place. I think you asked me about this, but I think I must not have been very clear about answering - what I meant was, if we keep them separate (programmatically) we can have different telemetry for them, but visually, they should not be separated into different sections.
> 
> Tabs that get closed manually should be the same as tabs that were closed as a result of closing your session.

See my comment [here](https://bugzilla.mozilla.org/show_bug.cgi?id=1251362#c63).

Comment 66

[Chenxia Liu [:liuche]]

Attachment: Mock: Recently closed panel

JanH: Here's the mock for the "Recently Closed" panel, I wasn't aware that it existed but now I've found it. The header is a separate thing (bug 1253654) and the styling of the footer button is different (so these don't need to be incorporated in this bug), but the main structure is correct.

Comment 67

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jan Henning [:JanH] from comment #63)
> antlam, liuche: To explain my reasoning for keeping "Recently closed tabs"
> and "Tabs from last time" somewhat separate, these two types of closed tabs
> have somewhat differing lifecycles, so just mixing the two together might
> potentially be slightly confusing:

I'm not too worried about this. Lifecycles are very hard to keep track of via memory, especially if it's been a while. 

The goal here is to simplify the model in our users head. If they're looking for a tab that has been closed (regardless of lifecycle), they can find in the "Recently closed" smart folder.

The only separation I'd like us to create here is whether it's been closed, or not.

> "Recently closed tabs" are updated live and on-the-go - as soon as you close
> a tab, it appears on the Recent Tabs list. However we only keep track of a
> limited number of recently closed tabs (5 by default, I think), so once you
> start exceeding that number, your least recently closed tabs are getting
> pushed off that list.

Can we extend this number beyond 5? 10 would be more helpful (that's also what I counted in some other browsers).

> The number of "Tabs from last time" on the other hand can be arbitrarily
> large (as large as your last session was) and they're only updated during
> application startup.
>
> So if we'd simply throw the two together in one list, we'd get into a
> situation where some closed tabs are constantly appearing and then later
> disappearing (as new closed tabs come in) from one half of the list, while
> all the other tabs remain unchanged as long as the browsing session lasts.
> Explicitly labelling that other half as "Tabs from last time" makes this
> difference in behaviour clearer to my eyes.

I can see how that makes sense. But this goes back to my first point about simplifying the model. I'm still not seeing a case for this separation since a closed tab is a closed tab. Perhaps we can see how this feels and then revisit if it really starts being confusing. 

> About the Open all button, how relevant would the following scenario be (for
> a user who doesn't automatically restore her or his tabs):
> - You start Firefox by launching it from a link in an external app, i.e. you
> jump directly into a new tab.
> - You eventually close that tab and decide to resume your last session.
> 
> If there's only one Open All button, there's no easy way to resume your last
> session without mixing in any other tabs you might have already closed
> during the current browsing session.

This seems like an edge case to me. There's History, and Recently closed where you can either restore all in that list, or just long-press on one and "Open in New Tab".

Comment 68

[Jan Henning [:JanH]]

Thanks for the feedback, I'll update my patches accordingly.

@liuche: Should I just drop the "Tabs from last time" string directly then, or leave it in until we can evaluate the new design in the wild (and file a follow-up bug, so we don't forget about it)?

Comment 69

[Chenxia Liu [:liuche]]

Let's take it out - if we're not uplifting any code this shouldn't matter too much, and the linter will complain about unused string resources.

Comment 70

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Temporarily cancelling, as I still need to push a fix so as not to access mRecyclerView if it is still null (if onRestore is called before onViewCreated), which took stupidly long to debug since instead of issuing a proper exception or crashing, the UI simply started hanging instead.

Comment 71

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1251362 - Part 4 - Tint Recent Tabs icon to match the colour of the Synced Devices cloud icon. r=liuche

Review commit: https://reviewboard.mozilla.org/r/56044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56044/

Comment 72

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 9 - Display a button to open all recently closed tabs.

Depending on the History panel's PanelLevel state, we now dynamically set the panel footer button's text and determine its onClick behaviour.

Review commit: https://reviewboard.mozilla.org/r/56046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56046/

Comment 73

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 12 - Remember more recently closed tabs.

Review commit: https://reviewboard.mozilla.org/r/56048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56048/

Comment 74

[Jan Henning [:JanH]]

Comment on attachment 8752994 [details]
Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52896/diff/2-3/

Comment 75

[Jan Henning [:JanH]]

Comment on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52916/diff/3-4/

Comment 76

[Jan Henning [:JanH]]

Comment on attachment 8752995 [details]
Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52898/diff/3-4/

Comment 77

[Jan Henning [:JanH]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52900/diff/3-4/

Comment 78

[Jan Henning [:JanH]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52902/diff/3-4/

Comment 79

[Jan Henning [:JanH]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52904/diff/3-4/

Comment 80

[Jan Henning [:JanH]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52906/diff/3-4/

Comment 81

[Jan Henning [:JanH]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52908/diff/3-4/

Comment 82

[Jan Henning [:JanH]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52910/diff/3-4/

Comment 83

[Jan Henning [:JanH]]

Comment on attachment 8753002 [details]
MozReview Request: Bug 1251362 - Part 13 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52912/diff/3-4/

Comment 84

[Jan Henning [:JanH]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52914/diff/3-4/

Comment 85

[Jan Henning [:JanH]]

Comment on attachment 8755197 [details]
Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54438/diff/2-3/

Comment 86

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52918/diff/3-4/

Comment 87

[Jan Henning [:JanH]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52920/diff/3-4/

Comment 88

[Jan Henning [:JanH]]

Comment on attachment 8755198 [details]
Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54440/diff/2-3/

Comment 89

[Jan Henning [:JanH]]

Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54442/diff/2-3/

Comment 90

[Jan Henning [:JanH]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52924/diff/3-4/

Comment 91

[Jan Henning [:JanH]]

Comment on attachment 8753009 [details]
Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52926/diff/3-4/

Comment 92

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52900/#review51378

> We can be a little more clever about this notifyDataSetChanged with RecyclerView.

I've worked something up, I just hope it's not *too* clever for its own good and that my understanding of how RecyclerView's notifyItem...() API is supposed to work is correct.
Parts of it are also a bit difficult to test, since most of the actions that affect the list's state are only possible while the Recent Tabs folder isn't even visible.

What limited testing I've been able to do by having the folder open immediately during startup before Gecko is ready (either through my debug hack, or by having about:home?panel=5c2601a5-eedc-4477-b297-ce4cef52adf8 open and setting the tab restore setting back to "Always"), or by having that special about:home URL open in one tab and then closing a different tab and switching rapidly back to the about:home panel before the ClosedTabs:Data messsage comes through seems to suggest that at least adding new tabs to the list is properly animated now, though.

Comment 93

[Jan Henning [:JanH]]

Updated APK for testing:
http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-501a19b6e746e84cdc1c800df035596894191f3f/try-android-api-15/fennec-49.0a1.en-US.android-arm.apk

Comment 94

[Jan Henning [:JanH]]

Come to think about it, that APK version will migrate your home panel config and remove the Recent Tabs panel, and there's no easy way back without clearing your user data or root access.
I'll post a version without the migration logic tomorrow.

Comment 95

[Jan Henning [:JanH]]

Okay, this APK doesn't contain any migration logic, so it shouldn't mess up your current profile if you later revert to a normal Nightly before this has landed:
http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-68f97153c96ee63c2854f662bf112b791b8683ac/try-android-api-15/fennec-49.0a1.en-US.android-arm.apk

Comment 96

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jan Henning [:JanH] from comment #93)
> Updated APK for testing:
> http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 501a19b6e746e84cdc1c800df035596894191f3f/try-android-api-15/fennec-49.0a1.en-
> US.android-arm.apk

I didn't see the other APKs attached but I tried this one on my N9.

Looking good JanH! thanks for the build. It's really handy to see on a device. 

Some feedback:

 - "Recenty closed tabs" as the title - let's change this to "Recently closed" since we also list "# tabs" underneath.

 - Sometimes when I switch to the History panel, I see the "Recently closed tabs" smart folder slide in and push everything else down. Is it possible to remove this animation? having things move around at that moment isn't a great experience :)

- Action button in the "Recently closed" smart folder, can we say "RESTORE ALL" instead? 

Otherwise, this looks great!

Comment 97

[Jan Henning [:JanH]]

I've done the string changes as you requested.

About the animation, the problem is that probably occasionally Gecko can take a while to send the recently closed tabs back to the History panel (a bit like the problem in bug 1270162, where the history queries take a while to resolve).

I could disable the animation, although the rest of the history panel contents would still be pushed/jump downwards once the Recent Tabs smart folder becomes visible. Plus the current implementation has one extenuating feature - if you've already started scrolling down within the history, the viewport won't jump around once the Recent Tabs folder appears (on the basis that if you've already started scrolling the history, you're probably not interested in the contents of the smart folders). If we don't animate, then the history panel will always jump around by one item, regardless of the current viewport position.
(Strictly speaking I could just always add the Recent Tabs smart folder without moving the viewport, but that means that you would get no visual notification that the Recent Tabs folder has become available - you'd always have to attempt to scroll up and see whether it's possible or not).

I'll ask liuche if we should consider caching the closed tabs or something else in a follow up bug, though.

Comment 98

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jan Henning [:JanH] from comment #97)
> I've done the string changes as you requested.

Awesome, thanks!

> About the animation, the problem is that probably occasionally Gecko can
> take a while to send the recently closed tabs back to the History panel (a
> bit like the problem in bug 1270162, where the history queries take a while
> to resolve).
> 
> I could disable the animation, although the rest of the history panel
> contents would still be pushed/jump downwards once the Recent Tabs smart
> folder becomes visible. Plus the current implementation has one extenuating
> feature - if you've already started scrolling down within the history, the
> viewport won't jump around once the Recent Tabs folder appears (on the basis
> that if you've already started scrolling the history, you're probably not
> interested in the contents of the smart folders). If we don't animate, then
> the history panel will always jump around by one item, regardless of the
> current viewport position.
> (Strictly speaking I could just always add the Recent Tabs smart folder
> without moving the viewport, but that means that you would get no visual
> notification that the Recent Tabs folder has become available - you'd always
> have to attempt to scroll up and see whether it's possible or not).
> 
> I'll ask liuche if we should consider caching the closed tabs or something
> else in a follow up bug, though.

Ah, gotcha. 

Is it hard to keep the folder item in place (i.e. fixed)? Perhaps if we leave it there, then it doesn't have to "animate in"?

For a new user, it might be empty. But that's okay - "Recently closed, 0 tabs" is clear enough.

Can we do that?

Comment 99

[Jan Henning [:JanH]]

Liuche, since you originally said

(In reply to Chenxia Liu [:liuche] from comment #2)
> and the smartfolder should only be visible if there are items in it.

do you have any particular opinion on that?

@antlam: Implementation-wise it's no problem to keep the "Recently closed" folder visible all the time, I could either adjust the folder visibility condition to always return true, or more properly, simply not include that part (https://reviewboard.mozilla.org/r/52912/) of the patch in the final version.

Comment 100

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Actually, my mistake. I remember that now ^ from the original spec.

Let's keep it as is. 

Thanks!

Comment 101

[Jan Henning [:JanH]]

You're welcome :-)

Leaving the NI for liuche re the question whether we should consider caching the tabs in a follow up bug, or not.

Comment 102

[Jan Henning [:JanH]]

Comment on attachment 8752994 [details]
Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52896/diff/3-4/

Comment 103

[Jan Henning [:JanH]]

Comment on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52916/diff/4-5/

Comment 104

[Jan Henning [:JanH]]

Comment on attachment 8752995 [details]
Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52898/diff/4-5/

Comment 105

[Jan Henning [:JanH]]

Comment on attachment 8757648 [details]
MozReview Request: Bug 1251362 - Part 4 - Tint Recent Tabs icon to match the colour of the Synced Devices cloud icon. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56044/diff/1-2/

Comment 106

[Jan Henning [:JanH]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52900/diff/4-5/

Comment 107

[Jan Henning [:JanH]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52902/diff/4-5/

Comment 108

[Jan Henning [:JanH]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52904/diff/4-5/

Comment 109

[Jan Henning [:JanH]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52906/diff/4-5/

Comment 110

[Jan Henning [:JanH]]

Comment on attachment 8757649 [details]
Bug 1251362 - Part 9 - Display a button to open all recently closed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56046/diff/1-2/

Comment 111

[Jan Henning [:JanH]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52908/diff/4-5/

Comment 112

[Jan Henning [:JanH]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52910/diff/4-5/

Comment 113

[Jan Henning [:JanH]]

Comment on attachment 8757650 [details]
Bug 1251362 - Part 12 - Remember more recently closed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56048/diff/1-2/

Comment 114

[Jan Henning [:JanH]]

Comment on attachment 8753002 [details]
MozReview Request: Bug 1251362 - Part 13 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52912/diff/4-5/

Comment 115

[Jan Henning [:JanH]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52914/diff/4-5/

Comment 116

[Jan Henning [:JanH]]

Comment on attachment 8755197 [details]
Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54438/diff/3-4/

Comment 117

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52918/diff/4-5/

Comment 118

[Jan Henning [:JanH]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52920/diff/4-5/

Comment 119

[Jan Henning [:JanH]]

Comment on attachment 8755198 [details]
Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54440/diff/3-4/

Comment 120

[Jan Henning [:JanH]]

Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54442/diff/3-4/

Comment 121

[Jan Henning [:JanH]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52924/diff/4-5/

Comment 122

[Jan Henning [:JanH]]

Comment on attachment 8753009 [details]
Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52926/diff/4-5/

Comment 123

[Chenxia Liu [:liuche]]

Comment on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

https://reviewboard.mozilla.org/r/52916/#review53426

Comment 124

[Chenxia Liu [:liuche]]

Comment on attachment 8757648 [details]
MozReview Request: Bug 1251362 - Part 4 - Tint Recent Tabs icon to match the colour of the Synced Devices cloud icon. r=liuche

https://reviewboard.mozilla.org/r/56044/#review53430

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:59
(Diff revision 2)
>  
> -    public CombinedHistoryAdapter(Resources resources) {
> +    public CombinedHistoryAdapter(Context context, Resources resources) {
>          super();
>          sectionHeaders = new SparseArray<>();
>          HistorySectionsHelper.updateRecentSectionOffset(resources, sectionDateRangeArray);
> +        // Colors chosen so that the end result after tinting is disabled_grey (#BFBFBF).

We can do this, but I think the real solution is to either have another resource (not great) or to get a new resource in white/black that we tint in both places.

Comment 125

[Chenxia Liu [:liuche]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

https://reviewboard.mozilla.org/r/52906/#review52654

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:335
(Diff revision 5)
>  
>      private int getLastLastSessionTabIndex() {
>          return getLastRecentTabIndex() + lastSessionTabs.length;
>      }
>  
> +    private ClosedTab getClosedTabForPosition(int position) {

Nice, I was going to comment that this probably should be pulled into a method. Perhaps add a comment for the if/else (// Recently closed tab // Last session tab)

Comment 126

[Chenxia Liu [:liuche]]

Comment on attachment 8757649 [details]
Bug 1251362 - Part 9 - Display a button to open all recently closed tabs.

https://reviewboard.mozilla.org/r/56046/#review53452

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:237
(Diff revision 2)
> +        AddTabDataToList(dataList, lastSessionTabs);
> +
> +        restoreSessionWithHistory(dataList);
> +    }
> +
> +    private void AddTabDataToList(List<String> dataList, ClosedTab[] closedTabs) {

Nit: Method name should not be capitalized.

Comment 127

[Chenxia Liu [:liuche]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

https://reviewboard.mozilla.org/r/52908/#review53454

Comment 128

[Chenxia Liu [:liuche]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

https://reviewboard.mozilla.org/r/52900/#review53434

r+ with comments addressed.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:99
(Diff revision 5)
> +                int prevSectionHeaderIndex = getSectionHeaderIndex();
> +
> +                recentlyClosedTabs = closedTabs;
> +                int closedTabCountChange = recentlyClosedTabs.length - prevClosedTabsCount;
> +
> +                // Handle the section header hiding/unhiding.

I'd consider moving this to a separate method because it's reused, updateHeaderVisibility().

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:107
(Diff revision 5)
> +                } else if (!prevSectionHeaderVisibility && isSectionHeaderVisible()) {
> +                    notifyItemInserted(getSectionHeaderIndex());
> +                }
> +
> +                // Update the "Recently closed" part of the tab list.
> +                if (recentlyClosedTabs.length == 0) {

Could we make this a little simpler by reordering this into 1) notifyChanged(), 2) notifyAdded/Removed depending on the prevClosedTabsCount.

e.g.,

if (recentlyClosed.length > 0) {
  notifyItemRangedChanged(getFirstRecentTabIndex(), MIN(recentlyClosed.length, prevClosedTabsCount));
}

if (closedTabCountChange > 0) {
  notifyItemRangeAdded(getLastRecentTabIndex() + 1, closedTabCountChange
} else if (closedTabCountChange < 0) {
  notifyItemRangeRemoved(getLastRecentTabIndex() + 1, -closedTabCountChange)
}

(plus some // Comments)
That way there's less nesting.

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:265
(Diff revision 5)
>      private CombinedHistoryItem.ItemType getItemTypeForPosition(int position) {
> -        if (position == 0) {
> +        if (position == NAVIGATION_BACK_BUTTON_INDEX) {
>              return ItemType.NAVIGATION_BACK;
>          }
>  
> +        if (position == getSectionHeaderIndex() && isSectionHeaderVisible()) {

Nit: I would reverse these two to short-circuit on the section header visibliity check.

Comment 129

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52900/#review53434

> Nit: I would reverse these two to short-circuit on the section header visibliity check.

Hmm, I originally had them the other way round, but then I'd reversed them because the header will only be hidden when there are no tabs to display. If the panel is empty, though, I've hidden the folder button, which means it's almost impossible to actually view the panel in its empty state - it's only accessible through the crash loop protection or if you manually enter that about:home?panel=... URL. So `isSectionHeaderVisible()` will almost always be true, whereas the position check will pass only once while going through all positions.

Comment 130

[Chenxia Liu [:liuche]]

> > Nit: I would reverse these two to short-circuit on the section header visibliity check.
> 
> Hmm, I originally had them the other way round, but then I'd reversed them
> because the header will only be hidden when there are no tabs to display. If
> the panel is empty, though, I've hidden the folder button, which means it's
> almost impossible to actually view the panel in its empty state - it's only
> accessible through the crash loop protection or if you manually enter that
> about:home?panel=... URL. So `isSectionHeaderVisible()` will almost always
> be true, whereas the position check will pass only once while going through
> all positions.

Sure.

> > and the smartfolder should only be visible if there are items in it.
> 
> do you have any particular opinion on that?
> 
> @antlam: Implementation-wise it's no problem to keep the "Recently closed"
> folder visible all the time, I could either adjust the folder visibility
> condition to always return true, or more properly, simply not include that
> part (https://reviewboard.mozilla.org/r/52912/) of the patch in the final
> version.

Okay, I think that makes sense to make this patch series simpler - we don't need to be extremely clever about it, and if we do want to change it later, we can consider that in isolation. Sorry to have the specs changing so much!

Comment 131

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52898/#review51376

> Thinking some more about it, in the above use case we're only really interested in the "Tabs from last time" and reading the sessionstore.bak file doesn't depend on Gecko being up and running, however from a more general perspective, we'd still have to wait for the JS session store to be running before we know for certain whether we've got any closed tabs to display or not.

Dropped, because at least as far as this initial implementation is concerned, we no longer want to hide the folder if it is empty.

Comment 132

[Jan Henning [:JanH]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52900/diff/5-6/

Comment 133

[Jan Henning [:JanH]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52902/diff/5-6/

Comment 134

[Jan Henning [:JanH]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52904/diff/5-6/

Comment 135

[Jan Henning [:JanH]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52906/diff/5-6/

Comment 136

[Jan Henning [:JanH]]

Comment on attachment 8757649 [details]
Bug 1251362 - Part 9 - Display a button to open all recently closed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56046/diff/2-3/

Comment 137

[Jan Henning [:JanH]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52908/diff/5-6/

Comment 138

[Jan Henning [:JanH]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52910/diff/5-6/

Comment 139

[Jan Henning [:JanH]]

Comment on attachment 8757650 [details]
Bug 1251362 - Part 12 - Remember more recently closed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56048/diff/2-3/

Comment 140

[Jan Henning [:JanH]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52914/diff/5-6/

Comment 141

[Jan Henning [:JanH]]

Comment on attachment 8755197 [details]
Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54438/diff/4-5/

Comment 142

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52918/diff/5-6/

Comment 143

[Jan Henning [:JanH]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52920/diff/5-6/

Comment 144

[Jan Henning [:JanH]]

Comment on attachment 8755198 [details]
Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54440/diff/4-5/

Comment 145

[Jan Henning [:JanH]]

Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54442/diff/4-5/

Comment 146

[Jan Henning [:JanH]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52924/diff/5-6/

Comment 147

[Jan Henning [:JanH]]

Comment on attachment 8753009 [details]
Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52926/diff/5-6/

Comment 148

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52900/#review53434

> I'd consider moving this to a separate method because it's reused, updateHeaderVisibility().

Done. I've also moved the tabs list stuff below into it's own function, too - at first I got hung up over the lack of proper delegate support in Java, but I've realised that I don't need to actually pass one of the `getFirstRecentTabIndex()` etc. functions - just passing the list index it returns and using that is enough.

> Could we make this a little simpler by reordering this into 1) notifyChanged(), 2) notifyAdded/Removed depending on the prevClosedTabsCount.
> 
> e.g.,
> 
> if (recentlyClosed.length > 0) {
>   notifyItemRangedChanged(getFirstRecentTabIndex(), MIN(recentlyClosed.length, prevClosedTabsCount));
> }
> 
> if (closedTabCountChange > 0) {
>   notifyItemRangeAdded(getLastRecentTabIndex() + 1, closedTabCountChange
> } else if (closedTabCountChange < 0) {
>   notifyItemRangeRemoved(getLastRecentTabIndex() + 1, -closedTabCountChange)
> }
> 
> (plus some // Comments)
> That way there's less nesting.

I need to think this through carefully, but your example is inserting new tabs at the bottom, whereas I want to insert them at the top, since the list is sorted newest first.
As long as it's okay to pass an itemCount of 0 to those notify... functions, it looks like it's possible to get rid of those additional if-levels anyway, though.

> Hmm, I originally had them the other way round, but then I'd reversed them because the header will only be hidden when there are no tabs to display. If the panel is empty, though, I've hidden the folder button, which means it's almost impossible to actually view the panel in its empty state - it's only accessible through the crash loop protection or if you manually enter that about:home?panel=... URL. So `isSectionHeaderVisible()` will almost always be true, whereas the position check will pass only once while going through all positions.

We're no longer hiding the folder in its empty state, but I think the above is still valid - also, in its emtpy state, the Recently closed list will contain only one item (the back button) so we never hit the section header check.

Comment 149

[Chenxia Liu [:liuche]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

https://reviewboard.mozilla.org/r/52910/#review53458

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:418
(Diff revision 6)
>                              } catch (JSONException e) {
>                                  Log.e(LOGTAG, "JSON error", e);
>                              }
>  
>                              GeckoAppShell.notifyObservers("Sanitize:ClearData", json.toString());
> +                    mRecentTabsAdapter.clearLastSessionData();

Do these tabs get cleared if the Sanitize:ClearData is called from somewhere else (like Settings)? If not, that's okay, but file a follow-up bug to handle this properly (like adding a listener for Sanitize:Finished instead).

Comment 150

[Chenxia Liu [:liuche]]

Comment on attachment 8757650 [details]
Bug 1251362 - Part 12 - Remember more recently closed tabs.

https://reviewboard.mozilla.org/r/56048/#review54192

Comment 151

[Chenxia Liu [:liuche]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

https://reviewboard.mozilla.org/r/52914/#review54194

Comment 152

[Chenxia Liu [:liuche]]

Comment on attachment 8755197 [details]
Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

https://reviewboard.mozilla.org/r/54438/#review54202

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:261
(Diff revision 5)
>  
>          final List<String> dataList = new ArrayList<>(getClosedTabsCount());
>          addTabDataToList(dataList, recentlyClosedTabs);
>          addTabDataToList(dataList, lastSessionTabs);
>  
> +        final String telemetryExtra = recentlyClosedTabs.length > 0 && lastSessionTabs.length > 0 ? TELEMETRY_EXTRA_MIXED :

Stepping back and thinking about the kinds of telemetry that we'd like to have - it's interesting to know if people are restoring recent tabs or session tabs, and it's also interesting to know if they're clicking "Open all" vs opening them one by one. I like the idea of having MIXED, but can you also add a telemetry event for clicking on the "Open all" button?

Comment 153

[Chenxia Liu [:liuche]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

https://reviewboard.mozilla.org/r/52918/#review54210

Comment 154

[Chenxia Liu [:liuche]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

https://reviewboard.mozilla.org/r/52920/#review54212

Comment 155

[Chenxia Liu [:liuche]]

Comment on attachment 8755198 [details]
Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

https://reviewboard.mozilla.org/r/54440/#review54214

Comment 156

[Chenxia Liu [:liuche]]

Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

https://reviewboard.mozilla.org/r/54442/#review54216

Comment 157

[Chenxia Liu [:liuche]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

https://reviewboard.mozilla.org/r/52924/#review54218

Comment 158

[Chenxia Liu [:liuche]]

Great! I'd also run 'mach gradle lint' to double check that references have been removed, etc.

Also, would you upload some screenshots and a build for antlam to try out? And if all is well, we should be able to land this (though I'd consider waiting until 50, on Monday, in case there are regressions).

Comment 159

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/52910/#review53458

> Do these tabs get cleared if the Sanitize:ClearData is called from somewhere else (like Settings)? If not, that's okay, but file a follow-up bug to handle this properly (like adding a listener for Sanitize:Finished instead).

In my experience, everything (including this adapter) gets destroyed as soon as about:home is exited, so the empty/non-existant sessionstore.bak is re-read the next time you're looking at the home panels. So this is only relevant when clearing history from within the home panels, but thanks anyway for the hint - I'll file a follow-up bug.

Comment 160

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/54438/#review54202

> Stepping back and thinking about the kinds of telemetry that we'd like to have - it's interesting to know if people are restoring recent tabs or session tabs, and it's also interesting to know if they're clicking "Open all" vs opening them one by one. I like the idea of having MIXED, but can you also add a telemetry event for clicking on the "Open all" button?

Hmm, isn't that already handled by the fact that clicking on a single closed tab sends `TelemetryContract.Method.LIST_ITEM`, whereas clicking on the Restore All button sends `TelemetryContract.Method.BUTTON`? The telemetry extra indicates only what kind of tab has been restored - recently closed, last session or, in the case of the button, possibly both.

Comment 161

[Jan Henning [:JanH]]

Attachment: History panel with Recently closed smart folder.png

Comment 162

[Jan Henning [:JanH]]

Attachment: Empty Recently closed smart folder.png

Comment 163

[Jan Henning [:JanH]]

Attachment: Recently closed smart folder with some tabs.png

Comment 164

[Jan Henning [:JanH]]

Final APK for testing (includes migration code):
http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-1458211b6fb17cea6874a3b1814908212f186fa1/try-android-api-15/fennec-49.0a1.en-US.android-arm.apk

Comment 165

[Chenxia Liu [:liuche]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16e3899c64b6

> Hmm, isn't that already handled by the fact that clicking on a single closed
> tab sends `TelemetryContract.Method.LIST_ITEM`, whereas clicking on the
> Restore All button sends `TelemetryContract.Method.BUTTON`? The telemetry
> extra indicates only what kind of tab has been restored - recently closed,
> last session or, in the case of the button, possibly both.

Oh you're right, carry on then.

Comment 166

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jan Henning [:JanH] from comment #164)
> Final APK for testing (includes migration code):
> http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 1458211b6fb17cea6874a3b1814908212f186fa1/try-android-api-15/fennec-49.0a1.en-
> US.android-arm.apk

(In reply to Jan Henning [:JanH] from comment #164)
> Final APK for testing (includes migration code):
> http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 1458211b6fb17cea6874a3b1814908212f186fa1/try-android-api-15/fennec-49.0a1.en-
> US.android-arm.apk

Hey JanH!

I'm testing this now and the "Recently closed" smart folder is there by default. I think I created some confusion with comment 100. I agree with your point in comment 99, we should maintain the original UX of "don't show this smart folder unless there has been a recently closed tab". 

Can we restore that behaviour?

Also, I'm going to give you a clock icon for the folder since it seems like we're just reusing the one from the empty state right now and there are some nits I have :)

Comment 167

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_recent.zip

try these :)

Comment 168

[Jan Henning [:JanH]]

(In reply to Anthony Lam (:antlam) from comment #166)  
> I'm testing this now and the "Recently closed" smart folder is there by
> default. I think I created some confusion with comment 100. I agree with
> your point in comment 99, we should maintain the original UX of "don't show
> this smart folder unless there has been a recently closed tab". 
> 
> Can we restore that behaviour?

Based on your initial reaction, liuche had preferred moving the dynamic visibility to a follow-up bug if necessary, however if you're certain you want this even in its current state, I can easily add it back in - as suggested by liuche, I'm waiting until after merge day to land this, anyway.

Comment 169

[Chenxia Liu [:liuche]]

> I'm testing this now and the "Recently closed" smart folder is there by
> default. I think I created some confusion with comment 100. I agree with
> your point in comment 99, we should maintain the original UX of "don't show
> this smart folder unless there has been a recently closed tab". 
> 
> Can we restore that behaviour?

Actually, since there is still the issue of the jump/animation when the smartfolder appears, I'd like to just split this off into a separate bug, where we can make those changes and figure out the UX (when tabs from a previous session show up async, and showing the smartfolder causes subsequent items to jump).

I've filed a follow-up bug 1277978.

Comment 170

[Jan Henning [:JanH]]

(In reply to Chenxia Liu [:liuche] from comment #158)
> Great! I'd also run 'mach gradle lint' to double check that references have
> been removed, etc.

Compared with running this on central, the number of lint warnings remains the same, which I guess is a good sign :-P Also, Android Studio isn't showing any additional unused resources, either.

I'll add the dynamic folder visibility back in and replace the tinting patch with adding and using antlam's new icons over the weekend and then hopefully this will finally be good to land next week.

Comment 171

[Jan Henning [:JanH]]

I see our comments have crossed, so I'll just update the icons and leave the rest for the follow-up.

Comment 172

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks JanH!

Comment 173

[Jan Henning [:JanH]]

Anthony, those icons you've posted seem a bit too small - on my phone for example (~ 233 dpi, i.e. hdpi), the icon is displayed as 48 * 48 screen px, which ends up noticeably blurry, because the HDPI version is currently only 27 * 27 px.

Comment 174

[Chenxia Liu [:liuche]]

JanH, what I've done in the past is put the XXHDPI icon into the drawable-nodpi folder, and let Android scale it.

There's also bug 1277999, which might mean that the icon dimensions are too big for some reason. (The current builds don't match the icons in the screenshots I took before landing the patches.)

Comment 175

[Jan Henning [:JanH]]

Ah right, the "Synced tabs" cloud has a similar resolution, so I guess that should be okay. Thanks for the suggestion.

Comment 176

[Jan Henning [:JanH]]

Attachment: Bug 1251362 - Part 3 - Add new "Recent" icon.

Bug 1277277 will track getting this icon used for the search term history, too, so we can remove the old resources (icon_most_recent_empty.png) completely.

Review commit: https://reviewboard.mozilla.org/r/58198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58198/

Comment 177

[Jan Henning [:JanH]]

Comment on attachment 8752994 [details]
Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52896/diff/4-5/

Comment 178

[Jan Henning [:JanH]]

Comment on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52916/diff/5-6/

Comment 179

[Jan Henning [:JanH]]

Comment on attachment 8752995 [details]
Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52898/diff/5-6/

Comment 180

[Jan Henning [:JanH]]

Comment on attachment 8752996 [details]
Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52900/diff/6-7/

Comment 181

[Jan Henning [:JanH]]

Comment on attachment 8752997 [details]
Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52902/diff/6-7/

Comment 182

[Jan Henning [:JanH]]

Comment on attachment 8752998 [details]
Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52904/diff/6-7/

Comment 183

[Jan Henning [:JanH]]

Comment on attachment 8752999 [details]
Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52906/diff/6-7/

Comment 184

[Jan Henning [:JanH]]

Comment on attachment 8757649 [details]
Bug 1251362 - Part 9 - Display a button to open all recently closed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56046/diff/3-4/

Comment 185

[Jan Henning [:JanH]]

Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52908/diff/6-7/

Comment 186

[Jan Henning [:JanH]]

Comment on attachment 8753001 [details]
Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52910/diff/6-7/

Comment 187

[Jan Henning [:JanH]]

Comment on attachment 8757650 [details]
Bug 1251362 - Part 12 - Remember more recently closed tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56048/diff/3-4/

Comment 188

[Jan Henning [:JanH]]

Comment on attachment 8753003 [details]
Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52914/diff/6-7/

Comment 189

[Jan Henning [:JanH]]

Comment on attachment 8755197 [details]
Bug 1251362 - Part 14 - Add telemetry for restoring tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54438/diff/5-6/

Comment 190

[Jan Henning [:JanH]]

Comment on attachment 8753005 [details]
Bug 1251362 - Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52918/diff/6-7/

Comment 191

[Jan Henning [:JanH]]

Comment on attachment 8753006 [details]
Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52920/diff/6-7/

Comment 192

[Jan Henning [:JanH]]

Comment on attachment 8755198 [details]
Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54440/diff/5-6/

Comment 193

[Jan Henning [:JanH]]

Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54442/diff/5-6/

Comment 194

[Jan Henning [:JanH]]

Comment on attachment 8753008 [details]
Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52924/diff/6-7/

Comment 195

[Jan Henning [:JanH]]

Comment on attachment 8753009 [details]
Bug 1251362 - DON'T LAND - Debug temp: If the startup tab restore setting is set to "Never", always try switching to the recent tabs panel on startup, so we can test the code path for redirecting this to the combined history panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52926/diff/6-7/

Comment 196

[Chenxia Liu [:liuche]]

Comment on attachment 8760698 [details]
Bug 1251362 - Part 3 - Add new "Recent" icon.

https://reviewboard.mozilla.org/r/58198/#review55182

Comment 197

[Jan Henning [:JanH]]

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

Comment 198

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/35e71e69db49
Part 1 - Increase SwipeRefreshLayout weight. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/3fe4b60dbcd0
Part 2 - Import OnPanelLevelChangeListener.PanelLevel. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/7e6ccddb8b09
Part 3 - Add new "Recent" icon. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/0c997d222636
Part 4 - Add a Recent Tabs folder to the Combined History panel. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/9ccad5daa84c
Part 5 - Actually show recently closed tabs when opening the smart folder. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/6d3b17555d95
Part 6 - Update empty panel state when recent tabs count changes. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/26b34894a22e
Part 7 - Update closed tabs count in the History panel main view. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/1e50a28316e7
Part 8 - Handle Recent Tabs in onItemClicked(). r=liuche
https://hg.mozilla.org/integration/fx-team/rev/8c7176aa08d7
Part 9 - Display a button to open all recently closed tabs. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/ab0c1b005ffe
Part 10 - Display a context menu for closed tab entries. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/7c334cfa74a0
Part 11 - Directly notify the RecentTabsAdapter when clearing history. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/a284a1caa422
Part 12 - Remember more recently closed tabs. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/85eee022eb7d
Part 13 - Only enable swipe to refresh within the Synced devices smart folder. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/1060ef1606c5
Part 14 - Add telemetry for restoring tabs. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/e2b22babd7b3
Part 15 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/2ded136fbae5
Part 16 - Remove the Recent Tabs panel from the default home panel config. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/22f8e6c629ea
Part 17 - Turn reading list panel migration function into a generic panel removal function. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/4509c776bcaa
Part 18 - Migrate (customised) home panel configurations. r=liuche
https://hg.mozilla.org/integration/fx-team/rev/f87d1fde25f3
Part 19 - Remove code and resources for the old Recent Tabs panel. r=liuche

Comment 199

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/35e71e69db49
https://hg.mozilla.org/mozilla-central/rev/3fe4b60dbcd0
https://hg.mozilla.org/mozilla-central/rev/7e6ccddb8b09
https://hg.mozilla.org/mozilla-central/rev/0c997d222636
https://hg.mozilla.org/mozilla-central/rev/9ccad5daa84c
https://hg.mozilla.org/mozilla-central/rev/6d3b17555d95
https://hg.mozilla.org/mozilla-central/rev/26b34894a22e
https://hg.mozilla.org/mozilla-central/rev/1e50a28316e7
https://hg.mozilla.org/mozilla-central/rev/8c7176aa08d7
https://hg.mozilla.org/mozilla-central/rev/ab0c1b005ffe
https://hg.mozilla.org/mozilla-central/rev/7c334cfa74a0
https://hg.mozilla.org/mozilla-central/rev/a284a1caa422
https://hg.mozilla.org/mozilla-central/rev/85eee022eb7d
https://hg.mozilla.org/mozilla-central/rev/1060ef1606c5
https://hg.mozilla.org/mozilla-central/rev/e2b22babd7b3
https://hg.mozilla.org/mozilla-central/rev/2ded136fbae5
https://hg.mozilla.org/mozilla-central/rev/22f8e6c629ea
https://hg.mozilla.org/mozilla-central/rev/4509c776bcaa
https://hg.mozilla.org/mozilla-central/rev/f87d1fde25f3

Comment 200

[Jan Henning [:JanH]]

Release Note Request (optional, but appreciated)
[Why is this notable]: The synced tabs moving into the history panel got their own relnote, too
[Suggested wording]: Move recently closed tabs into history panel
[Links (documentation, blog post, etc)]: none

Comment 201

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx50 (Aurora) release notes.

Comment 202

[Ioana Chiorean]

Ninu's QA work will be updated here: https://wiki.mozilla.org/QA/Fennec/Move_recent_tabs_into_history_home_panel

Comment 203

[u549602]

QA work test plan is up to date