1220928 - Combine Synced Tabs and History home panels

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Chenxia Liu [:liuche]]

We've been talking about how history in Fennec can be confusing because we have 3 home panels dedicated to tab history - recent tabs, history, and synced tabs. Top Sites could be considered another one, because it's frecency history.

We should simplify our history display to fewer panels, to lighten the cognitive load.

One suggestion would be to have a single tab for Recent tabs, (collapsed) history, (collapsed) synced tabs, so all-things-history are in one panel. If we wanted to further cut down on home panels, we could combine all of these previously mentioned history items under the Top Sites grid view, keeping to Top Sites grid items but replacing the Top Sites list items (on beta, the combined clicks on list items is less than even the lowest Top Sites grid item on phones, and about 1/20 of the clicks of the top clicked grid item).

Comment 1

[Richard Newman [:rnewman]]

CCing Robin and Maria, because we've talked about this.

Worth looking at the Chrome home and history views on iOS for an example of how these might be unified.

Comment 2

[Maria Popova]

This is logged under Android - can we make it generic for Mobile (cloning to iOS may create more issues)? I'm very much in favor of simplifying history-related panels

Comment 3

[:Margaret Leibovic]

I think this is a way for us to address bug 1195728 without creating more UX around our panels. To solve the problem of too many panels, let's have fewer panels! :)

Comment 4

[Richard Newman [:rnewman]]

(In reply to mpopova from comment #2)
> This is logged under Android - can we make it generic for Mobile (cloning to
> iOS may create more issues)? I'm very much in favor of simplifying
> history-related panels

Our panels do differ between iOS and Android, the work involved definitely does, and our tracking flags do too, so it's probably best that we have two (or more) bugs.

I do agree that we have similar issues to solve on both platforms (and on desktop!), so let's discuss with Robin and Anthony, and make the space we need to reconsider the problem and figure out what we might do.

I'll try to be the bridge to the history work we're thinking about in services and on desktop in the next year…

Comment 5

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

CC'ing Nick Chapman

This might overlap with some of Nick's ideas for a better task continuity experience.

Comment 6

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

I think this is something that we'd like to look at holistically with our other known Home Panel issues. I've be thinking about this more with Darrin and Robin so we can keep these components across mobile feeling more similar but not necessarily being the same.

more to come

Comment 7

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

Attachment: prev_combinedhistory1.png

Here's a WIP prototype of what I'm thinking as a "merged history" panel.

This places the synced devices at the top (collapsed by default), followed by the aggregated History list. 

There are some UI issues that we will need to tweak slightly to align better with the guidelines around these list items but it shouldn't be too much change. 

For example, the date divider (and "# devices hidden" item) in this mock now don't have as much visual prominence. That's done by adjusting the vertical height of these elements to be more consistent with each other, while remaining balanced against the vertical height of the regular list items.

Comment 8

[Chenxia Liu [:liuche]]

Looks good! Does this mean we're getting rid of "recently closed X" completely, and you'll just go through "Today" history to see the tabs you've most recently opened?

Comment 9

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #8)
> Looks good! Does this mean we're getting rid of "recently closed X"
> completely, and you'll just go through "Today" history to see the tabs
> you've most recently opened?

We'll have to figure out what this means for restoring these tabs. This would also remove the "Open all" functionality that we currently have in the recent tabs panel.

Given how few people use that panel, maybe it's fine to get rid of these buttons? We should look at telemetry data.

We could also provide a different way to restore your previous session. But maybe if you don't have session restore enabled by default, you don't really care.

In any case, I think we should implement this as a completely new panel, and we can worry about removing the old panels once we've either ported or decided to drop their functionality.

Comment 10

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

From a UX point of view, I think removing that panel works. 

But, I do think there's some connection with the "Restore tabs" settings pref that is OFF by default. I think it's ON by default in iOS and perhaps having that ON would help the situation.

That being said, if we feel that we need to test this, creating this as a new panel would be a good start.

Comment 11

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #10)
> From a UX point of view, I think removing that panel works. 
> 
> But, I do think there's some connection with the "Restore tabs" settings
> pref that is OFF by default. I think it's ON by default in iOS and perhaps
> having that ON would help the situation.

I am interested to know how much people use this restore tabs functionality. We do have a probe for how often people hit the "Open all" buttons - it's the "button" method here:
https://www.dropbox.com/s/4gxvuxguz158tyy/Screenshot%202016-01-05%2020.42.26.PNG?dl=0

But right now we don't distinguish between recently closed tabs and tabs from last time. I'll file a bug to improve the probes here.

However, you can see that clearly this is a pretty small percentage of the URL loads that come from the home panels.

Comment 12

[Ben Bucksch (:BenB)]

I agree that the 4 panels are confusing and too similar.

OTOH, they serve different purposes. For example, I want to use only Bookmarks and (Full) History. I never want to use Top Sites nor Synced Tabs (because I don't use sync). "Restored Tabs" shouldn't even be a (long-living) panel, because I need it only when I start, just restore them (after asking me) and then I don't need them anymore. "Synced Tabs" could probably work the same way: They should be actualy tabs, not links on a panel. So, that would already remove "Restored Tabs" and "Synced Tabs"

That leaves only Bookmarks, Top Sites and Full History. I think these are easy enough to understand. It's a question of preference and work organization whether the user uses Bookmarks or Top Sites or both. I'm a control freak and want to organize my start page, including what appears and what does not, in which order, and with what title (thus I use "Bookmarks"), others couldn't care less and want it to be done automatically ("Top Sites").

Comment 13

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

Attachment: prev_history_mock3.png

Talked about this more IRL - here's the updated mock. We'll reuse the current Bookmarks folder UX.

Comment 14

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

Attachment: prev_history_mock4b.png

Comment 15

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

Attachment: prev_history_mock4.png

Comment 16

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

Attachment: Screen Shot 2016-02-25 at 11.13.04 AM.png

Comment 17

[Chenxia Liu [:liuche]]

Morphing this bug to split the scope of this bug.

Combining Recent Tabs will now be tracked by bug 1251362.

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Screenshot A: Combined History/Synced Tabs panel

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Screenshot B: "Child" view of tabs of a synced client

A few more things that need to be done:

* Add date/month headers to History view
* Update footer button in child view to say "Open all"
* Add context menu items
* Clean up code to make this more extensible for other panels

And as a next part, port the tablet dual-fragment view.

Comment 20

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 21

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 22

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 23

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 24

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 25

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 26

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 27

[Chenxia Liu [:liuche]]

And some WIP patches for this work.

Comment 28

[Chenxia Liu [:liuche]]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 29

[Chenxia Liu [:liuche]]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 30

[Chenxia Liu [:liuche]]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 31

[Chenxia Liu [:liuche]]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 32

[Chenxia Liu [:liuche]]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 33

[Chenxia Liu [:liuche]]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 34

[Chenxia Liu [:liuche]]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 35

[Chenxia Liu [:liuche]]

I cleaned up all the previous code and fixed all the little problems from this being a hack, and also added RecyclerView decorations.

All these patches so far should be ready for review, though there is more to come:
- Adding date headers for history
- Handling the empty state
- Context menus
- Hiding hidden remote clients
- Handling tablet state

And of course removing the HistoryPanel and replacing it with the CombinedHistoryPanel.

Comment 36

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Combined History/Synced Tabs panel v2

Comment 37

[Chenxia Liu [:liuche]]

Attachment: Screenshot: "Child" view of tabs of a synced client

Some updated screenshots.

Comment 38

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

Is it just me, or does the Device row in the main History page look like a header and the history items look like history from that device?

Shouldn't the Device row give some indication that it spawns a child? Like a ">" on the right side? Don't we use that for folder? Isn't this similar in spirit?

Are we worried about having several Device rows at the top?

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

https://reviewboard.mozilla.org/r/36983/#review35867

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:19
(Diff revision 2)
> +    private enum ItemType {
> +        CLIENT, HISTORY
> +    }

Just from this patch's point of view I'd use int constants instead of an enum; they have less overhead and your are only using .ordinal() here anyways. But maybe this changes in the other patches. :)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:34
(Diff revision 2)
> +                view = inflater.inflate(R.layout.home_item_row, viewGroup, false);
> +                return new CombinedHistoryItem(view);

NIT: Indentation

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:41
(Diff revision 2)
> +        final int numClients = remoteClients == null ? 0 : remoteClients.size();

This code is not complete yet, but often this code is much simpler if we guarantee to always have a list (empty) and skip the null checks. Collections.emptyList() is helpful for that.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryItem.java:15
(Diff revision 2)
> +    public static class ClientItem extends CombinedHistoryItem {

It took a while to understand this: The inner class extends the outer class. Why not just two separate ViewHolder classes? Introduce an interface if you want the adapter to handle both through shared methods.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryRecyclerView.java:36
(Diff revision 2)
> +        layoutManager.scrollToPosition(0);

Is position 0 not the default anyways? :)

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

https://reviewboard.mozilla.org/r/36985/#review35869

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:31
(Diff revision 2)
> +    public void replaceClients(List<RemoteClient> clients) {
> +        remoteClients = clients;
> +        notifyDataSetChanged();
> +    }
> +
> +    public void replaceHistory(Cursor history) {

I feel like setHistory() or setClients() would be easier to understand without needing to read the code? I was confused by "replace". :)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:54
(Diff revision 2)
> +        context = viewGroup.getContext();

Maybe consider making this an explicit constructor argument if other methods need this. This will avoid NullPointerExceptions and there's no implicit "you can only use 'context' after onCreateViewHolder() has been called at least once".

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:80
(Diff revision 2)
> +        final int localPosition = transformPosition(itemType, position);
> +        switch (itemType) {
> +            case CLIENT:
> +                final CombinedHistoryItem.ClientItem clientItem = (CombinedHistoryItem.ClientItem) viewHolder;
> +                final RemoteClient client = remoteClients.get(localPosition);
> +                clientItem.nameView.setText(client.name);
> +                clientItem.nameView.setTextColor(ContextCompat.getColor(context, R.color.placeholder_active_grey));
> +                clientItem.deviceTypeView.setImageResource("desktop".equals(client.deviceType) ? R.drawable.sync_desktop : R.drawable.sync_mobile);
> +
> +                final long now = System.currentTimeMillis();
> +                clientItem.lastModifiedView.setText(RemoteTabsExpandableListAdapter.getLastSyncedString(context, now, client.lastModified));
> +                break;
> +
> +            case HISTORY:
> +                final TwoLinePageRow pageRow = (TwoLinePageRow) viewHolder.itemView;
> +                pageRow.setShowIcons(true);
> +                if (historyCursor == null || !historyCursor.moveToPosition(localPosition)) {
> +                    throw new IllegalStateException("Couldn't move cursor to position " + localPosition);
> +                }
> +                pageRow.updateFromCursor(historyCursor);
> +                break;
> +        }
> +    }

I often move this code into a bind() method into the view holders. The code here will just delegate and every view holder is only handling itself (simple code).

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:48
(Diff revision 2)
> +        mClearHistoryButton = view.findViewById(R.id.clear_history_button);

Just FYI: Currently the "clear history" button is shielded by Restrictions.isAllowed(context, Restrictable.CLEAR_HISTORY); A restricted profile might not be allowed to clear the history.

Comment 41

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

https://reviewboard.mozilla.org/r/36989/#review35871

You could move some of that code into methods of the ItemType enum: toViewType(), createViewHolder(), static fromViewType(int). Not sure if this more readable?

Comment 42

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

https://reviewboard.mozilla.org/r/36991/#review35873

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryRecyclerView.java:15
(Diff revision 2)
> +import java.util.EnumSet;

Oh cool, I have never used EnumSet. This seems to be handy.

Comment 43

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

(In reply to Mark Finkle (:mfinkle) from comment #38)
> Is it just me, or does the Device row in the main History page look like a
> header and the history items look like history from that device?

Yeah, I noticed that too. I believe :liuche polished up the padding there so that the labels were all vertically aligned. That way, there is no indentation.

> Shouldn't the Device row give some indication that it spawns a child? Like a
> ">" on the right side? Don't we use that for folder? Isn't this similar in
> spirit?

Actually, we don't have arrows for our folders currently. So, the plan right now is to revert the visual style to be consistent with our current folders UI. That is, removing the ">". We may bring this back later for actual "collapse/expand"-able items in the panel.

> Are we worried about having several Device rows at the top?

Not particularly. Since we know most users aren't really going to be having more than 3 or so. I have 5 and it still works fine I think. 

After all, this is the "biggest bucket" of the three main panels.

Comment 44

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/36983/#review35867

> It took a while to understand this: The inner class extends the outer class. Why not just two separate ViewHolder classes? Introduce an interface if you want the adapter to handle both through shared methods.

Yeah, the original intent behind this was for 1) grouping all the ViewHolders together and 2) the History ViewHolder didn't do anything, so it seemed silly to give it an empty class. However, I think it's a good point that this is confusing, so I think I'll use two classes and also put the onBindView code into each ViewHolder class.

RecyclerView.Adapter requires a single type as the generic, and it also requires that that type inherit from ViewHolder, so trying out an interface doesn't work because interfaces can't extend any classes.

Comment 45

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/36985/#review35869

> Just FYI: Currently the "clear history" button is shielded by Restrictions.isAllowed(context, Restrictable.CLEAR_HISTORY); A restricted profile might not be allowed to clear the history.

I'll address this in the empty state patch! Thanks for reminding me.

Comment 46

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/36989/#review35871

Good call! I'll do that for the toViewType and fromViewType calls. I'd rather keep the createViewHolder call where it is, because I think the ItemType interface can be separate from layouts.

Comment 47

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

https://reviewboard.mozilla.org/r/36993/#review36339

Comment 48

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

https://reviewboard.mozilla.org/r/36995/#review36343

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:69
(Diff revision 2)
> +        mPanelFooterButton.setOnClickListener(new OnFooterButtonClickListener());
> +        mPanelFooterButton.setVisibility(View.VISIBLE);

Restrictions reminder. :)

Comment 49

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

https://reviewboard.mozilla.org/r/36987/#review36345

Comment 50

[Chenxia Liu [:liuche]]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 51

[Chenxia Liu [:liuche]]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 52

[Chenxia Liu [:liuche]]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 53

[Chenxia Liu [:liuche]]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 54

[Chenxia Liu [:liuche]]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 55

[Chenxia Liu [:liuche]]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 56

[Chenxia Liu [:liuche]]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 57

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

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

Comment 58

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add empty state. r=sebastian

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

Comment 59

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/40281/#review36793

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:255
(Diff revision 1)
>                      }
>                      break;
>              }
>          }
>      }
> +

I'm not super happy about this logic but it seems necessary to use the ViewStub/avoid inflating this view every time. With ListViews you could use setEmptyView and the listview would handle hiding and showing the view (I copied this empty view code from HistoryPanel), but recyclerview doesn't have the same [1].

[1] Although databinding suggests itself as a solution! However, I decided to forgo DB for now, because I'm not sure how it would interact with view stubs, though maybe we could bind the data inside of the viewstub (though since that's reused by all the home panels, this might require more refactoring and I didn't want to open that can of worms right now).

Comment 60

[Chenxia Liu [:liuche]]

Also, sebastian - I added the logic for detecting Restricted profile, but the restricted profile restrictions didn't seem to work for me. I filed bug 1257302 for that, but I'd like to test these patches out again before I land them (since I wasn't able to actually verify they worked).

Comment 61

[Chenxia Liu [:liuche]]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 62

[Chenxia Liu [:liuche]]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 63

[Chenxia Liu [:liuche]]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 64

[Chenxia Liu [:liuche]]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 65

[Chenxia Liu [:liuche]]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 66

[Chenxia Liu [:liuche]]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 67

[Chenxia Liu [:liuche]]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 68

[Chenxia Liu [:liuche]]

Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

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

Comment 69

[Chenxia Liu [:liuche]]

Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

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

Comment 70

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add section headers. r=sebastian

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

Comment 71

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Combined History/Synced Tabs panel v3

I pulled a lot of this code from HistoryPanel/Adapter, but cleaned it up a bit, fixed some bugs, renamed things to make them clearer, and also added more comments.

Comment 72

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

https://reviewboard.mozilla.org/r/40279/#review37559

Comment 73

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

https://reviewboard.mozilla.org/r/40281/#review37563

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:175
(Diff revision 2)
>                      break;
>              }
>  
> +            // Check and set empty state.
> +            updateButtonFromLevel(OnPanelLevelChangeListener.PanelLevel.PARENT);
> +            showEmptyView(mAdapter.getItemCount() == 0);

The name is a bit confusing: I expect showEmptyView() to always show the view but it sometimes hides the view too.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:284
(Diff revision 2)
> +                emptyHint.setVisibility(View.GONE);
> +            }
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (isEmpty) {
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (mEmptyView != null){

Nit: Space before {

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:285
(Diff revision 2)
> +            }
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (isEmpty) {
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (mEmptyView != null){
> +            mEmptyView.setVisibility(View.GONE);

NIT: I had a bit problems following this if/else tree for the different situations. Particularly here I was stuck: Why do we not hide the view if it is empty. But then I realized we do: At this point isEmpty has to be false.

I wonder if this could be restructured to be more clear:

if (isEmpty) {
  if (mEmptyView == null) {
    // create view
  }
  mEmptyView.setVisibility(..);
} else {
  if (mEmptyView != null) {
    mEmptyView.setVisibility(..);
  }
}

What do you think? Your choice.

Comment 74

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

https://reviewboard.mozilla.org/r/40717/#review37567

I remember seeing something similar (for quick jumping to sections in a list of text) in the framework using a binary search:
https://android.googlesource.com/platform/frameworks/base.git/+/master/core/java/android/widget/AlphabetIndexer.java
I don't think that's needed here but maybe it's interesting reference code. :)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:185
(Diff revision 1)
> +     * @param sparseArray SparseArray to populate
> +     */
> +    private static void populateSectionHeaders(Cursor c, SparseArray<SectionHeader> sparseArray) {
> +        sparseArray.clear();
> +
> +        if (c == null || !c.moveToFirst()) {

NIT: You could ommit the moveToFirst() here and then start your loop with moveToNext():

while (cursor.moveToNext()) {
..
}

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:191
(Diff revision 1)
> +            final int historyPosition = c.getPosition();
> +            final long visitTime = c.getLong(c.getColumnIndexOrThrow(BrowserContract.History.DATE_LAST_VISITED));
> +            final SectionHeader itemSection = CombinedHistoryPanel.getSectionFromTime(visitTime);

I wonder if there's a performance gain if those variables are declared outside the loop and reused.

Comment 75

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/40717/#review37567

> NIT: You could ommit the moveToFirst() here and then start your loop with moveToNext():
> 
> while (cursor.moveToNext()) {
> ..
> }

Hmm, I think that's a good point, since we're using the cursor immediately after setting it in the adapter. But since this is a static method, I'd like to guarantee that the cursor is at the right place when I start, so it doesn't need to assume any state.

Comment 76

[Chenxia Liu [:liuche]]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 77

[Chenxia Liu [:liuche]]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 78

[Chenxia Liu [:liuche]]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 79

[Chenxia Liu [:liuche]]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 80

[Chenxia Liu [:liuche]]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 81

[Chenxia Liu [:liuche]]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 82

[Chenxia Liu [:liuche]]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 83

[Chenxia Liu [:liuche]]

Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

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

Comment 84

[Chenxia Liu [:liuche]]

Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

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

Comment 85

[Chenxia Liu [:liuche]]

Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

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

Comment 86

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add context menu. r=sebastian

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

Comment 87

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

***

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

Comment 88

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

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

Comment 89

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

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

Comment 90

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

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

Comment 91

[Chenxia Liu [:liuche]]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 92

[Chenxia Liu [:liuche]]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 93

[Chenxia Liu [:liuche]]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 94

[Chenxia Liu [:liuche]]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 95

[Chenxia Liu [:liuche]]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 96

[Chenxia Liu [:liuche]]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 97

[Chenxia Liu [:liuche]]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 98

[Chenxia Liu [:liuche]]

Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

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

Comment 99

[Chenxia Liu [:liuche]]

Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

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

Comment 100

[Chenxia Liu [:liuche]]

Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

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

Comment 101

[Chenxia Liu [:liuche]]

Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

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

Comment 102

[Chenxia Liu [:liuche]]

I'd like to finish up the phone mode of combined history panels, and then handle the tablet mode in a follow-up so we get more Nightly testing time, so I filed bug 1260320.

This also doesn't handle migrations of users with custom home panels, which I will do in bug 1260321.

Comment 103

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/42135/#review39431

There's a typo in my name and reviewboard didn't flag me to review this patch. :)

Comment 104

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

https://reviewboard.mozilla.org/r/42135/#review39433

Comment 105

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

https://reviewboard.mozilla.org/r/42817/#review39435

There are two MozReview-Commit-ID in the commit message - this looks wrong. Maybe from collapsing two commits?

Comment 106

[Chenxia Liu [:liuche]]

Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

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

Comment 107

[Chenxia Liu [:liuche]]

Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

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

Comment 108

[Chenxia Liu [:liuche]]

Comment on attachment 8735655 [details]
MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

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

Comment 109

[Chenxia Liu [:liuche]]

Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

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

Comment 110

[Chenxia Liu [:liuche]]

Comment on attachment 8735657 [details]
MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

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

Comment 111

[Chenxia Liu [:liuche]]

Whoops! Thanks for reviewing that patch anyways :) I fixed the mozreview commit ids - I think I had pushed a draft and then collapsed two commits and didn't delete the combined commit messages.

Comment 112

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Combined History/Synced Tabs panel v4 (with hidden clients)

Hiding and showing the clients also uses the recyclerview sliding animation. I didn't do this for the history items because a) it's (read-only) cursor and we requery the database and b) it's inconsistent with how we remove link/history items in other panels, so I'll see if antlam cares about that inconsistency there.

Comment 113

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

https://reviewboard.mozilla.org/r/42817/#review39545

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:53
(Diff revision 2)
>      private List<RemoteClient> remoteClients = Collections.emptyList();
>      private List<RemoteTab> clientChildren;
>      private Cursor historyCursor;
>  
> +    // Maintain group collapsed and hidden state. Only accessed from the UI thread.
> +    protected static RemoteTabsExpandableListState sState;

Why is this static? From a quick glimpse I doesn't seem to be very expensive. It reads and writes from SharedPreferences.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:125
(Diff revision 2)
> +                if (!hasHiddenClients) {
> +                    // Add item for unhiding clients;
> +                    remoteClients.add(null);
> +                } else {
> +                    // Update hidden devices number.
> +                    notifyItemChanged(remoteClients.size() - 1);

"remoteClients.size() - 1" is used multiple times and I wonder if we should hide this behind a helper method?

Comment 114

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735655 [details]
MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

https://reviewboard.mozilla.org/r/42891/#review39547

Comment 115

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

https://reviewboard.mozilla.org/r/42893/#review39549

Should those config changes be behind a build or switchboard flag?

Comment 116

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

https://reviewboard.mozilla.org/r/42893/#review39551

Comment 117

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735657 [details]
MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

https://reviewboard.mozilla.org/r/42895/#review39553

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:48
(Diff revision 2)
>          public static int itemTypeToViewType(ItemType itemType) {
>              return itemType.ordinal();
>          }
>      }
>  
> +

Nit: Empty line?

Comment 118

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/42817/#review39645

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:53
(Diff revision 2)
>      private List<RemoteClient> remoteClients = Collections.emptyList();
>      private List<RemoteTab> clientChildren;
>      private Cursor historyCursor;
>  
> +    // Maintain group collapsed and hidden state. Only accessed from the UI thread.
> +    protected static RemoteTabsExpandableListState sState;

This is static to decrease racing because ideally there is only on sState - although according to the RemoteTabs code, even if it did race, this shouldn't be a big deal because it ends up being backed by SharedPreferences.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:125
(Diff revision 2)
> +                if (!hasHiddenClients) {
> +                    // Add item for unhiding clients;
> +                    remoteClients.add(null);
> +                } else {
> +                    // Update hidden devices number.
> +                    notifyItemChanged(remoteClients.size() - 1);

Okay, I added a separate method, with a comment.

Of note, the ordering of notifyItem...() calls does matter, which messed me up a bit here.
http://www.birbit.com/recyclerview-animations-part-2-behind-the-scenes/

Comment 119

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/42893/#review39549

Putting these changes behind a switchboard flag makes it a little hard for migration, so we decided to not do that - this is just a partial landing to get everything out for more Nightly testing while I finish the tablet mode.

Comment 120

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d64938914b3e
https://hg.mozilla.org/integration/fx-team/rev/ceb669fb17f2
https://hg.mozilla.org/integration/fx-team/rev/f464c44bf882
https://hg.mozilla.org/integration/fx-team/rev/6220841fe652
https://hg.mozilla.org/integration/fx-team/rev/875fbe8190d3
https://hg.mozilla.org/integration/fx-team/rev/297774d61b59
https://hg.mozilla.org/integration/fx-team/rev/450290121373
https://hg.mozilla.org/integration/fx-team/rev/f2a5f6a248ed
https://hg.mozilla.org/integration/fx-team/rev/4fc2017aee75
https://hg.mozilla.org/integration/fx-team/rev/671ce80172a0
https://hg.mozilla.org/integration/fx-team/rev/b2828872f3e7
https://hg.mozilla.org/integration/fx-team/rev/81b90ed9ff46
https://hg.mozilla.org/integration/fx-team/rev/916fb74ed491
https://hg.mozilla.org/integration/fx-team/rev/ac7d8d5ec09e
https://hg.mozilla.org/integration/fx-team/rev/c1a1079daea1

Comment 121

[Chenxia Liu [:liuche]]

Note, this is a partial landing of the full feature because there are a lot of patches and we wanted to get this into Nightly.

STR: for devices without customized home panels (e.g., have not hidden or reordered or added any panels from settings). This does not implement landscape tablet mode.

- Removes Synced Tabs panel
- Shows clients in the History panel, and allows navigating into them to see synced tabs
- Maintains all old history behavior

Does not handle split-pane tablet landscape mode, or customized home panels.

Comment 122

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8311619&repo=fx-team

Comment 123

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/c89262d70608
https://hg.mozilla.org/integration/fx-team/rev/f4449c8ce9a8
https://hg.mozilla.org/integration/fx-team/rev/a30dd3829cd0
https://hg.mozilla.org/integration/fx-team/rev/3cdaff81f4d5
https://hg.mozilla.org/integration/fx-team/rev/9c2817f5c900
https://hg.mozilla.org/integration/fx-team/rev/0bc1a886196e
https://hg.mozilla.org/integration/fx-team/rev/19947a4a78b4
https://hg.mozilla.org/integration/fx-team/rev/48cabf56e441
https://hg.mozilla.org/integration/fx-team/rev/81a8f88a50c3
https://hg.mozilla.org/integration/fx-team/rev/bacf5386e4b7
https://hg.mozilla.org/integration/fx-team/rev/7709b4037ab1
https://hg.mozilla.org/integration/fx-team/rev/6c3e6356f8a7
https://hg.mozilla.org/integration/fx-team/rev/d27578912ddd
https://hg.mozilla.org/integration/fx-team/rev/52247b54de2a
https://hg.mozilla.org/integration/fx-team/rev/435d12d164aa

Comment 124

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1220928 - Update test references to old history panel. r=sebastian

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

Comment 125

[Chenxia Liu [:liuche]]

Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

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

Comment 126

[Chenxia Liu [:liuche]]

Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

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

Comment 127

[Chenxia Liu [:liuche]]

Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

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

Comment 128

[Chenxia Liu [:liuche]]

Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

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

Comment 129

[Chenxia Liu [:liuche]]

Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

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

Comment 130

[Chenxia Liu [:liuche]]

Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

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

Comment 131

[Chenxia Liu [:liuche]]

Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

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

Comment 132

[Chenxia Liu [:liuche]]

Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

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

Comment 133

[Chenxia Liu [:liuche]]

Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

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

Comment 134

[Chenxia Liu [:liuche]]

Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

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

Comment 135

[Chenxia Liu [:liuche]]

Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

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

Comment 136

[Chenxia Liu [:liuche]]

Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

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

Comment 137

[Chenxia Liu [:liuche]]

Comment on attachment 8735655 [details]
MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

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

Comment 138

[Chenxia Liu [:liuche]]

Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

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

Comment 139

[Chenxia Liu [:liuche]]

Comment on attachment 8735657 [details]
MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

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

Comment 140

[Chenxia Liu [:liuche]]

(oh man, that was a lot of comments...!)

I forgot to update the tests, and did so in this last patch.

Green try run here (with some unrelated telemetry orange): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2705c212a6c9dfd5a940dd5a4b9728dab9d2311

Comment 141

[Chenxia Liu [:liuche]]

Comment on attachment 8736516 [details]
MozReview Request: Bug 1220928 - Update test references to old history panel. r=sebastian

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

Comment 142

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736516 [details]
MozReview Request: Bug 1220928 - Update test references to old history panel. r=sebastian

https://reviewboard.mozilla.org/r/43337/#review40047

Comment 143

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c3b0d96c2329
https://hg.mozilla.org/integration/fx-team/rev/da52ef729a4b
https://hg.mozilla.org/integration/fx-team/rev/82af1bb19859
https://hg.mozilla.org/integration/fx-team/rev/df8b8021d419
https://hg.mozilla.org/integration/fx-team/rev/1139e8f7b278
https://hg.mozilla.org/integration/fx-team/rev/fe241494b578
https://hg.mozilla.org/integration/fx-team/rev/60cd54c9bf89
https://hg.mozilla.org/integration/fx-team/rev/1ee51352897e
https://hg.mozilla.org/integration/fx-team/rev/5539cc5e5980
https://hg.mozilla.org/integration/fx-team/rev/024280137587
https://hg.mozilla.org/integration/fx-team/rev/d55a8f609a81
https://hg.mozilla.org/integration/fx-team/rev/72040ea10a61
https://hg.mozilla.org/integration/fx-team/rev/c5cdf619cd2e
https://hg.mozilla.org/integration/fx-team/rev/c63615d8c252
https://hg.mozilla.org/integration/fx-team/rev/251bd4b73f61
https://hg.mozilla.org/integration/fx-team/rev/af73f5dcd1b1

Comment 144

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

https://hg.mozilla.org/mozilla-central/rev/c3b0d96c2329
https://hg.mozilla.org/mozilla-central/rev/da52ef729a4b
https://hg.mozilla.org/mozilla-central/rev/82af1bb19859
https://hg.mozilla.org/mozilla-central/rev/df8b8021d419
https://hg.mozilla.org/mozilla-central/rev/1139e8f7b278
https://hg.mozilla.org/mozilla-central/rev/fe241494b578
https://hg.mozilla.org/mozilla-central/rev/60cd54c9bf89
https://hg.mozilla.org/mozilla-central/rev/1ee51352897e
https://hg.mozilla.org/mozilla-central/rev/5539cc5e5980
https://hg.mozilla.org/mozilla-central/rev/024280137587
https://hg.mozilla.org/mozilla-central/rev/d55a8f609a81
https://hg.mozilla.org/mozilla-central/rev/72040ea10a61
https://hg.mozilla.org/mozilla-central/rev/c5cdf619cd2e
https://hg.mozilla.org/mozilla-central/rev/c63615d8c252
https://hg.mozilla.org/mozilla-central/rev/251bd4b73f61
https://hg.mozilla.org/mozilla-central/rev/af73f5dcd1b1

Comment 145

[Chenxia Liu [:liuche]]

Hi Ninu, it looks like you pinged me on irc but I wasn't around. In the future, just comment in bugzilla and needinfo me.

I put an STR in this bug (comment #121) but it might have gotten lost in the backouts/review requests. Take a look there, and also at the dependencies on this bug, and the history panel metabug (bug 1262929).

Let me know if you have any more questions.

(Also, if you could add a [:ninu] to your Bugzilla name, it'll help people quickly NI you on bugzilla.)

Comment 146

[u549602]

Hi Chenxia, 

Noted, all clear now, thanks a lot for the explanations, if i have further questions we'll keep in touch.

Comment 147

[u549602]

Verified as fixed on both the Nightly (49.0a1 2016-04-27) and Aurora (48.0a2 2016-04-28) builds on a Xiaomi mi i4 with Android 5.1.1 and Samsung Galaxy S7 Edge with Android 6.0.1