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.

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

(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.

(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.

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

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.

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

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?

(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 :-)

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

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

Preview for the "confirm your account" step

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

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?

(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. :)

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 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.

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.

Update credentials screen..

Confirm account screen

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.

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.

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.

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 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 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 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).

(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 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 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).

(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 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

(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 :)

(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 :(

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.

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

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.

(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 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 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+.

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

(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 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 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 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 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.

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

(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.

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.

(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 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 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.

(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.

(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.

(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.

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

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

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

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

Hi Aaron, Is the qe-verify+ flag still valid? Thanks!

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