Brought up in our meeting, to help increase the visibility of Firefox Accounts, we could show off some of the benefits to having an account (as it pertains to Reading List) on the empty state. Designs to follow.

Moving this to block bug 1123101, since we'll need the reading list service to work before this makes sense :)

antlam: we should generalize what we already do for the Remote Tabs home panel. Can you produce a unified design? I reckon we can use the same code, possibly with some visual tweaks for the relevant panel.

I think this is most of Bug 1123105, so let's add another layer of bug dependency indirection :D

(In reply to Nick Alexander :nalexander from comment #2) > antlam: we should generalize what we already do for the Remote Tabs home > panel. Can you produce a unified design? I reckon we can use the same > code, possibly with some visual tweaks for the relevant panel. Good point. exactly what I was thinking too.

vivek is interested in generalizing the existing code and adding support for the reading list \o/ The first step is to extract an abstract "WithAccountHomePanel" class from the existing code at [1], and to add configuration where appropriate that RemoteTabsPanel can customize. We probably want to generalize the static panel configurations as well to be less "Remote Tabs" specific -- they're pretty general already. [1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsPanel.java

A new patch on top of patch series in bug 1063742 * WithAccountHomePanel abstract super class extracted from RemoteTabsPanel

Refactored makeFragmentForAction() in Part1 patch to be abstract

Second Part, Read List panel is now a parent fragment of type WithAccountPanel holding the newly created ReadingListFragment. When there are no reading list, sync account related information is shown in Reading list panel.

Comment on attachment 8556891 [details] [diff] [review] 1122710_part1.patch Review of attachment 8556891 [details] [diff] [review]: ----------------------------------------------------------------- This seems basically right. I'm not convinced that makePanelFragment is the right API for this. How do you feel about pushing makePanelFragment into the abstract base class and extracting the different choices into abstract functions? The reason to do that is so that implementers can't screw up the case statement. When we add a new state/action required, every implementer will have to update. ::: mobile/android/base/home/WithAccountHomePanel.java @@ +26,5 @@ > +/** > + * A abstract <code>HomeFragment</code> that, depending on the state of accounts on the > + * device: > + * <ul> > + * <li>displays remote tabs / reading list from other devices;</li> Update this comment. @@ +69,5 @@ > + public void load() { > + getLoaderManager().initLoader(LOADER_ID_ACCOUNT, null, mAccountLoaderCallbacks); > + } > + > + protected abstract int getPanelContainerResource(); Javadoc comments explaining all abstract methods and how they should be used. Calling this a resource rather than an ID suggests a layout, which is mis-leading. @@ +71,5 @@ > + } > + > + protected abstract int getPanelContainerResource(); > + > + protected abstract Fragment makeFragmentForAction(Action action); Maybe this needs to take <Orientation, Action> or similar? Or something more general? @@ +161,5 @@ > + Fragment fragment = mFragmentCache.get(actionNeeded); > + > + // On a tablet devices with accounts authenticated, create a new fragment based on the current orientation. > + // The cached fragment in the above case may not be the valid fragment for the current orientation. > + if (fragment == null || (HardwareUtils.isTablet() && actionNeeded == Action.None)) { This isn't right. Perhaps we need to include the orientation in the cache? So we key by <Orientation, ActionNeeded> rather than just <ActionNeeded>?

Comment on attachment 8556894 [details] [diff] [review] 1122710_part1.patch Review of attachment 8556894 [details] [diff] [review]: ----------------------------------------------------------------- Did this obsolete the other patch? I think the comments still stand, so I'm not going to try to figure out the differences. If anything major changed, highlight it for me in the follow-up review. Thanks!

Awesome! Do we have a screenshot of the progress? I've cleaned up about:passwords empty state in bug 1101746 with Ally. This was a merge of the designs from the current empty state panels (particularly the Synced tabs panel) and the about:pages styling. In light of consistency, I just wanted to point that bug out.

A patch with review comments addressed. makeFragmentForAction handled in Base class with respective fragment for each action created in sub class. Fragment cache key changed to Pair<Boolean, Action> instead of Action where the bool represents if there is a separate landscape layout.

ReadingListFragment now shows sync account related fragments when empty.

@Antlam: An Apk build with above changes can be found at https://www.dropbox.com/s/io365f9072xd3ss/fennec-38.0a1.en-US.android-arm.apk?dl=0

Comment on attachment 8558038 [details] [diff] [review] 1122710_part1.patch Review of attachment 8558038 [details] [diff] [review]: ----------------------------------------------------------------- This looks basically OK, but we'll need to hold for some UX input. ::: mobile/android/base/home/WithAccountHomePanel.java @@ +199,5 @@ > + } > + return mFallbackFragment; > + } > + > + final Pair<Boolean, Action> pair = Pair.create(supportLandscapeLayout(actionNeeded), actionNeeded); This is a little different than what you were trying to do for the split pane flickering. Elaborate on why?

Comment on attachment 8558040 [details] [diff] [review] 1122710_part2.patch Review of attachment 8558040 [details] [diff] [review]: ----------------------------------------------------------------- Again, this looks basically OK. We're not quite ready to push this out yet but the code is looking reasonable. I didn't look too closely at the fragment; I assume it was mostly just moving code from the existing panel to a separate fragment. As written, the two patches are incompatible, because patch 1 assumes that the fragment exists, but it's not added until patch 2. It's not a show-stopper but it is bad form. ::: mobile/android/base/resources/layout/home_reading_list_panel.xml @@ +9,5 @@ > android:orientation="vertical"> > > + <FrameLayout android:id="@+id/reading_list_container" > + android:layout_width="match_parent" > + android:layout_height="0dp" Why not just match_parent/match_parent? @@ +16,2 @@ > </LinearLayout> > + nit: kill new line.

With Reading List getting folded into the bookmarks panel, we're not going to do this.