Split AboutHomeContent into smaller views

RESOLVED FIXED in Firefox 22

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 22
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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

Updated

6 years ago
Assignee: nobody → sriram
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
(Assignee)

Comment 3

6 years ago
Created attachment 727392 [details] [diff] [review]
Part 1: Remove images

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.
Attachment #727392 - Flags: review?(mark.finkle)
(Assignee)

Comment 4

6 years ago
Created attachment 727395 [details] [diff] [review]
Part 2: Move views

Move about:home specific views to widget/ directory.
Attachment #727395 - Flags: review?(mark.finkle)
(Assignee)

Comment 5

6 years ago
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!
Attachment #727396 - Flags: review?(mark.finkle)
(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.
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 729258 [details] [diff] [review]
Patch 1: Move views to widget

This moves the AboutHomeContent to widget/ folder.
Attachment #727395 - Attachment is obsolete: true
Attachment #727395 - Flags: review?(mark.finkle)
Attachment #729258 - Flags: review?(bnicholson)
(Assignee)

Comment 10

6 years ago
Created attachment 729259 [details] [diff] [review]
Part 2: Split AboutHomeContent

This splits AboutHomeContent to smaller views.
Attachment #727396 - Attachment is obsolete: true
Attachment #727396 - Flags: review?(mark.finkle)
Attachment #729259 - Flags: review?(bnicholson)
(Assignee)

Comment 11

6 years ago
Created attachment 729260 [details] [diff] [review]
Part 3: Remove images

Ah, so clean! Removes the image.
Attachment #727392 - Attachment is obsolete: true
Attachment #727392 - Flags: review?(mark.finkle)
Attachment #729260 - Flags: review?(bnicholson)
(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?
(Assignee)

Comment 14

6 years ago
(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?
(Assignee)

Comment 17

6 years ago
(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
(Assignee)

Comment 18

6 years ago
(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.
Attachment #729258 - Flags: review?(bnicholson) → review+
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.
Attachment #729259 - Flags: review?(bnicholson) → review+
Attachment #729260 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 21

6 years ago
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.
Whiteboard: [leave open]
(Assignee)

Comment 23

6 years ago
Filed bug 855888 for moving the "Gecko." to "org.mozilla.gecko.widget."
Depends on: 855888
closed?

Also see possible regression in bug 857459
Depends on: 857459
Depends on: 838793

Updated

6 years ago
Depends on: 862234
Blocks: 862793
The meta bug for the about:home redesign is bug 862793 now. We can close this one I guess.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(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.
Summary: about:home needs a re-design → Split AboutHomeContent into smaller views
Whiteboard: [leave open]

Updated

6 years ago
Depends on: 864510

Updated

6 years ago
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.