1014994 - create a synced tabs panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Attachment: Sync tabs on home screen.jpg

Synced tabs serve the same function as bookmarks, top sites. They are shortcuts to your interested destinations. They don't have favicons or thumbnails. Essentially they are links. 

Besides, sync tabs on home screen will make them easy to access by our users, especially with the new horizontal tab strip. 


This concept was implemented for Firefox Metro and worked quite well for Windows 8 users.

Comment 1

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Next step:
* Review with PM and Engineer
* Visual tweaks
* Implementation

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #1)
> Next step:
> * Review with PM and Engineer

I'm going to do most of the engineering work on this, aiming for the end of this cycle (the 33 cycle).

My first request is that yuan and antlam get together and make a future version of these mocks incorporating antlam's hidden device designs from Bug 899643.  This functionality won't be in my planned initial landing, but I'd like to plan ahead for that capability.  That is, I'd like a v1 (here) and a v2 (later, with hidden devices) to stage landing new functionality.

My second request is that I get mocks for tablet portrait and phone (just portrait).  This is a classic master/detail flow; on tablet landscape, we'll go split devices and tabs (like above), but we probably don't want that split on tablet portrait, and we definitely don't want that split on phone.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #1)
> Next step:
> * Review with PM and Engineer
> * Visual tweaks
> * Implementation

I have explored what a Remote Tabs panel looks like.  Implementation is long, but not really difficult.  An annoyance is that we have a home panel-of-sub panels, each sub-panel reflecting the underlying Firefox Account's state.

1) implement an AsyncTaskLoader returning an Android Account (Sync or Firefox);

2) implement the master panel (using loader above) that shows one of: Get Started call to action, list of Remote Tabs, needs verification/password/update and call to action button.

3) move existing Setup and Verification tabs tray panels to home panel architecture;

4) implement needs password and needs update home panels;

5) rewrite Remote Tabs list to use home panel list styling.

This gets us feature parity with the existing tabs tray implementation.  To get the tablet implementation, we should:

6) regularize TabsProvider's DBs, so that we can use the standard Android ExpandableListView with CursorTreeAdapter (Bug 1025128);

7) do the work to implement the split pane master/detail navigation in Yuan's mocks.

Strictly speaking, 6 is not necessary.  I think it's the right thing to do, but we can implement master/detail using our existing DBs.

Comment 4

[Nick Alexander :nalexander [he/him]]

lucasr: ni to you since you're co-ordinating some of this tablet work and for general work/plan overview.

Comment 5

[Lucas Rocha (:lucasr)]

The general view structure should be fairly similar to our current History panel on tablets. The main difference being that the list of 'sections' is dynamic (the list of devices) and the 'detail' fragment is the pretty much the same for all sections, it just gets different filters depending on the selected selection.

Comment 6

[Lucas Rocha (:lucasr)]

I assume the plan for the phone UI is to just show a flat list divided by headers for each device?

Comment 7

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

Nick, I will sit down with Yuan again this week to move this along. I've had to put this on the back burner a little because of some other bugs but I'll definitely try to pick this up, maybe even with the other hub bugs on my list. 

(In reply to Lucas Rocha (:lucasr) from comment #6)
> I assume the plan for the phone UI is to just show a flat list divided by
> headers for each device?

by "flat" do you mean not collapsible?

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Anthony Lam (:antlam) from comment #7)
> Nick, I will sit down with Yuan again this week to move this along. I've had
> to put this on the back burner a little because of some other bugs but I'll
> definitely try to pick this up, maybe even with the other hub bugs on my
> list. 
> 
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > I assume the plan for the phone UI is to just show a flat list divided by
> > headers for each device?
> 
> by "flat" do you mean not collapsible?

Not necessarily. I meant the same UI we currently have. But collapsible works for me :-)

Comment 9

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

Attachment: prev_mob_syncpanel_getstarted1.png

Attaching a WIP shot for what some of these initial screens would look like (mobile only ATM) as per my discussions with Nick.

Comment 10

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

Attachment: prev_mob_syncpanel_tabs1.png

Preview of the tabs list after a user is set up.

Comment 11

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

Attachment: prev_mob_syncpanel_confirm1.png

Preview for the "confirm your account" step

Comment 12

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

Attachment: prev_mob_syncpanel_update1.png

...the "update your credentials" step (if a user changes their pw elsewhere and needs to re-enter it for other devices)

Comment 13

[Lucas Rocha (:lucasr)]

Drive-by design feedback: Anthony, I feel the text in the informational screens ('Cannot connect', 'Confirm your account', etc) lack a clearer sense of hierarchy. Maybe the titles could be bold and/or bigger with roboto-light? Maybe something more like the empty screens on the existing panels. It would be nice if we could keep some consistency across panels here. Thoughts?

Comment 14

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

(In reply to Lucas Rocha (:lucasr) from comment #13)
> Drive-by design feedback: Anthony, I feel the text in the informational
> screens ('Cannot connect', 'Confirm your account', etc) lack a clearer sense
> of hierarchy. Maybe the titles could be bold and/or bigger with
> roboto-light? Maybe something more like the empty screens on the existing
> panels. It would be nice if we could keep some consistency across panels
> here. Thoughts?

Good point Lucas. I've played with this too but wanted to get something posted and I think I tried to keep it consistent to the current Sync screens. But I'm going to give that a try too I like Roboto Light. :)

Comment 15

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

Attachment: prev_mob_syncpanel_getstarted2.png

Made some adjustments as per Lucas's suggestions. Essentially the font for the title is now 20sp Roboto Light, and adjusted the padding a little bit. I also pinned the "emblem" to the bottom of the screen (30dp from the bottom).

I think this works better

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8451101 [details]
prev_mob_syncpanel_getstarted2.png

I had the same impression as lucasr (about sizes).  I still feel that having the heading being /lighter/ than the body text is backwards, but over-all I like it.  We can always iterate after landing.

Comment 17

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

Attachment: prev_mob_syncpanel_getstarted3.png

Revising the colors a bit and increased the CTA button. I think it was way too small before. I played with this on my Nexus 5 and the CTA button feels a lot better.

Comment 18

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

Attachment: prev_mob_syncpanel_update3.png

Update credentials screen..

Comment 19

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

Attachment: prev_mob_syncpanel_confirm3.png

Confirm account screen

Comment 20

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Register RemoteTabsPanel. r=lucasr

This is the boilerplate to add a new RemoteTabsPanel HomeFragment.  At
the moment, it merely displays a static view with a static string.

Open for discussion: panel title and index of panel in array of panels.

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Display static views in Remote Tabs panel when Account is missing or unhealthy.

There are three major issues with this code as written:

1) I'm waiting for movement on Bug 1055724.  I have a dummy (test.com)
link in place for confirmation email support right now.  We may not be
able to ship a real link in time.

2) I have hard-coded strings in place.  I'll want to verify copy before
landing.

3) I haven't tried to style this at all.  It's using the styles from the
existing Tabs Panel, which look acceptable, but don't behave correctly
in landscape mode.  antlam will get a chance to hammer on the visuals
before landing.

mcomella: you did the most work getting additional status panels into
the existing tabs tray, and you're aware of the state of the
AccountsLoader.  The SUMO team got on Bug 1055724 really quick, so we
can land with a better URL.  I'm looking for code review; we'll
iterate with antlam to get visuals and copy in place before (or
immediately after?) landing.

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Display list of Remote Tabs when possible.

mcomella: margaret: this could be reviewed by either of you.

I elected not to modify the existing group/child layouts because the
colour scheme is different and its not worth trying to smoothly land a
dual thing before ripping one out.

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Enable swipe-to-refresh Remote Tabs list.

margaret: mcomella: again, this could be either of y'all.

I wanted to allow for swipe-refreshing the Remote Tabs list no matter
what panel is being displayed, so that users who have an account in an
intermediate state can force a sync and thereby update the local
device's understanding of that state.  But it's tricky because the
swipe refresh container expects a single (scrollable) child view, and
we sometimes have a hierarchy of views.  This is simple and works now.

Comment 24

[:Margaret Leibovic]

Comment on attachment 8478389 [details] [diff] [review]
Part 1: Register RemoteTabsPanel. r=lucasr

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

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +78,5 @@
>          if (!HardwareUtils.isLowMemoryPlatform()) {
>              panelConfigs.add(createBuiltinPanelConfig(mContext, PanelType.READING_LIST));
>          }
>  
> +        panelConfigs.add(createBuiltinPanelConfig(mContext, PanelType.REMOTE_TABS));

You'll also need to add a migration to add this panel to stored customized configurations:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeConfigPrefsBackend.java#124

Luckily, most the leg-work is already done there, you'll just need to rev the version and add a new migration case :)

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +382,5 @@
>       previous location in the navigation, such as the previous folder -->
>  <!ENTITY home_move_up_to_filter "Up to &formatS;">
>  
> +<!ENTITY home_remote_tabs_title "Remote Tabs">
> +<!ENTITY home_remote_tabs_empty "Your remote tabs show up here.">

Maybe something like "Your tabs from other devices show up here"? I don't know what users think of when they hear "remote tabs".

Comment 25

[:Margaret Leibovic]

Comment on attachment 8478389 [details] [diff] [review]
Part 1: Register RemoteTabsPanel. r=lucasr

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

Oops, didn't notice that I was actually on the hook for the real review here (I saw lucasr's name in the commit message). Doing a more thorough look now...

Looks good to me, let's just get some UX feedback about ordering, as well as migration behavior (where to stick the new panel if the user has already customized their panels, probably the end would make the most sense).

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +62,5 @@
> +                                .detach(this)
> +                                .attach(this)
> +                                .commitAllowingStateLoss();
> +        }
> +    }

Don't you not need this now, given your recent refactoring changes?

::: mobile/android/base/resources/layout/home_remote_tabs_panel.xml
@@ +10,5 @@
> +
> +    <TextView android:layout_width="match_parent"
> +		  android:layout_height="wrap_content"
> +		  android:layout_gravity="center_vertical"
> +              android:text="@string/home_remote_tabs_empty" />

Oops, looks like some indentation issues here.

Comment 26

[:Margaret Leibovic]

Comment on attachment 8478402 [details] [diff] [review]
Part 3: Display list of Remote Tabs when possible.

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

r+ if you answer my questions :)

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ -93,5 @@
>          final View view;
>          if (convertView != null) {
>              view = convertView;
>          } else {
> -            final LayoutInflater inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);

I don't see this RemoteTabsExpandableListAdapter class in the tree. Where is this coming from?

Also, it could probably move into the home package with the work you're doing in this bug.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +73,5 @@
> +
> +    @Override
> +    public void onViewCreated(View view, Bundle savedInstanceState) {
> +        mList = (ExpandableListView) view.findViewById(R.id.list);
> +        mList.setTag(HomePager.LIST_TAG_REMOTE_TABS);

This reminds me that there are robocop tests you will likely need to update to account for this new panel. See this patch for hints: https://hg.mozilla.org/mozilla-central/rev/9801180cd3b7

@@ +75,5 @@
> +    public void onViewCreated(View view, Bundle savedInstanceState) {
> +        mList = (ExpandableListView) view.findViewById(R.id.list);
> +        mList.setTag(HomePager.LIST_TAG_REMOTE_TABS);
> +
> +        mList.setOnChildClickListener(new OnChildClickListener() {

How does this differ from OnItemClickListener? Is there a reason in particular we need to use this instead?

@@ +86,5 @@
> +                }
> +
> +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);
> +
> +                // This item is a TwoLinePageRow, so we allow switch-to-tab.

Did you test this to make sure that the switch-to-tab UI appears properly in the item?

@@ +126,5 @@
> +    private void updateUiFromClients(List<RemoteClient> clients) {
> +        if (clients != null && !clients.isEmpty()) {
> +            for (int i = 0; i < mList.getExpandableListAdapter().getGroupCount(); i++) {
> +                mList.expandGroup(i);
> +            }

Can you explain what this logic is for? Is there any way to just default the groups to be expanded?

::: mobile/android/base/home/RemoteTabsPanel.java
@@ -83,5 @@
> -                                .detach(this)
> -                                .attach(this)
> -                                .commitAllowingStateLoss();
> -        }
> -    }

Oh, I see my previous comment is addressed here :)

::: mobile/android/base/resources/layout/home_remote_tabs_group.xml
@@ +17,5 @@
> +        android:id="@+id/device_type"
> +        android:layout_width="@dimen/favicon_bg"
> +        android:layout_height="@dimen/favicon_bg"
> +        android:layout_marginLeft="10dip"
> +        android:layout_marginRight="10dip" />

This file looks suspiciously like the two_line_page_row layout. Is the only difference that you want to use a regular ImageView here instead of a FaviconView? Maybe a follow-up could be to create a more generic layout that allows us to share more of this.

@@ +20,5 @@
> +        android:layout_marginLeft="10dip"
> +        android:layout_marginRight="10dip" />
> +
> +    <LinearLayout
> +        android:layout_width="fill_parent"

fill_parent is deprecated, this should be match_parent (see bug 1027831).

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #26)
> Comment on attachment 8478402 [details] [diff] [review]
> Part 3: Display list of Remote Tabs when possible.
> 
> Review of attachment 8478402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if you answer my questions :)

Of course!

> ::: mobile/android/base/RemoteTabsExpandableListAdapter.java
> @@ -93,5 @@
> >          final View view;
> >          if (convertView != null) {
> >              view = convertView;
> >          } else {
> > -            final LayoutInflater inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
> 
> I don't see this RemoteTabsExpandableListAdapter class in the tree. Where is
> this coming from?
> 
> Also, it could probably move into the home package with the work you're
> doing in this bug.

I should have been more clear: I broke it out into dependant Bug 1057637, which I think has most of a review from mcomella.  It's shared by the tabs tray and the home panel for now, so it's in base.  I'd be okay moving it, but maybe when we remove the tabs tray section.

> ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
> @@ +73,5 @@
> > +
> > +    @Override
> > +    public void onViewCreated(View view, Bundle savedInstanceState) {
> > +        mList = (ExpandableListView) view.findViewById(R.id.list);
> > +        mList.setTag(HomePager.LIST_TAG_REMOTE_TABS);
> 
> This reminds me that there are robocop tests you will likely need to update
> to account for this new panel. See this patch for hints:
> https://hg.mozilla.org/mozilla-central/rev/9801180cd3b7
> 
> @@ +75,5 @@
> > +    public void onViewCreated(View view, Bundle savedInstanceState) {
> > +        mList = (ExpandableListView) view.findViewById(R.id.list);
> > +        mList.setTag(HomePager.LIST_TAG_REMOTE_TABS);
> > +
> > +        mList.setOnChildClickListener(new OnChildClickListener() {
> 
> How does this differ from OnItemClickListener? Is there a reason in
> particular we need to use this instead?

We may do so, if we choose, but it's slightly less idiomatic.  We'd have to handle children and groups in the same listener; and we'd have to unpack the list position into the tuple of (child, group) positions manually.  Again, if you feel strongly, we may do so.

> @@ +86,5 @@
> > +                }
> > +
> > +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);
> > +
> > +                // This item is a TwoLinePageRow, so we allow switch-to-tab.
> 
> Did you test this to make sure that the switch-to-tab UI appears properly in
> the item?

Actually, I didn't; and I don't think it does.  I will investigate, but I'd prefer not to block landing on this nicety.  I'd be happy to kill switch-to-tab and make it follow-up, if you prefer.

> @@ +126,5 @@
> > +    private void updateUiFromClients(List<RemoteClient> clients) {
> > +        if (clients != null && !clients.isEmpty()) {
> > +            for (int i = 0; i < mList.getExpandableListAdapter().getGroupCount(); i++) {
> > +                mList.expandGroup(i);
> > +            }
> 
> Can you explain what this logic is for? Is there any way to just default the
> groups to be expanded?

When the underlying tabs-and-clients cursor changes, we update the adapter client list.  Unfortunately, the adapter does not preserve stable tab IDs, so it's as if we deleted and added all the data again.  (I am not 100% on this, but I believe so.)  As part of that, we definitely lose the expanded/collapsed settings.  This expands everything.  I intend to preserve these settings in follow-up.

There is no one-shot method to expand, and I have seen only awful hacks (in getViewForGroup, etc) to expand by default.  The API is not rich.

> ::: mobile/android/base/home/RemoteTabsPanel.java
> @@ -83,5 @@
> > -                                .detach(this)
> > -                                .attach(this)
> > -                                .commitAllowingStateLoss();
> > -        }
> > -    }
> 
> Oh, I see my previous comment is addressed here :)

Yes; but this is technically in the wrong commit.  I did a great deal of refactoring to get things this smooth, but apparently I missed some things.

> ::: mobile/android/base/resources/layout/home_remote_tabs_group.xml
> @@ +17,5 @@
> > +        android:id="@+id/device_type"
> > +        android:layout_width="@dimen/favicon_bg"
> > +        android:layout_height="@dimen/favicon_bg"
> > +        android:layout_marginLeft="10dip"
> > +        android:layout_marginRight="10dip" />
> 
> This file looks suspiciously like the two_line_page_row layout. Is the only
> difference that you want to use a regular ImageView here instead of a
> FaviconView? Maybe a follow-up could be to create a more generic layout that
> allows us to share more of this.

It is essentially identical, with the change you suggest.  TwoLinePageRow is rather special purpose, so I didn't want to muddy the waters with a premature re-factor.  Follow-up is good for me.

> @@ +20,5 @@
> > +        android:layout_marginLeft="10dip"
> > +        android:layout_marginRight="10dip" />
> > +
> > +    <LinearLayout
> > +        android:layout_width="fill_parent"
> 
> fill_parent is deprecated, this should be match_parent (see bug 1027831).

Roger that.  Thanks for the review!

Comment 28

[:Margaret Leibovic]

Comment on attachment 8478406 [details] [diff] [review]
Part 4: Enable swipe-to-refresh Remote Tabs list.

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

Assuming the FirefoxAccounts APIs do what they say they do, this looks good to me.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +64,5 @@
>      // Callbacks used for the loader.
>      private CursorLoaderCallbacks mCursorLoaderCallbacks;
>  
> +    // Child refresh layout view.
> +    private GeckoSwipeRefreshLayout refreshLayout;

Nit: Use m prefix to be consistent with the rest of the file (or get rid of prefixes in the patch where you create this file).

@@ +212,5 @@
> +        public void onRefresh() {
> +            if (FirefoxAccounts.firefoxAccountsExist(getActivity())) {
> +                final Account account = FirefoxAccounts.getFirefoxAccount(getActivity());
> +                FirefoxAccounts.requestSync(account, FirefoxAccounts.FORCE, STAGES_TO_SYNC_ON_REFRESH, null);
> +            }

If no account exists, we should probably call setRefreshing(false) to turn off the animation.

But would this even be expected? If there isn't an account, we shouldn't be showing a list you can pull to refresh, right? So maybe it would be good to log an error here if that happens.

::: mobile/android/base/resources/layout/home_remote_tabs_list_panel.xml
@@ +24,5 @@
> +              style="@style/Widget.RemoteTabsListView"
> +              android:groupIndicator="@android:color/transparent"
> +              android:layout_width="fill_parent"
> +              android:layout_height="0dp"
> +              android:layout_weight="1"/>

I don't think layout_weight will do anything here, since GeckoSwipeRefreshLayout doesn't extend LinearLayout. You can probably just do height match_parent.

(Same comment as before about how you should use match_parent, not fill_parent.)

Comment 29

[:Margaret Leibovic]

Comment on attachment 8478391 [details] [diff] [review]
Part 2: Display static views in Remote Tabs panel when Account is missing or unhealthy.

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

I'll let mcomella handle the main review, but here on some comments on quick glance.

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +170,5 @@
> +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_verification);
> +        case NeedsPassword:
> +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_password);
> +        case NeedsUpgrade:
> +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_upgrade);

Instead of creating all these different layouts, I wonder if we could dynamically adjust a single layout depending on the case. It seems like you could use remote_tabs_needs_verification as a base, then update strings/hide views as necessary.

::: mobile/android/base/resources/layout/remote_tabs_needs_password.xml
@@ +19,5 @@
> +    <TextView
> +        style="@style/TabsPanelItem.TextAppearance"
> +        android:layout_width="match_parent"
> +        android:layout_height="wrap_content"
> +        android:text="You need to sign in to start syncing." />

Needs localization (there are a few places in here where you need to do that).

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #29)
> Comment on attachment 8478391 [details] [diff] [review]
> Part 2: Display static views in Remote Tabs panel when Account is missing or
> unhealthy.
> 
> Review of attachment 8478391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll let mcomella handle the main review, but here on some comments on quick
> glance.
> 
> ::: mobile/android/base/home/RemoteTabsPanel.java
> @@ +170,5 @@
> > +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_verification);
> > +        case NeedsPassword:
> > +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_password);
> > +        case NeedsUpgrade:
> > +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_upgrade);
> 
> Instead of creating all these different layouts, I wonder if we could
> dynamically adjust a single layout depending on the case. It seems like you
> could use remote_tabs_needs_verification as a base, then update strings/hide
> views as necessary.

That's true.  In general, such dynamic trickery seems like a bad idea, but I could be convinced otherwise.  We don't expect Account state to change frequently, so I'm not too worried about creating garbage views; even so, I think I'll cache all the different states, or make it possible to set the view of a StaticFragment.  mcomella, you implemented exactly this, but with different requirements due to layout changes on tablets and in portrait/landscape mode.  antlam and I have talked and I don't expect layout changes to be required on any devices.  Do you have an opinion?

> ::: mobile/android/base/resources/layout/remote_tabs_needs_password.xml
> @@ +19,5 @@
> > +    <TextView
> > +        style="@style/TabsPanelItem.TextAppearance"
> > +        android:layout_width="match_parent"
> > +        android:layout_height="wrap_content"
> > +        android:text="You need to sign in to start syncing." />
> 
> Needs localization (there are a few places in here where you need to do
> that).

Yep, I am aware and noted this in the review request.  I'm planning to go over it with antlam.

Comment 31

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

Comment on attachment 8478391 [details] [diff] [review]
Part 2: Display static views in Remote Tabs panel when Account is missing or unhealthy.

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

Please reflag me for review when you've answered my questions.

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +97,5 @@
> +        getChildFragmentManager()
> +            .beginTransaction()
> +            .addToBackStack(null)
> +            .replace(R.id.remote_tabs_container, subPanel)
> +            .commitAllowingStateLoss();

Why commitAllowingStateLoss? Because we never need to restore the state?

@@ +126,5 @@
> +        }
> +
> +        if (!FxAccountConstants.ACCOUNT_TYPE.equals(account.type)) {
> +            Log.wtf(LOGTAG, "Non Sync, non Firefox Android Account returned by AccountLoader; offering setup panel.");
> +            return Action.None;

Shouldn't this be null?

nit: (Assuming Action.None over null:) This message could be inaccurate if we ever handle `Action.None` differently from the setup panel - I'd prefer to to just say we're returning `Action.None`. We can return `displaying setup panel` when we handle the action.

@@ +175,5 @@
> +        default:
> +            // This should never happen, but we're confident we have a Firefox
> +            // Account at this point, so let's show the needs password screen.
> +            // That's our best hope of righting the ship.
> +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_password);

You might want to log here too.

::: mobile/android/base/home/RemoteTabsStaticFragment.java
@@ +35,5 @@
> + * To do so, it expects its containing <code>Activity</code> to implement
> + * <code>OnUrlOpenListener<code>; to suggest this invariant at compile time, we
> + * inherit from <code>HomeFragment</code>.
> + */
> +public class RemoteTabsStaticFragment extends HomeFragment implements OnClickListener {

What do you mean by StaticFragment? How is this static?

@@ +45,5 @@
> +
> +    protected int mResourceId;
> +
> +    // On URL open listener.
> +    protected OnUrlOpenListener mUrlOpenListener;

Doesn't this shadow HomeFragment's mUrlOpenListener, which is protected? Why not use that?

@@ +70,5 @@
> +        final Bundle args = getArguments();
> +        if (args != null) {
> +            mResourceId = args.getInt(RESOURCE_ID, DEFAULT_RESOURCE_ID);
> +        } else {
> +            mResourceId = DEFAULT_RESOURCE_ID;

Why use a default here? It seems likely to introduce bugs when we can just be explicit instead.

@@ +75,5 @@
> +        }
> +    }
> +
> +    @Override
> +    public void onAttach(Activity activity) {

onAttach and onDetach appear to be duplicated from HomeFragment - is there a reason for that?

@@ +125,5 @@
> +            // This Activity will redirect to the correct Activity as needed.
> +            final Intent intent = new Intent(getActivity(), FxAccountCreateAccountActivity.class);
> +            intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
> +            startActivity(intent);
> +            return;

Rather than using 'return', it might be more natural to use `else if`.

::: mobile/android/base/resources/layout/home_remote_tabs_panel.xml
@@ +8,5 @@
>                android:layout_height="match_parent"
>                android:orientation="vertical">
>  
> +    <FrameLayout android:id="@+id/remote_tabs_container"
> +                 android:layout_width="fill_parent"

nit: match_parent

::: mobile/android/base/resources/layout/remote_tabs_needs_verification.xml
@@ +32,5 @@
> +
> +    <TextView
> +        android:id="@+id/remote_tabs_needs_verification_help"
> +        style="@style/TabsPanelItem.TextAppearance.Linkified"
> +        android:layout_width="fill_parent"

nit: match_parent

Comment 32

[:Margaret Leibovic]

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

> > ::: mobile/android/base/RemoteTabsExpandableListAdapter.java
> > @@ -93,5 @@
> > >          final View view;
> > >          if (convertView != null) {
> > >              view = convertView;
> > >          } else {
> > > -            final LayoutInflater inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
> > 
> > I don't see this RemoteTabsExpandableListAdapter class in the tree. Where is
> > this coming from?
> > 
> > Also, it could probably move into the home package with the work you're
> > doing in this bug.
> 
> I should have been more clear: I broke it out into dependant Bug 1057637,
> which I think has most of a review from mcomella.  It's shared by the tabs
> tray and the home panel for now, so it's in base.  I'd be okay moving it,
> but maybe when we remove the tabs tray section.

Sounds good, no big rush.

> > ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
> > @@ +73,5 @@
> > > +
> > > +    @Override
> > > +    public void onViewCreated(View view, Bundle savedInstanceState) {
> > > +        mList = (ExpandableListView) view.findViewById(R.id.list);
> > > +        mList.setTag(HomePager.LIST_TAG_REMOTE_TABS);
> > 
> > This reminds me that there are robocop tests you will likely need to update
> > to account for this new panel. See this patch for hints:
> > https://hg.mozilla.org/mozilla-central/rev/9801180cd3b7
> > 
> > @@ +75,5 @@
> > > +    public void onViewCreated(View view, Bundle savedInstanceState) {
> > > +        mList = (ExpandableListView) view.findViewById(R.id.list);
> > > +        mList.setTag(HomePager.LIST_TAG_REMOTE_TABS);
> > > +
> > > +        mList.setOnChildClickListener(new OnChildClickListener() {
> > 
> > How does this differ from OnItemClickListener? Is there a reason in
> > particular we need to use this instead?
> 
> We may do so, if we choose, but it's slightly less idiomatic.  We'd have to
> handle children and groups in the same listener; and we'd have to unpack the
> list position into the tuple of (child, group) positions manually.  Again,
> if you feel strongly, we may do so.

I don't have a strong opinion, I was just curious for the reasoning behind this. It sounds like OnChildClickListener makes more sense, let's stick with this. I didn't think through the fact that OnItemClickListener would also fire for the group headers, which you don't want to deal with.

> > @@ +86,5 @@
> > > +                }
> > > +
> > > +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);
> > > +
> > > +                // This item is a TwoLinePageRow, so we allow switch-to-tab.
> > 
> > Did you test this to make sure that the switch-to-tab UI appears properly in
> > the item?
> 
> Actually, I didn't; and I don't think it does.  I will investigate, but I'd
> prefer not to block landing on this nicety.  I'd be happy to kill
> switch-to-tab and make it follow-up, if you prefer.

Yeah, this sounds like fine follow-up material. I just wanted to make sure we didn't land some unknown broken-ness.

> > @@ +126,5 @@
> > > +    private void updateUiFromClients(List<RemoteClient> clients) {
> > > +        if (clients != null && !clients.isEmpty()) {
> > > +            for (int i = 0; i < mList.getExpandableListAdapter().getGroupCount(); i++) {
> > > +                mList.expandGroup(i);
> > > +            }
> > 
> > Can you explain what this logic is for? Is there any way to just default the
> > groups to be expanded?
> 
> When the underlying tabs-and-clients cursor changes, we update the adapter
> client list.  Unfortunately, the adapter does not preserve stable tab IDs,
> so it's as if we deleted and added all the data again.  (I am not 100% on
> this, but I believe so.)  As part of that, we definitely lose the
> expanded/collapsed settings.  This expands everything.  I intend to preserve
> these settings in follow-up.
> 
> There is no one-shot method to expand, and I have seen only awful hacks (in
> getViewForGroup, etc) to expand by default.  The API is not rich.

Fair enough. Maybe this bit of code deserves a comment explaining why we need to do things this way.

> > ::: mobile/android/base/resources/layout/home_remote_tabs_group.xml
> > @@ +17,5 @@
> > > +        android:id="@+id/device_type"
> > > +        android:layout_width="@dimen/favicon_bg"
> > > +        android:layout_height="@dimen/favicon_bg"
> > > +        android:layout_marginLeft="10dip"
> > > +        android:layout_marginRight="10dip" />
> > 
> > This file looks suspiciously like the two_line_page_row layout. Is the only
> > difference that you want to use a regular ImageView here instead of a
> > FaviconView? Maybe a follow-up could be to create a more generic layout that
> > allows us to share more of this.
> 
> It is essentially identical, with the change you suggest.  TwoLinePageRow is
> rather special purpose, so I didn't want to muddy the waters with a
> premature re-factor.  Follow-up is good for me.

Sounds good. Maybe it's just all the APK-shrinking conversation that's making me want to avoid extra layout resources in the tree :)

Comment 33

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #24)
> Comment on attachment 8478389 [details] [diff] [review]
> Part 1: Register RemoteTabsPanel. r=lucasr
>
> Review of attachment 8478389 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +78,5 @@
> >          if (!HardwareUtils.isLowMemoryPlatform()) {
> >              panelConfigs.add(createBuiltinPanelConfig(mContext, PanelType.READING_LIST));
> >          }
> >
> > +        panelConfigs.add(createBuiltinPanelConfig(mContext, PanelType.REMOTE_TABS));
>
> You'll also need to add a migration to add this panel to stored customized
> configurations:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeConfigPrefsBackend.java#124
>
> Luckily, most the leg-work is already done there, you'll just need to rev
> the version and add a new migration case :)

Done, as an additional commit.

> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +382,5 @@
> >       previous location in the navigation, such as the previous folder -->
> >  <!ENTITY home_move_up_to_filter "Up to &formatS;">
> >
> > +<!ENTITY home_remote_tabs_title "Remote Tabs">
> > +<!ENTITY home_remote_tabs_empty "Your remote tabs show up here.">
>
> Maybe something like "Your tabs from other devices show up here"? I don't
> know what users think of when they hear "remote tabs".

I changed the title to "Synced Tabs" to square with yuan's mocks; and
took your copy.

(In reply to :Margaret Leibovic from comment #25)
> Comment on attachment 8478389 [details] [diff] [review]
> Part 1: Register RemoteTabsPanel. r=lucasr
>
> Review of attachment 8478389 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Oops, didn't notice that I was actually on the hook for the real review here
> (I saw lucasr's name in the commit message). Doing a more thorough look
> now...
>
> Looks good to me, let's just get some UX feedback about ordering, as well as
> migration behavior (where to stick the new panel if the user has already
> customized their panels, probably the end would make the most sense).

yuan's mocks have it last on tablet; I think we should do what we did
for Recent Tabs, which is move it to the front on phones and the end on
tablets.  Those are the least valuable positions, I think; we can talk
about moving it into a more valuable slot after landing.

> ::: mobile/android/base/home/RemoteTabsPanel.java
> @@ +62,5 @@
> > +                                .detach(this)
> > +                                .attach(this)
> > +                                .commitAllowingStateLoss();
> > +        }
> > +    }
>
> Don't you not need this now, given your recent refactoring changes?

Removed.

> ::: mobile/android/base/resources/layout/home_remote_tabs_panel.xml
> @@ +10,5 @@
> > +
> > +    <TextView android:layout_width="match_parent"
> > +		  android:layout_height="wrap_content"
> > +		  android:layout_gravity="center_vertical"
> > +              android:text="@string/home_remote_tabs_empty" />
>
> Oops, looks like some indentation issues here.

Thanks.  Eclipse formats XML with tabs, and I can't figure out how to
make it better :(

Comment 34

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Register RemoteTabsPanel. r=margaret

This is the boilerplate to add a new RemoteTabsPanel HomeFragment.  At
the moment, it merely displays a static view with a static string.

The panel title is "Synced Tabs", per yuan's design PDF; and the panel
appears at the front (far left) of the list on phones, and at the
back (far right) of the list on tablets.

Comment 35

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Add RemoteTabsPanel configuration migration. r=margaret

This factors out a small migration helper, since we want the same logic
for Remote Tabs as existed for Recent Tabs.

Comment 36

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Display static views in Remote Tabs panel when Account is missing or unhealthy. r=mcomella

mcomella: I cached all the sub-panels, 'cuz it unifies some things and
helps prevent flicker; and I will comment answering your questions in
a moment.  This includes strings and styling.  Screenshots tomorrow AM.

Comment 37

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Display list of Remote Tabs when possible. r=margaret

Comment 38

[Nick Alexander :nalexander [he/him]]

Attachment: Part 5: Enable swipe-to-refresh Remote Tabs list. r=margaret

Comment 39

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #31)
> Comment on attachment 8478391 [details] [diff] [review]
> Part 2: Display static views in Remote Tabs panel when Account is missing or
> unhealthy.
>
> Review of attachment 8478391 [details] [diff] [review]:
> -----------------------------------------------------------------
>>
> Please reflag me for review when you've answered my questions.
>
> ::: mobile/android/base/home/RemoteTabsPanel.java
> @@ +97,5 @@
> > +        getChildFragmentManager()
> > +            .beginTransaction()
> > +            .addToBackStack(null)
> > +            .replace(R.id.remote_tabs_container, subPanel)
> > +            .commitAllowingStateLoss();
>
> Why commitAllowingStateLoss? Because we never need to restore the state?

Yeah, this is manually switching the panel.  There's no concept of
"going back" here, merely changing the panel entirely.

> @@ +126,5 @@
> > +        }
> > +
> > +        if (!FxAccountConstants.ACCOUNT_TYPE.equals(account.type)) {
> > +            Log.wtf(LOGTAG, "Non Sync, non Firefox Android Account returned by AccountLoader; offering setup panel.");
> > +            return Action.None;
>
> Shouldn't this be null?

Yes!  Well spotted.

> nit: (Assuming Action.None over null:) This message could be inaccurate if
> we ever handle `Action.None` differently from the setup panel - I'd prefer
> to to just say we're returning `Action.None`. We can return `displaying
> setup panel` when we handle the action.

Updated the comment to say "returning null".

> @@ +175,5 @@
> > +        default:
> > +            // This should never happen, but we're confident we have a Firefox
> > +            // Account at this point, so let's show the needs password screen.
> > +            // That's our best hope of righting the ship.
> > +            return RemoteTabsStaticFragment.newInstance(R.layout.remote_tabs_needs_password);
>
> You might want to log here too.

Done.

> ::: mobile/android/base/home/RemoteTabsStaticFragment.java
> @@ +35,5 @@
> > + * To do so, it expects its containing <code>Activity</code> to implement
> > + * <code>OnUrlOpenListener<code>; to suggest this invariant at compile time, we
> > + * inherit from <code>HomeFragment</code>.
> > + */
> > +public class RemoteTabsStaticFragment extends HomeFragment implements OnClickListener {
>
> What do you mean by StaticFragment? How is this static?

The idea is that the underlying view is static, i.e., not driven by data
(in the way that the BookmarkListView, or the RemoteTabsListFragment,
are).

> @@ +45,5 @@
> > +
> > +    protected int mResourceId;
> > +
> > +    // On URL open listener.
> > +    protected OnUrlOpenListener mUrlOpenListener;
>
> Doesn't this shadow HomeFragment's mUrlOpenListener, which is protected? Why
> not use that?

Yep, refactor fail.  Corrected.

> @@ +70,5 @@
> > +        final Bundle args = getArguments();
> > +        if (args != null) {
> > +            mResourceId = args.getInt(RESOURCE_ID, DEFAULT_RESOURCE_ID);
> > +        } else {
> > +            mResourceId = DEFAULT_RESOURCE_ID;
>
> Why use a default here? It seems likely to introduce bugs when we can just
> be explicit instead.

This is tricky; it's present, but not explained, in the other panels.
When this panel gets killed, its state gets preserved.  When we scroll
back, Android re-creates it with the default constructor and arguments
passed in the arguments to onCreate.  Therefore, we have to be able to
handle these arguments; and that pretty much mandates a default.

> @@ +75,5 @@
> > +        }
> > +    }
> > +
> > +    @Override
> > +    public void onAttach(Activity activity) {
>
> onAttach and onDetach appear to be duplicated from HomeFragment - is there a
> reason for that?

Refactor fail.  Corrected.

> @@ +125,5 @@
> > +            // This Activity will redirect to the correct Activity as needed.
> > +            final Intent intent = new Intent(getActivity(), FxAccountCreateAccountActivity.class);
> > +            intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
> > +            startActivity(intent);
> > +            return;
>
> Rather than using 'return', it might be more natural to use `else if`.

Done.

> ::: mobile/android/base/resources/layout/home_remote_tabs_panel.xml
> @@ +8,5 @@
> >                android:layout_height="match_parent"
> >                android:orientation="vertical">
> >
> > +    <FrameLayout android:id="@+id/remote_tabs_container"
> > +                 android:layout_width="fill_parent"
>
> nit: match_parent
>
> ::: mobile/android/base/resources/layout/remote_tabs_needs_verification.xml
> @@ +32,5 @@
> > +
> > +    <TextView
> > +        android:id="@+id/remote_tabs_needs_verification_help"
> > +        style="@style/TabsPanelItem.TextAppearance.Linkified"
> > +        android:layout_width="fill_parent"
>
> nit: match_parent

Fixed both of these.

Comment 40

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483262 [details] [diff] [review]
Part 1: Register RemoteTabsPanel. r=margaret

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

Carrying forward margaret's r+.  Migration is in a separate patch.

Comment 41

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483268 [details] [diff] [review]
Part 5: Enable swipe-to-refresh Remote Tabs list. r=margaret

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

Carrying forward margaret's r+.

Comment 42

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=a5e7a9b48ef9
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a5e7a9b48ef9

Comment 43

[Nick Alexander :nalexander [he/him]]

Attachment: Synced.Tabs.01.Get.Started.png

Comment 44

[Nick Alexander :nalexander [he/him]]

Attachment: Synced.Tabs.02.Needs.Confirmation.png

Comment 45

[Nick Alexander :nalexander [he/him]]

Attachment: Synced.Tabs.03.Needs.Password.png

Comment 46

[Nick Alexander :nalexander [he/him]]

Attachment: Synced.Tabs.04.Needs.Upgrade.png

Comment 47

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #44)
> Created attachment 8483584 [details]
> Synced.Tabs.02.Needs.Confirmation.png

antlam: can I get feedback on these screenshots?  Pay particular attention to this copy and the copy for 03; I changed it.  Due to the panel life cycles, I don't want to say (or do!) anything like sending an email each time a user "sees" the panel.  Therefore, I just show the button.

Comment 48

[Nick Alexander :nalexander [he/him]]

Attachment: Part 6: Display device type icon. r=margaret

Comment 49

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

Comment on attachment 8483265 [details] [diff] [review]
Part 3: Display static views in Remote Tabs panel when Account is missing or unhealthy. r=mcomella

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

lgtm.

> The idea is that the underlying view is static, i.e., not driven by data
> (in the way that the BookmarkListView, or the RemoteTabsListFragment,
> are).

I don't think Fragments are so frequently data-driven that it's worth having the distinction in the name (particularly when it conflicts with a Java keyword) but up to you.

::: mobile/android/base/resources/values/styles.xml
@@ +834,5 @@
> +        <item name="android:gravity">center</item>
> +        <item name="android:layout_marginBottom">16dp</item>
> +	</style>
> +
> +	<style name="RemoteTabsPanelItem.TextAppearance">

nit: indentation.

@@ +851,5 @@
> +        <item name="android:focusable">true</item>
> +        <item name="android:textColor">#0092DB</item>
> +    </style>
> +
> +    <style name="RemoteTabsPanelItem.Button">

There's no padding on this button (like TabsPanelItem.Button) - intentional?

Comment 50

[:Margaret Leibovic]

Comment on attachment 8483263 [details] [diff] [review]
Part 2: Add RemoteTabsPanel configuration migration. r=margaret

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

If I'm reading this correctly, it will replace the current first panel with the new panel in the add-to-front case, rather than add it to the front and shift the rest of the panels down the array.

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +140,5 @@
> +        jsonPanelConfig.put(PanelConfig.JSON_KEY_DISABLED,
> +                                 allPanelsAreDisabled(jsonPanels));
> +
> +        final boolean isPhone = !HardwareUtils.isTablet();
> +        final boolean isTablet = !isPhone;

Nit: It feels like it would be more intuitive to reverse these.

final boolean isTablet = HardwareUtils.isTablet();
final boolean isPhone = !isTablet;

@@ +145,5 @@
> +
> +        // Maybe add the new panel to the front of the array.
> +        if ((isPhone && positionOnPhones == Position.FRONT) ||
> +            (isTablet && positionOnTablets == Position.FRONT)) {
> +            jsonPanels.put(0, jsonPanelConfig);

This will replace the value that's currently at the front, which isn't what we want. There's some conversation about it in here: https://bugzilla.mozilla.org/show_bug.cgi?id=1004850#c44

That's why I had to add that logic to create a new array of panels.

@@ +148,5 @@
> +            (isTablet && positionOnTablets == Position.FRONT)) {
> +            jsonPanels.put(0, jsonPanelConfig);
> +        }
> +
> +        // Maybe add the new panel to the front of the array.

Copy/pasta comment error.

Comment 51

[:Margaret Leibovic]

Comment on attachment 8483267 [details] [diff] [review]
Part 4: Display list of Remote Tabs when possible. r=margaret

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

Looks good.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +87,5 @@
> +
> +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);
> +
> +                // This item is a TwoLinePageRow, so we allow switch-to-tab.
> +                mUrlOpenListener.onUrlOpen(tab.url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));

In the comments, I thought you said switch-to-tab doesn't work. Will this just work once bug 977164 is fixed?

::: mobile/android/base/resources/layout/home_remote_tabs_list_panel.xml
@@ +14,5 @@
> +              android:layout_height="match_parent"/>
> +
> +    <TextView android:id="@+id/title"
> +              style="@style/Widget.Home.HistoryPanelTitle"
> +              android:visibility="gone"/>

Is this view used? Or just copy/pasted from home_history_list.xml? I don't even think this is used in our history views anymore, we can probably get rid of it there as well.

::: mobile/android/base/resources/values/styles.xml
@@ +595,5 @@
> +        <item name="android:background">#fff5f7f9</item>
> +    </style>
> +
> +    <style name="Widget.RemoteTabsListView" parent="Widget.HomeListView">
> +        <item name="android:childDivider">#E7ECF0</item>

I assume this is taken from the Widget.HomeListView style. It might be worth splitting this color out into a color value to make sure those styles don't get out of sync. Could be a good mentor bug!

Comment 52

[:Margaret Leibovic]

Comment on attachment 8483836 [details] [diff] [review]
Part 6: Display device type icon. r=margaret

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

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +112,5 @@
> +        // Therefore, we must handle null.
> +        final ImageView deviceTypeView = (ImageView) view.findViewById(R.id.device_type);
> +        if (deviceTypeView != null) {
> +            if ("desktop".equals(client.deviceType)) {
> +                deviceTypeView.setBackgroundResource(R.drawable.sync_desktop);

I only see mdpi versions of these icons in the tree. Are we going to need better icons for high-res screens? We can do this in a separate bug if it's necessary.

Comment 53

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Add RemoteTabsPanel configuration migration. r=margaret

This factors out a small migration helper, since we want the same logic
for Remote Tabs as existed for Recent Tabs.

Comment 54

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #50)
> Comment on attachment 8483263 [details] [diff] [review]
> Part 2: Add RemoteTabsPanel configuration migration. r=margaret
>
> Review of attachment 8483263 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> If I'm reading this correctly, it will replace the current first panel with
> the new panel in the add-to-front case, rather than add it to the front and
> shift the rest of the panels down the array.

I handled this by adding a reverse loop through the existing panels.
It's inefficient, but migration is rare and the exposed JSONArray API is
not rich.

In addition, I wrote a JUnit 3 test for this, which uncovered Bug
1063028.  If I can land that sometime soon, I'll land the test, which
passes locally.

> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +140,5 @@
> > +        jsonPanelConfig.put(PanelConfig.JSON_KEY_DISABLED,
> > +                                 allPanelsAreDisabled(jsonPanels));
> > +
> > +        final boolean isPhone = !HardwareUtils.isTablet();
> > +        final boolean isTablet = !isPhone;
>
> Nit: It feels like it would be more intuitive to reverse these.
>
> final boolean isTablet = HardwareUtils.isTablet();
> final boolean isPhone = !isTablet;

Done.

> @@ +145,5 @@
> > +
> > +        // Maybe add the new panel to the front of the array.
> > +        if ((isPhone && positionOnPhones == Position.FRONT) ||
> > +            (isTablet && positionOnTablets == Position.FRONT)) {
> > +            jsonPanels.put(0, jsonPanelConfig);
>
> This will replace the value that's currently at the front, which isn't what
> we want. There's some conversation about it in here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1004850#c44
>
> That's why I had to add that logic to create a new array of panels.

Commented above.

> @@ +148,5 @@
> > +            (isTablet && positionOnTablets == Position.FRONT)) {
> > +            jsonPanels.put(0, jsonPanelConfig);
> > +        }
> > +
> > +        // Maybe add the new panel to the front of the array.
>
> Copy/pasta comment error.

Fixed.

Comment 55

[Nick Alexander :nalexander [he/him]]

Attachment: Post: Add TestHomeConfigPrefsBackend browser JUnit 3 test. r=margaret

margaret: This won't yet apply, because it depends on Bug 1063028; but
it shows what I've tested locally.

Earlier, I also actually rolled back and verified I got a Remote Tabs
panel added to a modified configuration on my device; but I must not
have noticed the existing panel that I would have over-written.

Comment 56

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

(In reply to Nick Alexander :nalexander from comment #47)
> (In reply to Nick Alexander :nalexander from comment #44)
> > Created attachment 8483584 [details]
> > Synced.Tabs.02.Needs.Confirmation.png
> 
> antlam: can I get feedback on these screenshots?  Pay particular attention
> to this copy and the copy for 03; I changed it.  Due to the panel life
> cycles, I don't want to say (or do!) anything like sending an email each
> time a user "sees" the panel.  Therefore, I just show the button.

Overall:

Could I get a check on what the font sizes for the header and body copy are? :) 
They should be 20 sp and 16 sp respectively. The button label should be 20sp as well I believe on Regular weight. 

The button size itself also seems like it's a bit small. It should be 60 dp high and I think set at 300 dp wide.

02- Confirm your account: 
What do you think about "Please verify your Firefox Account to start syncing"? I think it might be more direct and uses less words.

03- Unable to connect:
Looks good! 

04- 
I wonder.. should it be "upgrade" or "update"?

Comment 57

[:Margaret Leibovic]

Comment on attachment 8484403 [details] [diff] [review]
Part 2: Add RemoteTabsPanel configuration migration. r=margaret

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

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +149,5 @@
> +            // This is an inefficient way to stretch [a, b, c] to [a, a, b, c].
> +            for (int i = jsonPanels.length(); i >= 1; i--) {
> +                jsonPanels.put(i, jsonPanels.get(i - 1));
> +            }
> +            // And this inserts [d, a, b, c].

Nice illustrative comments.

Comment 58

[:Margaret Leibovic]

Comment on attachment 8484408 [details] [diff] [review]
Post: Add TestHomeConfigPrefsBackend browser JUnit 3 test. r=margaret

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

This seems like a nice simple unit test. It makes me think that we should try writing some unit tests for all sorts of stuff in HomeConfig. I wonder if lucasr ever tried something like that.

Comment 59

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #51)
> Comment on attachment 8483267 [details] [diff] [review]
> Part 4: Display list of Remote Tabs when possible. r=margaret
> 
> Review of attachment 8483267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
> @@ +87,5 @@
> > +
> > +                Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.LIST_ITEM);
> > +
> > +                // This item is a TwoLinePageRow, so we allow switch-to-tab.
> > +                mUrlOpenListener.onUrlOpen(tab.url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
> 
> In the comments, I thought you said switch-to-tab doesn't work. Will this
> just work once bug 977164 is fixed?

That is correct: post Bug 977164, the UI will reflect what will happen when you tap.  Pre Bug 977164, you'll switch to tab with no warning that is the case.

> ::: mobile/android/base/resources/layout/home_remote_tabs_list_panel.xml
> @@ +14,5 @@
> > +              android:layout_height="match_parent"/>
> > +
> > +    <TextView android:id="@+id/title"
> > +              style="@style/Widget.Home.HistoryPanelTitle"
> > +              android:visibility="gone"/>
> 
> Is this view used? Or just copy/pasted from home_history_list.xml? I don't
> even think this is used in our history views anymore, we can probably get
> rid of it there as well.

It is both used and copy/pasted.  The next commit removes it.

> ::: mobile/android/base/resources/values/styles.xml
> @@ +595,5 @@
> > +        <item name="android:background">#fff5f7f9</item>
> > +    </style>
> > +
> > +    <style name="Widget.RemoteTabsListView" parent="Widget.HomeListView">
> > +        <item name="android:childDivider">#E7ECF0</item>
> 
> I assume this is taken from the Widget.HomeListView style. It might be worth
> splitting this color out into a color value to make sure those styles don't
> get out of sync. Could be a good mentor bug!

It was copied, and it is a good first bug.  Will file.

Comment 60

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #49)
> Comment on attachment 8483265 [details] [diff] [review]
> Part 3: Display static views in Remote Tabs panel when Account is missing or
> unhealthy. r=mcomella
> 
> Review of attachment 8483265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm.
> 
> > The idea is that the underlying view is static, i.e., not driven by data
> > (in the way that the BookmarkListView, or the RemoteTabsListFragment,
> > are).
> 
> I don't think Fragments are so frequently data-driven that it's worth having
> the distinction in the name (particularly when it conflicts with a Java
> keyword) but up to you.

I'm going to keep it because I can't stomach rebasing right now, but I understand the confusion with Java's keyword.  Next time.

> ::: mobile/android/base/resources/values/styles.xml
> @@ +834,5 @@
> > +        <item name="android:gravity">center</item>
> > +        <item name="android:layout_marginBottom">16dp</item>
> > +	</style>
> > +
> > +	<style name="RemoteTabsPanelItem.TextAppearance">
> 
> nit: indentation.

Fixed.

> @@ +851,5 @@
> > +        <item name="android:focusable">true</item>
> > +        <item name="android:textColor">#0092DB</item>
> > +    </style>
> > +
> > +    <style name="RemoteTabsPanelItem.Button">
> 
> There's no padding on this button (like TabsPanelItem.Button) - intentional?

I could go either way, here.  I was tearing out most of the hard-coded margins and padding and trying to uniformize.  I'll see what it looks like with the padding back in.

Comment 61

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #56)
> (In reply to Nick Alexander :nalexander from comment #47)
> > (In reply to Nick Alexander :nalexander from comment #44)
> > > Created attachment 8483584 [details]
> > > Synced.Tabs.02.Needs.Confirmation.png
> > 
> > antlam: can I get feedback on these screenshots?  Pay particular attention
> > to this copy and the copy for 03; I changed it.  Due to the panel life
> > cycles, I don't want to say (or do!) anything like sending an email each
> > time a user "sees" the panel.  Therefore, I just show the button.
> 
> Overall:
> 
> Could I get a check on what the font sizes for the header and body copy are?
> :) 
> They should be 20 sp and 16 sp respectively. The button label should be 20sp
> as well I believe on Regular weight. 

Header was always 20sp; body was always 16sp.  Button was 18/bold; changed to 20/regular.

> The button size itself also seems like it's a bit small. It should be 60 dp
> high and I think set at 300 dp wide.

Specifying heights and widths is not the right way to go here.  I'm setting the margins, padding, etc; we'll make the button larger that way.

We'll float the entire frame in the center on tablets.

> 02- Confirm your account: 
> What do you think about "Please verify your Firefox Account to start
> syncing"? I think it might be more direct and uses less words.

Done.

> 03- Unable to connect:
> Looks good! 
> 
> 04- 
> I wonder.. should it be "upgrade" or "update"?

We use the existing copy elsewhere in FxA, so I'd like to land with it and reconsider this decision later, if you so choose.

Thanks for your feedback!  We can iterate on this in the next days/week.

Comment 62

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/9fd663519e7e
https://hg.mozilla.org/integration/fx-team/rev/46d4058a0b32
https://hg.mozilla.org/integration/fx-team/rev/beaa7b7dad99
https://hg.mozilla.org/integration/fx-team/rev/06839323ed2f
https://hg.mozilla.org/integration/fx-team/rev/11d61fe8011c
https://hg.mozilla.org/integration/fx-team/rev/287bd4d03bbc

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fd663519e7e
https://hg.mozilla.org/mozilla-central/rev/46d4058a0b32
https://hg.mozilla.org/mozilla-central/rev/beaa7b7dad99
https://hg.mozilla.org/mozilla-central/rev/06839323ed2f
https://hg.mozilla.org/mozilla-central/rev/11d61fe8011c
https://hg.mozilla.org/mozilla-central/rev/287bd4d03bbc

Comment 64

[Nick Alexander :nalexander [he/him]]

If yuan (or antlam, or ...) wants changes, we'll start a new ticket.

Comment 65

[Aaron Train [:aaronmt]]

We'll want to update our MozTrap tests to support the changes here.

Comment 66

[miralobontiu]

Hi Aaron,

Is the qe-verify+ flag still valid?

Thanks!

Comment 67

[Kevin Brosnan [Ex-Mozilla]]

This specific UI does not exist any more. It was rolled into the history panel.