862794 - About:home "visited" panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

This is the bug to track work on the new "visited" panel on about:home. 

This panel is intended to display a places the user has been, including their frecency (awesomebar) list, history, and tabs from last time

Mockups to follow shortly.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: Mockups of "Visited" panel

Comment 2

[Ian Barlow (:ibarlow)]

Work in this bug should include

* Creating a frecency based history list
* Creating secondary sections for chronological History and Tabs from last time
* Restyling list elements
* Set text overflow to fade to white, instead of using an ellipsis
* Apply dominant background / border colors to list items with small (16px) favicons. See bug 837392

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (1/2) Factor out the code for handling favicon loaders

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: (2/2) Implement Visited page in about:home

Comment 5

[Lucas Rocha (:lucasr)]

FYI: the "history" and "tabs from last time" will be implemented in separate patches (see bug 882715 and bug 882716)

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 762070 [details] [diff] [review]
(1/2) Factor out the code for handling favicon loaders

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

::: mobile/android/base/home/FaviconsLoader.java
@@ +29,5 @@
> +    // Argument containing list of urls for the favicons loader
> +    private static final String FAVICONS_LOADER_URLS_ARG = "urls";
> +
> +	private FaviconsLoader() {
> +	}

AH TABS

@@ +84,5 @@
> +	public static void restartFromCursor(LoaderManager manager, int loaderId,
> +			LoaderCallbacks<Cursor> callbacks, Cursor c) {
> +		// If there urls without in-memory favicons, trigger a new loader
> +		// to load the images from disk to memory.
> +		ArrayList<String> urls = getUrlsWithoutFavicon(c);

NO! MOAR TABS

@@ +99,5 @@
> +		return new FaviconsCursorLoader(context, urls);
> +	}
> +
> +    private static class FaviconsCursorLoader extends SimpleCursorLoader {
> +        private ArrayList<String> mUrls;

nit: final

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 762071 [details] [diff] [review]
(2/2) Implement Visited page in about:home

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

::: mobile/android/base/home/VisitedPage.java
@@ +79,5 @@
> +    @Override
> +    public void onDetach() {
> +        super.onDetach();
> +
> +        mUrlOpenListener = null;

You should also clear the mInflater reference here.

@@ +86,5 @@
> +    @Override
> +    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
> +        // All list views are styled to look the same with a global activity theme.
> +        // If the style of the list changes, inflate it from an XML.
> +        mList = new HomeListView(container.getContext());

Set mList to null in onDestroyView() so it doesn't leak.

@@ +94,5 @@
> +    @Override
> +    public void onViewCreated(View view, Bundle savedInstanceState) {
> +        super.onViewCreated(view, savedInstanceState);
> +
> +        mList.setOnItemClickListener(this);

Do you think it would be better to register this handler internally like you do with CursorLoaderCallbacks? That way VisitedPage wouldn't have to expose a new onItemClick() method.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) (gone through June 22) from comment #6)
> Comment on attachment 762070 [details] [diff] [review]
> (1/2) Factor out the code for handling favicon loaders
> 
> Review of attachment 762070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/FaviconsLoader.java
> @@ +29,5 @@
> > +    // Argument containing list of urls for the favicons loader
> > +    private static final String FAVICONS_LOADER_URLS_ARG = "urls";
> > +
> > +	private FaviconsLoader() {
> > +	}
> 
> AH TABS

Eek, removed :-)

> @@ +84,5 @@
> > +	public static void restartFromCursor(LoaderManager manager, int loaderId,
> > +			LoaderCallbacks<Cursor> callbacks, Cursor c) {
> > +		// If there urls without in-memory favicons, trigger a new loader
> > +		// to load the images from disk to memory.
> > +		ArrayList<String> urls = getUrlsWithoutFavicon(c);
> 
> NO! MOAR TABS

Argh, removed.

> @@ +99,5 @@
> > +		return new FaviconsCursorLoader(context, urls);
> > +	}
> > +
> > +    private static class FaviconsCursorLoader extends SimpleCursorLoader {
> > +        private ArrayList<String> mUrls;
> 
> nit: final

Fixed.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) (gone through June 22) from comment #7)
> Comment on attachment 762071 [details] [diff] [review]
> (2/2) Implement Visited page in about:home
> 
> Review of attachment 762071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/VisitedPage.java
> @@ +79,5 @@
> > +    @Override
> > +    public void onDetach() {
> > +        super.onDetach();
> > +
> > +        mUrlOpenListener = null;
> 
> You should also clear the mInflater reference here.

Done.

> @@ +86,5 @@
> > +    @Override
> > +    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
> > +        // All list views are styled to look the same with a global activity theme.
> > +        // If the style of the list changes, inflate it from an XML.
> > +        mList = new HomeListView(container.getContext());
> 
> Set mList to null in onDestroyView() so it doesn't leak.

Setting to references null is only necessary when you're dealing with a component that we don't own. Once the fragment is removed, it will eventually be GC'd together with its internal bits.

In other words, I'd be surprised if mList is leaked if we don't set it to null here.

> @@ +94,5 @@
> > +    @Override
> > +    public void onViewCreated(View view, Bundle savedInstanceState) {
> > +        super.onViewCreated(view, savedInstanceState);
> > +
> > +        mList.setOnItemClickListener(this);
> 
> Do you think it would be better to register this handler internally like you
> do with CursorLoaderCallbacks? That way VisitedPage wouldn't have to expose
> a new onItemClick() method.

Not sure I follow. You mean encapsulate the onItemClick handler into a class so that it can be shared or something? I feel like this is not enough code to warrant such an effort.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Setting to references null is only necessary when you're dealing with a
> component that we don't own. Once the fragment is removed, it will
> eventually be GC'd together with its internal bits.

Yeah, I was just referring to the period when the fragment is alive. It won't affect us as long as we're using a FragmentStatePagerAdapter, but if we decide to switch to a FragmentPagerAdapter, we don't want to hold onto that view reference when going between pages.

> Not sure I follow. You mean encapsulate the onItemClick handler into a class
> so that it can be shared or something? I feel like this is not enough code
> to warrant such an effort.

Not so it can be shared, but just so it isn't exposed as an additional public method. Using inheritance over composition here unnecessarily breaks encapsulation when methods are made public in a class just so they can implement internal callbacks. It also makes things harder to read IMO when a class implements multiple listeners and I have to figure out which methods are associated with which interfaces.

We already do this in lots of other places though (both in Gecko and Java), so I don't feel too strongly about it -- just a suggestion.

Comment 11

[Sriram Ramasubramanian [:sriram]]

"Using inheritance over composition here unnecessarily breaks
encapsulation when methods are made public in a class just so they can
implement internal callbacks."

Woah!

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Implement Visited page in about:home

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 764233 [details] [diff] [review]
Implement Visited page in about:home

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

Nice.

Comment 14

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/projects/fig/rev/5e477b9520a2
http://hg.mozilla.org/projects/fig/rev/3561c794b2e4

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e477b9520a2
https://hg.mozilla.org/mozilla-central/rev/3561c794b2e4