Using a ScrollView in about:home has proved to be a resource-hogging, over-drawing, more-views-holding UI. The ViewPager with listview will reduce the content shown, and improve the performance too. Some of the mockups for about:home redesign are: http://cl.ly/image/1I1b3Q1A3O45/o http://cl.ly/image/1535093t2b2Q/o http://cl.ly/image/1S1C3c2a2N1k/o

I really think we should use a PagerTitleStrip here rather than normal tabs, to allow for future expansion/extensions on about:home. Unfortunately, the Android PagerTitleStrip sucks, so we'd have to write something custom.

There's Jake Wharton's ViewPagerIndicator: https://github.com/JakeWharton/Android-ViewPagerIndicator

This removes the images from the XML files. They aren't removed from the build yet. I need to check if thats the same image on gecko side too, and then remove it. That will be a separate patch.

Move about:home specific views to widget/ directory.

That biggg AboutHomeContent is split into smaller chunks. This can now be fed to a PagerAdapter. Yaay!

(In reply to Sriram Ramasubramanian [:sriram] from comment #5) > Created attachment 727396 [details] [diff] [review] > Part 3: Split AboutHomeContent > > That biggg AboutHomeContent is split into smaller chunks. This can now be > fed to a PagerAdapter. Yaay! This is fine as a short-term refactoring but, ideally, we should make each page of about:home a fragment so that we can have tighter control over their life-cycle. For instance, you probably want to only keep a max number of pages in memory i.e. using ViewPager's setOffscreenPageLimit(). You can do that view custom views but the code will tend to become much more complex as you'll be doing the life-cycle control "by hand" while Android provides all that for us if we use fragments. e.g. have a look the FragmentPagerAdapter and FragmentStatePagerAdapter.

(In reply to Lucas Rocha (:lucasr) from comment #6) > (In reply to Sriram Ramasubramanian [:sriram] from comment #5) > > Created attachment 727396 [details] [diff] [review] > > Part 3: Split AboutHomeContent > > > > That biggg AboutHomeContent is split into smaller chunks. This can now be > > fed to a PagerAdapter. Yaay! > > This is fine as a short-term refactoring but, ideally, we should make each > page of about:home a fragment so that we can have tighter control over their > life-cycle. > > For instance, you probably want to only keep a max number of pages in memory > i.e. using ViewPager's setOffscreenPageLimit(). You can do that view custom > views but the code will tend to become much more complex as you'll be doing > the life-cycle control "by hand" while Android provides all that for us if > we use fragments. e.g. have a look the FragmentPagerAdapter and > FragmentStatePagerAdapter. Definitely we are going to use fragments and Brian is going to work on it. This is just a start to split them into smaller manageable views.

(In reply to Lucas Rocha (:lucasr) from comment #6) > For instance, you probably want to only keep a max number of pages in memory > i.e. using ViewPager's setOffscreenPageLimit(). You can do that view custom > views but the code will tend to become much more complex as you'll be doing > the life-cycle control "by hand" while Android provides all that for us if > we use fragments. e.g. have a look the FragmentPagerAdapter and > FragmentStatePagerAdapter. I don't care if we use fragments, but this seems a bit misleading. The ViewPager itself handles the lifecycle here and calls instantiateItem/destroyItem on the adapter when it wants/is done with something. We can do what we want at that point to create/destroy things. Its not that complex. i.e. This doesn't really give us "tighter control over their lifecycle". It just forces us to implement a fragment interface rather than a generic one. I worry a bit about FragmentPagerAdapter because it puts even more code out of our control, but its pretty small, and we can always steal it if we need to.

This moves the AboutHomeContent to widget/ folder.

This splits AboutHomeContent to smaller views.

Ah, so clean! Removes the image.

(In reply to Wesley Johnston (:wesj) from comment #8) > (In reply to Lucas Rocha (:lucasr) from comment #6) > > For instance, you probably want to only keep a max number of pages in memory > > i.e. using ViewPager's setOffscreenPageLimit(). You can do that view custom > > views but the code will tend to become much more complex as you'll be doing > > the life-cycle control "by hand" while Android provides all that for us if > > we use fragments. e.g. have a look the FragmentPagerAdapter and > > FragmentStatePagerAdapter. > > I don't care if we use fragments, but this seems a bit misleading. The > ViewPager itself handles the lifecycle here and calls > instantiateItem/destroyItem on the adapter when it wants/is done with > something. We can do what we want at that point to create/destroy things. > Its not that complex. This is an oversimplification. There's a whole lot more complexity in handling the life cycle of each page in a ViewPager. e.g. saving the state of a page to be restored once the app is resumed, responding to the state of the parent activity accordingly, and much more. It's definitely not just about instantiating and destroying each page. > i.e. This doesn't really give us "tighter control over their lifecycle". It > just forces us to implement a fragment interface rather than a generic one. > I worry a bit about FragmentPagerAdapter because it puts even more code out > of our control, but its pretty small, and we can always steal it if we need > to. As I said, life cycle control is not just about instantiating and destroying views in the ViewPager. For more information, have a look at: http://developer.android.com/guide/components/fragments.html#Lifecycle Last but not least, implementing each page as fragments will allow us to recombine each component either inside a ViewPager or anywhere else in the UI — if we need to do so. It's much more future-proof than implementing custom interfaces that are directly tied to how PagerAdapter works. Just to restate (from bug 817586): I'd really like us to rely on built-in platform features/API whenever possible instead of coming up with fresh/unproven custom code to do the exact same thing.

Comment on attachment 729258 [details] [diff] [review] Patch 1: Move views to widget Review of attachment 729258 [details] [diff] [review]: ----------------------------------------------------------------- Some questions before I can give r+: ::: mobile/android/base/ActivityHandlerHelper.java @@ +27,5 @@ > import java.util.List; > import java.util.concurrent.SynchronousQueue; > import java.util.concurrent.TimeUnit; > > +public class ActivityHandlerHelper { Why are you making this class and its members public? It's only used in GeckoAppShell and has a pretty localized purpose, so I don't see the benefit of exposing it. ::: mobile/android/base/GeckoAppShell.java @@ +149,5 @@ > private static Sensor gLightSensor = null; > > private static boolean mLocationHighAccuracy = false; > > + public static ActivityHandlerHelper sActivityHelper = new ActivityHandlerHelper(); Why are you making this public? Seems like it should be made private. ::: mobile/android/base/resources/layout/gecko_app.xml @@ +26,5 @@ > android:layout_above="@+id/find_in_page"> > > <include layout="@layout/shared_ui_components"/> > > + <Gecko.AboutHomeContent android:id="@+id/abouthome_content" I don't understand what's happening here -- what is Gecko in this context?

(In reply to Brian Nicholson (:bnicholson) from comment #13) > Comment on attachment 729258 [details] [diff] [review] > Patch 1: Move views to widget > > Review of attachment 729258 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some questions before I can give r+: > > ::: mobile/android/base/ActivityHandlerHelper.java > @@ +27,5 @@ > > import java.util.List; > > import java.util.concurrent.SynchronousQueue; > > import java.util.concurrent.TimeUnit; > > > > +public class ActivityHandlerHelper { > > Why are you making this class and its members public? It's only used in > GeckoAppShell and has a pretty localized purpose, so I don't see the benefit > of exposing it. > > ::: mobile/android/base/GeckoAppShell.java > @@ +149,5 @@ > > private static Sensor gLightSensor = null; > > > > private static boolean mLocationHighAccuracy = false; > > > > + public static ActivityHandlerHelper sActivityHelper = new ActivityHandlerHelper(); > > Why are you making this public? Seems like it should be made private. > It's used in AboutHomeContent. It initially had a package access. Now that AboutHomeContent is moved to widget/, raising its level from package from public. > ::: mobile/android/base/resources/layout/gecko_app.xml > @@ +26,5 @@ > > android:layout_above="@+id/find_in_page"> > > > > <include layout="@layout/shared_ui_components"/> > > > > + <Gecko.AboutHomeContent android:id="@+id/abouthome_content" > > I don't understand what's happening here -- what is Gecko in this context? That's my small trick with your favorite GeckoViewsFactory. :D "Gecko." doesn't have any context. We will short-circuit Android and give it the required view, if the View starts with a "Gecko." signature. This is better than writing "org.mozilla.gecko.widget."

Comment on attachment 729258 [details] [diff] [review] Patch 1: Move views to widget (In reply to Sriram Ramasubramanian [:sriram] from comment #14) > > It's used in AboutHomeContent. It initially had a package access. Now that > AboutHomeContent is moved to widget/, raising its level from package from > public. Oh...somehow I missed uses of the ActivityHandlerHelper outside of GeckoAppShell when I grepped for it. > That's my small trick with your favorite GeckoViewsFactory. :D "Gecko." > doesn't have any context. We will short-circuit Android and give it the > required view, if the View starts with a "Gecko." signature. This is better > than writing "org.mozilla.gecko.widget." Why is it better? It's not at all clear what it means unless you're familiar with GeckoViewsFactory as it doesn't follow Android convention. It seems like a bad idea to couple the names of views with the factory responsible for processing them. And, to make matters worse, we've already seen that this breaks compatibility with any processing tools such as Eclipse that don't understand the "Gecko." prefix. If we really need this factory code, is there any reason we can't simply use attributes to designate whether something needs processing (such as gecko:use_gecko_factory="true")? Also, what determines whether a view should be processed by the Gecko factory versus the standard Android factory? I don't understand why you have "Gecko.AboutHomeContent", when right below it you use "org.mozilla.gecko.FindInPageBar".

> android:layout_above="@+id/find_in_page"> > > <include layout="@layout/shared_ui_components"/> > > + <Gecko.AboutHomeContent android:id="@+id/abouthome_content" And wouldn't this need be Gecko.widget.AboutHomeContent?

(In reply to Brian Nicholson (:bnicholson) from comment #15) > Comment on attachment 729258 [details] [diff] [review] > Patch 1: Move views to widget > > That's my small trick with your favorite GeckoViewsFactory. :D "Gecko." > > doesn't have any context. We will short-circuit Android and give it the > > required view, if the View starts with a "Gecko." signature. This is better > > than writing "org.mozilla.gecko.widget." > > Why is it better? It's not at all clear what it means unless you're familiar > with GeckoViewsFactory as it doesn't follow Android convention. It seems > like a bad idea to couple the names of views with the factory responsible > for processing them. And, to make matters worse, we've already seen that > this breaks compatibility with any processing tools such as Eclipse that > don't understand the "Gecko." prefix. If we really need this factory code, > is there any reason we can't simply use attributes to designate whether > something needs processing (such as gecko:use_gecko_factory="true")? > > Also, what determines whether a view should be processed by the Gecko > factory versus the standard Android factory? I don't understand why you have > "Gecko.AboutHomeContent", when right below it you use > "org.mozilla.gecko.FindInPageBar". Ofcourse, this breaks eclipse. But "org.mozilla.gecko.widget" is too lengthy to parse. Until we have eclipse ready we can follow this convention. Also, if we want to make this eclipse ready, we need to add "xmlns:tools" to each xml file and give a context. Thats more work for now :( Gecko.AboutHomeContent is new. I'm adding this notation for new views i am adding. org.mozilla.gecko.FindInPageBar is there for more than a year now. And since I don't want to touch that code with this refactor patch, I didn't change it. But, soon it will also change. :D

(In reply to Brian Nicholson (:bnicholson) from comment #16) > > android:layout_above="@+id/find_in_page"> > > > > <include layout="@layout/shared_ui_components"/> > > > > + <Gecko.AboutHomeContent android:id="@+id/abouthome_content" > > And wouldn't this need be Gecko.widget.AboutHomeContent? Not necessarily. We never to that in GeckoViewsFactory. "Gecko." is more like a conventation to say "hey, this is a custom view. Don't search in android's views". Internally a "Gecko.CustomView" could be mapped to "org.mozilla.gecko.widget.small.internal.CustomView". :D

Comment on attachment 729258 [details] [diff] [review] Patch 1: Move views to widget We should drop the "Gecko." prefix as discussed on IRC, but that can be handled in a separate bug.

Comment on attachment 729259 [details] [diff] [review] Part 2: Split AboutHomeContent Review of attachment 729259 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this was mostly just keeping existing code but separating it into more manageable classes, so I assume the methods are all the same as they were. r+ as long as there aren't any significant changes I'm not aware of. ::: mobile/android/base/widget/RemoteTabsSection.java @@ +51,5 @@ > + Tabs.getInstance().loadUrl((String) v.getTag(), flags); > + } > + }; > + } > + nit: trailing whitespace @@ +77,5 @@ > + > + clear(); > + > + String client = null; > + nit: trailing whitespace @@ +90,5 @@ > + row.setTag(tab.url); > + addItem(row); > + row.setOnClickListener(mRemoteTabClickListener); > + } > + nit trailing whitespace etc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec706efc6c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/c3cb49f39533 Phew! Third patch will be pushed after the next merge as it changes the existing interface.

Ah, the white spaces: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e0dcc905c5

Filed bug 855888 for moving the "Gecko." to "org.mozilla.gecko.widget."

https://hg.mozilla.org/mozilla-central/rev/9ec706efc6c0 https://hg.mozilla.org/mozilla-central/rev/c3cb49f39533 https://hg.mozilla.org/mozilla-central/rev/e1e0dcc905c5

closed? Also see possible regression in bug 857459

The meta bug for the about:home redesign is bug 862793 now. We can close this one I guess.

(In reply to Lucas Rocha (:lucasr) from comment #26) > The meta bug for the about:home redesign is bug 862793 now. We can close > this one I guess. This was left open because Sriram never landed his last patch. I'm not sure what's required to land that, but maybe Brian knows. But I think we should just file a follow-up to land that patch, since the main patches in this bug landed long ago.

(In reply to :Margaret Leibovic from comment #27) > (In reply to Lucas Rocha (:lucasr) from comment #26) > > The meta bug for the about:home redesign is bug 862793 now. We can close > > this one I guess. > > This was left open because Sriram never landed his last patch. I'm not sure > what's required to land that, but maybe Brian knows. But I think we should > just file a follow-up to land that patch, since the main patches in this bug > landed long ago. I think the last patch drops the Firefox logo on about:home, and we didn't want to land that until about:home was redesigned. I agree that we should take care of that in a separate bug.