Closed Bug 1251362 Opened 4 years ago Closed 4 years ago

Combine Recent Tabs into History tab

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox47 --- affected
relnote-firefox --- 50+
firefox50 --- fixed

People

(Reporter: liuche, Assigned: JanH)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(25 files, 3 obsolete files)

58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
105.56 KB, image/png
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
54.00 KB, image/png
Details
20.96 KB, image/png
Details
33.72 KB, image/png
Details
3.94 KB, application/zip
Details
58 bytes, text/x-review-board-request
liuche
: review+
Details
Splitting this off from bug 1220928.
Depends on: 1220928
QA Contact: mihai.ninu
No longer depends on: 1220928
Duplicate of this bug: 1223646
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!
Flags: needinfo?(jh+bugzilla)
If it's not too urgent then yes, I can try taking a stab at it.
Flags: needinfo?(jh+bugzilla)
Assignee: nobody → jh+bugzilla
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/
Attachment #8752994 - Attachment description: MozReview Request: Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight, as it contains the smart folders and otherwise on my phone a second smart folder will be overlapped by the hint we display when no history is present → MozReview Request: Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight. r=liuche
Attachment #8752995 - Attachment description: MozReview Request: Bug 1251362 - Part 2 - Show recent tabs smart folder → MozReview Request: Bug 1251362 - Part 2 - Add a Recent Tabs folder to the Combined History panel. r=liuche
Attachment #8752996 - Attachment description: MozReview Request: Bug 1251362 - Part 3 - Actually show recently closed tabs when opening the smart folder → MozReview Request: Bug 1251362 - Part 3 - Actually show recently closed tabs when opening the smart folder. r=liuche
Attachment #8752997 - Attachment description: MozReview Request: Bug 1251362 - Part 4 - Update empty panel state when recent tabs count changes → MozReview Request: Bug 1251362 - Part 4 - Update empty panel state when recent tabs count changes. r=liuche
Attachment #8752998 - Attachment description: MozReview Request: Bug 1251362 - Part 5 - Update closed tabs count in overview → MozReview Request: Bug 1251362 - Part 5 - Update closed tabs count in the History panel main view. r=liuche
Attachment #8752999 - Attachment description: MozReview Request: Bug 1251362 - Part 6 - Hook up tab restoring code on click → MozReview Request: Bug 1251362 - Part 6 - Handle Recent Tabs in onItemClicked(). r=liuche
Attachment #8753000 - Attachment description: MozReview Request: Bug 1251362 - Part 7 - Show context menu → MozReview Request: Bug 1251362 - Part 7 - Display a context menu for closed tab entries. r=liuche
Attachment #8753001 - Attachment description: MozReview Request: Bug 1251362 - Part 8 - Directly notify the RecentTabsAdapter when clearing history, so the "tabs from last time" are removed immediately instead of having to wait for the next attempt to read the data from disk (and then noticing they'r → MozReview Request: Bug 1251362 - Part 8 - Directly notify the RecentTabsAdapter when clearing history. r=liuche
Attachment #8753002 - Attachment description: MozReview Request: Bug 1251362 - Part 9 - Hide smart folder if there are no tabs to be shown → MozReview Request: Bug 1251362 - Part 9 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche
Attachment #8753003 - Attachment description: MozReview Request: Bug 1251362 - Part 10 - Only enable swipe to refresh when viewing synced tabs → MozReview Request: Bug 1251362 - Part 10 - Only enable swipe to refresh within the Synced devices smart folder. r=liuche
Attachment #8753004 - Attachment description: MozReview Request: Bug 1251362 - Part 11 - Import OnPanelLevelChangeListener.PanelLevel → MozReview Request: Bug 1251362 - Part 12 - Import OnPanelLevelChangeListener.PanelLevel. r=liuche
Attachment #8753005 - Attachment description: MozReview Request: Bug 1251362 - Part 12 - Redirect direct loads of the old recent tabs panel to the new combined history panel and directly open the recent tabs sub-view there → MozReview Request: Bug 1251362 - Part 13 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel. r=liuche
Attachment #8753006 - Attachment description: MozReview Request: Bug 1251362 - Part 13 - Remove Recent Tabs from default panel config → MozReview Request: Bug 1251362 - Part 14 - Remove the Recent Tabs panel from the default home panel config. r=liuche
Attachment #8753008 - Attachment description: MozReview Request: Bug 1251362 - Part 15 - Remove old Recent Tabs Panel code → MozReview Request: Bug 1251362 - Part 17 - Remove code and resources for the old Recent Tabs panel. r=liuche
Attachment #8755197 - Flags: review?(liuche)
Attachment #8755198 - Flags: review?(liuche)
Attachment #8755199 - Flags: review?(liuche)
Attachment #8752994 - Flags: review?(liuche)
Attachment #8752995 - Flags: review?(liuche)
Attachment #8752996 - Flags: review?(liuche)
Attachment #8752997 - Flags: review?(liuche)
Attachment #8752998 - Flags: review?(liuche)
Attachment #8752999 - Flags: review?(liuche)
Attachment #8753000 - Flags: review?(liuche)
Attachment #8753001 - Flags: review?(liuche)
Attachment #8753002 - Flags: review?(liuche)
Attachment #8753003 - Flags: review?(liuche)
Attachment #8753004 - Flags: review?(liuche)
Attachment #8753005 - Flags: review?(liuche)
Attachment #8753006 - Flags: review?(liuche)
Attachment #8753008 - Flags: review?(liuche)
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/
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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/
Attachment #8753007 - Attachment is obsolete: true
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.
Attachment #8752994 - Flags: review?(liuche) → review+
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.
Attachment #8752995 - Flags: review?(liuche) → review+
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.
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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.
Attachment #8752996 - Flags: review?(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 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.
Attachment #8752997 - Flags: review?(liuche) → review+
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
Attachment #8752998 - Flags: review?(liuche) → review+
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.
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.
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).
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.
Flags: needinfo?(jh+bugzilla)
(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".
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)?
Flags: needinfo?(jh+bugzilla) → needinfo?(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.
Flags: needinfo?(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.

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.
Attachment #8753005 - Flags: review?(liuche)
Review commit: https://reviewboard.mozilla.org/r/56044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56044/
Attachment #8753004 - Attachment description: MozReview Request: Bug 1251362 - Part 12 - Import OnPanelLevelChangeListener.PanelLevel. r=liuche → MozReview Request: Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel. r=liuche
Attachment #8752995 - Attachment description: MozReview Request: Bug 1251362 - Part 2 - Add a Recent Tabs folder to the Combined History panel. r=liuche → MozReview Request: Bug 1251362 - Part 3 - Add a Recent Tabs folder to the Combined History panel. r=liuche
Attachment #8752996 - Attachment description: MozReview Request: Bug 1251362 - Part 3 - Actually show recently closed tabs when opening the smart folder. r=liuche → MozReview Request: Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder. r=liuche
Attachment #8752997 - Attachment description: MozReview Request: Bug 1251362 - Part 4 - Update empty panel state when recent tabs count changes. r=liuche → MozReview Request: Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes. r=liuche
Attachment #8752998 - Attachment description: MozReview Request: Bug 1251362 - Part 5 - Update closed tabs count in the History panel main view. r=liuche → MozReview Request: Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view. r=liuche
Attachment #8752999 - Attachment description: MozReview Request: Bug 1251362 - Part 6 - Handle Recent Tabs in onItemClicked(). r=liuche → MozReview Request: Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked(). r=liuche
Attachment #8753000 - Attachment description: MozReview Request: Bug 1251362 - Part 7 - Display a context menu for closed tab entries. r=liuche → MozReview Request: Bug 1251362 - Part 10 - Display a context menu for closed tab entries. r=liuche
Attachment #8753001 - Attachment description: MozReview Request: Bug 1251362 - Part 8 - Directly notify the RecentTabsAdapter when clearing history. r=liuche → MozReview Request: Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history. r=liuche
Attachment #8753002 - Attachment description: MozReview Request: Bug 1251362 - Part 9 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche → MozReview Request: Bug 1251362 - Part 13 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche
Attachment #8753003 - Attachment description: MozReview Request: Bug 1251362 - Part 10 - Only enable swipe to refresh within the Synced devices smart folder. r=liuche → MozReview Request: Bug 1251362 - Part 14 - Only enable swipe to refresh within the Synced devices smart folder. r=liuche
Attachment #8755197 - Attachment description: MozReview Request: Bug 1251362 - Part 11 - Add telemetry for restoring tabs. r=liuche → MozReview Request: Bug 1251362 - Part 15 - Add telemetry for restoring tabs. r=liuche
Attachment #8753005 - Attachment description: MozReview Request: Bug 1251362 - Part 13 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel. r=liuche → MozReview Request: Bug 1251362 - Part 16 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel. r=liuche
Attachment #8753006 - Attachment description: MozReview Request: Bug 1251362 - Part 14 - Remove the Recent Tabs panel from the default home panel config. r=liuche → MozReview Request: Bug 1251362 - Part 17 - Remove the Recent Tabs panel from the default home panel config. r=liuche
Attachment #8755198 - Attachment description: MozReview Request: Bug 1251362 - Part 15 - Turn reading list panel migration function into a generic panel removal function. r=liuche → MozReview Request: Bug 1251362 - Part 18 - Turn reading list panel migration function into a generic panel removal function. r=liuche
Attachment #8755199 - Attachment description: MozReview Request: Bug 1251362 - Part 16 - Migrate (customised) home panel configurations. r=liuche → MozReview Request: Bug 1251362 - Part 19 - Migrate (customised) home panel configurations. r=liuche
Attachment #8753008 - Attachment description: MozReview Request: Bug 1251362 - Part 17 - Remove code and resources for the old Recent Tabs panel. r=liuche → MozReview Request: Bug 1251362 - Part 20 - Remove code and resources for the old Recent Tabs panel. r=liuche
Attachment #8757648 - Flags: review?(liuche)
Attachment #8757649 - Flags: review?(liuche)
Attachment #8757650 - Flags: review?(liuche)
Attachment #8752996 - Flags: review?(liuche)
Attachment #8753005 - Flags: review?(liuche)
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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/
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.
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.
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/mozilla@buttercookie.de-68f97153c96ee63c2854f662bf112b791b8683ac/try-android-api-15/fennec-49.0a1.en-US.android-arm.apk
(In reply to Jan Henning [:JanH] from comment #93)
> Updated APK for testing:
> http://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-
> 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!
Flags: needinfo?(alam) → needinfo?(jh+bugzilla)
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.
Flags: needinfo?(jh+bugzilla)
(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?
Flags: needinfo?(jh+bugzilla)
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.
Flags: needinfo?(jh+bugzilla) → needinfo?(liuche)
Actually, my mistake. I remember that now ^ from the original spec.

Let's keep it as is. 

Thanks!
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 on attachment 8753004 [details]
Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.

https://reviewboard.mozilla.org/r/52916/#review53426
Attachment #8753004 - Flags: review?(liuche) → review+
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.
Attachment #8757648 - Flags: review?(liuche) → review+
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)
Attachment #8752999 - Flags: review?(liuche) → review+
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.
Attachment #8757649 - Flags: review?(liuche) → review+
Comment on attachment 8753000 [details]
Bug 1251362 - Part 10 - Display a context menu for closed tab entries.

https://reviewboard.mozilla.org/r/52908/#review53454
Attachment #8753000 - Flags: review?(liuche) → review+
Blocks: 1277277
Attachment #8752996 - Flags: review?(liuche) → review+
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.
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.
> > 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!
Flags: needinfo?(liuche)
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 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/
Attachment #8752996 - Attachment description: MozReview Request: Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder. r=liuche → Bug 1251362 - Part 5 - Actually show recently closed tabs when opening the smart folder.
Attachment #8752997 - Attachment description: MozReview Request: Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes. r=liuche → Bug 1251362 - Part 6 - Update empty panel state when recent tabs count changes.
Attachment #8752998 - Attachment description: MozReview Request: Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view. r=liuche → Bug 1251362 - Part 7 - Update closed tabs count in the History panel main view.
Attachment #8752999 - Attachment description: MozReview Request: Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked(). r=liuche → Bug 1251362 - Part 8 - Handle Recent Tabs in onItemClicked().
Attachment #8757649 - Attachment description: MozReview Request: Bug 1251362 - Part 9 - Display a button to open all recently closed tabs. r=liuche → Bug 1251362 - Part 9 - Display a button to open all recently closed tabs.
Attachment #8753000 - Attachment description: MozReview Request: Bug 1251362 - Part 10 - Display a context menu for closed tab entries. r=liuche → Bug 1251362 - Part 10 - Display a context menu for closed tab entries.
Attachment #8753001 - Attachment description: MozReview Request: Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history. r=liuche → Bug 1251362 - Part 11 - Directly notify the RecentTabsAdapter when clearing history.
Attachment #8757650 - Attachment description: MozReview Request: Bug 1251362 - Part 12 - Remember more recently closed tabs. r=liuche → Bug 1251362 - Part 12 - Remember more recently closed tabs.
Attachment #8753003 - Attachment description: MozReview Request: Bug 1251362 - Part 14 - Only enable swipe to refresh within the Synced devices smart folder. r=liuche → Bug 1251362 - Part 13 - Only enable swipe to refresh within the Synced devices smart folder.
Attachment #8755197 - Attachment description: MozReview Request: Bug 1251362 - Part 15 - Add telemetry for restoring tabs. r=liuche → Bug 1251362 - Part 14 - Add telemetry for restoring tabs.
Attachment #8753005 - Attachment description: MozReview Request: Bug 1251362 - Part 16 - Redirect direct loads of the Recent Tabs panel about:home URL to the Recent Tabs folder of the Combined History panel. r=liuche → 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.
Attachment #8753006 - Attachment description: MozReview Request: Bug 1251362 - Part 17 - Remove the Recent Tabs panel from the default home panel config. r=liuche → Bug 1251362 - Part 16 - Remove the Recent Tabs panel from the default home panel config.
Attachment #8755198 - Attachment description: MozReview Request: Bug 1251362 - Part 18 - Turn reading list panel migration function into a generic panel removal function. r=liuche → Bug 1251362 - Part 17 - Turn reading list panel migration function into a generic panel removal function.
Attachment #8755199 - Attachment description: MozReview Request: Bug 1251362 - Part 19 - Migrate (customised) home panel configurations. r=liuche → Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.
Attachment #8753008 - Attachment description: MozReview Request: Bug 1251362 - Part 20 - Remove code and resources for the old Recent Tabs panel. r=liuche → Bug 1251362 - Part 19 - Remove code and resources for the old Recent Tabs panel.
Attachment #8753009 - Attachment description: MozReview Request: 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. → 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.
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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/
Attachment #8753002 - Attachment is obsolete: true
Attachment #8753002 - Flags: review?(liuche)
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 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).
Attachment #8753001 - Flags: review?(liuche) → review+
Comment on attachment 8757650 [details]
Bug 1251362 - Part 12 - Remember more recently closed tabs.

https://reviewboard.mozilla.org/r/56048/#review54192
Attachment #8757650 - Flags: review?(liuche) → review+
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
Attachment #8753003 - Flags: review?(liuche) → review+
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?
Attachment #8755197 - Flags: review?(liuche) → review+
Attachment #8753005 - Flags: review?(liuche) → review+
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 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
Attachment #8753006 - Flags: review?(liuche) → review+
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
Attachment #8755198 - Flags: review?(liuche) → review+
Attachment #8755199 - Flags: review?(liuche) → review+
Comment on attachment 8755199 [details]
Bug 1251362 - Part 18 - Migrate (customised) home panel configurations.

https://reviewboard.mozilla.org/r/54442/#review54216
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
Attachment #8753008 - Flags: review?(liuche) → review+
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).
Blocks: 1277800
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.
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.
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.
(In reply to Jan Henning [:JanH] from comment #164)
> Final APK for testing (includes migration code):
> http://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-
> 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/mozilla@buttercookie.de-
> 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 :)
Flags: needinfo?(alam)
Attached file icon_recent.zip
try these :)
Flags: needinfo?(jh+bugzilla)
(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.
Flags: needinfo?(jh+bugzilla)
Blocks: 1277978
> 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.
(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.
I see our comments have crossed, so I'll just update the icons and leave the rest for the follow-up.
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.
Flags: needinfo?(alam)
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.)
Ah right, the "Synced tabs" cloud has a similar resolution, so I guess that should be okay. Thanks for the suggestion.
Flags: needinfo?(alam)
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/
Attachment #8752994 - Attachment description: MozReview Request: Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight. r=liuche → Bug 1251362 - Part 1 - Increase SwipeRefreshLayout weight.
Attachment #8753004 - Attachment description: MozReview Request: Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel. r=liuche → Bug 1251362 - Part 2 - Import OnPanelLevelChangeListener.PanelLevel.
Attachment #8752995 - Attachment description: MozReview Request: Bug 1251362 - Part 3 - Add a Recent Tabs folder to the Combined History panel. r=liuche → Bug 1251362 - Part 4 - Add a Recent Tabs folder to the Combined History panel.
Attachment #8760698 - Flags: review?(liuche)
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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/
Attachment #8757648 - Attachment is obsolete: true
Comment on attachment 8760698 [details]
Bug 1251362 - Part 3 - Add new "Recent" icon.

https://reviewboard.mozilla.org/r/58198/#review55182
Attachment #8760698 - Flags: review?(liuche) → review+
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
Depends on: 1274238
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
relnote-firefox: --- → ?
Ninu's QA work will be updated here: https://wiki.mozilla.org/QA/Fennec/Move_recent_tabs_into_history_home_panel
Flags: needinfo?(mihai.ninu)
QA work test plan is up to date
Flags: needinfo?(mihai.ninu)
You need to log in before you can comment on or make changes to this bug.