With the new Multipage about:home work underway, we should start thinking about ways to let add-on developers create their own custom panels that users can install. Imagine, for example, an Evernote or Pocket panel that could be added to the Firefox for Android homepage. This shouldn't block the initial about:home redesign release, but we should still start thinking about it.

We're going to fix this issue as part of the new "hub" work that ibarlow has been designing. To start, add-ons will be able to create pages with different kinds of lists, where the add-on supplies the data, and we build the UI with it. I have a prototype starting to work, so I'll assign myself to this bug. Our current game plan lets the add-on do everything in JS, so I'm removing the dependency on bug 799631.

Here's my WIP to start developing this JS API to let add-ons add new pages to about:home (right now all it can add is a list of items). I just hacked browser.js to test out the API. I haven't tested on a super slow device, but on my Nexus 4 this shared prefs dance is good for making sure the pages are created on startup before gecko is loaded. I also decided to send along a message to Java the first time an add-on adds a page, so that it will show up on first run if the add-on is bundled. However, maybe this is premature and we'd do something else to deal with this. I also decided that the list items will all have urls, and we just handle loaded the url in Java, but perhaps we should be more generic about what can go in that second line of the two line page row, and then just let the add-on register some click handler. Thinking about this now, I think I like that more than what's in this patch now. Some things I haven't done yet, but would probably be best to split into different bugs: * Icons in the lists * Arbitrarily long lists * Creating a way to let the add-on dynamically update the items in the list (however, I just realized that every time the add-on is loaded we're loading a new set of items, so it actually could update itself every time the browser starts up) * Tests! I think it would actually be pretty straightforward to create some smoketests for this, similar to testHomeBanner. But maybe we should wait for the API to stabilize a bit.

General comments: 0. This looks pretty nice. Being able to explore the core API and the Java UI configurations without focusing too much on data abstraction is great for first steps. Keep moving ahead without worrying too much about #1 and #2 yet. 1. Why SharedPreferences? We'd need to worry about profiles, since add-ons are profile based. Why not just save a JSON file to the profile. That stops Guest Browsing from showing the custom pages since the add-ons and the JSON is in the main profile. 2. I'm still holding out for using an SQLite file for the data repo. Passing data, even limited to 20, over JSON messaging seems like a problem. Passing a path to the SQLite DB seems easier. Then the Java UI can decide how many items to show. 3. We should try out more variations, like a Grid layout with images, and also think about whether all items are URL-based.

(In reply to Mark Finkle (:mfinkle) from comment #3) > General comments: > 0. This looks pretty nice. Being able to explore the core API and the Java > UI configurations without focusing too much on data abstraction is great for > first steps. Keep moving ahead without worrying too much about #1 and #2 yet. > > 1. Why SharedPreferences? We'd need to worry about profiles, since add-ons > are profile based. Why not just save a JSON file to the profile. That stops > Guest Browsing from showing the custom pages since the add-ons and the JSON > is in the main profile. We came up with this when just thinking of an easy way for Java/JS to access some storage, but I agree, the profile bit makes this less than ideal. A JSON file in the profile sounds like a decent idea to me. However, will we need to worry about JS writing to this file while we're trying to read it in Java? Not saying that's a deal breaker, but what kinds of things would we need to do to make sure this approach is robust? > 2. I'm still holding out for using an SQLite file for the data repo. Passing > data, even limited to 20, over JSON messaging seems like a problem. Passing > a path to the SQLite DB seems easier. Then the Java UI can decide how many > items to show. Yeah, the current implementation is definitely just a temporary solution. I'd like to play around with this SQLite idea, it seems like it could work well. With this approach, CustomListPage could really just look a lot like other other about:home list pages that use cursors. > 3. We should try out more variations, like a Grid layout with images, and > also think about whether all items are URL-based. Yeah, as I mentioned up above, right now my patch just loads the item's URL on the Java side when it's tapped, but maybe an add-on would want to do other things, like show some offline content. I don't want to scope creep this feature to include some alternate offline reading list support, but it's something to think about when designing the API.

Comment on attachment 830617 [details] [diff] [review] WIP - API to add a custom list page in JS Review of attachment 830617 [details] [diff] [review]: ----------------------------------------------------------------- > > 0. This looks pretty nice. Being able to explore the core API and the Java > > UI configurations without focusing too much on data abstraction is great for > > first steps. Concur, but regardless I've added some more forward-facing notes in my review comments. > > 1. Why SharedPreferences? We'd need to worry about profiles, ... Other than profile multiplexing, SharedPreferences is a near-perfect solution to this problem. Android handles the concurrency, caching, editing, persistence, reliable writes, and a nice API. More importantly, we already need to solve the problem of per-profile SharedPreferences for our other settings and features: locale choices, telemetry opt-in, etc. etc. etc. Special-casing that solution just rolls the problem on down the hill. If this is the feature that finally pushes us into adding a profile-aware SharedPreferences multiplexer to our prefs accessor, then great! Let's raise the flag and blow the horn! > Yeah, the current implementation is definitely just a temporary solution. > I'd like to play around with this SQLite idea, it seems like it could work > well. With this approach, CustomListPage could really just look a lot like > other other about:home list pages that use cursors. Yupyup. See review comment about adapters. I would quite like to see a JS filtered ContentProvider abstraction over a single DB. This buys us a lot of change notifications and more complex storage. > > 3. We should try out more variations, like a Grid layout with images, and > > also think about whether all items are URL-based. > > Yeah, as I mentioned up above, right now my patch just loads the item's URL > on the Java side when it's tapped, but maybe an add-on would want to do > other things, like show some offline content. I don't want to scope creep > this feature to include some alternate offline reading list support, but > it's something to think about when designing the API. It's probably fair to say that this is a v0.1 data format, and not gate on getting that right up-front -- in two ways: * Firstly, version the shit out of this. * Secondly, we're going to have a non-trivial list of potential fields: consider that we're aiming to represent playlists, article lists, images, files, etc., in a number of different views (big image, small image, list, tile...), and that some of these fields need to be sortable (so we can't just bump these into "big text", "small text"). That's too much to bite off for initial development, so rock on the way you're doing it. ::: mobile/android/base/home/CustomListPage.java @@ +96,5 @@ > + } > + > + // Listens for "CustomListPage:Data" > + @Override > + public void handleMessage(String event, JSONObject message) { I know this is very much a stub, but this should be replaced by a CustomListPageAdapter or something -- backed by JSON or a DB with an ID query. ::: mobile/android/base/home/HomePager.java @@ +179,5 @@ > + // Read shared pref and create additonal pages if necessary > + (new UiAsyncTask<Void, Void, String>(ThreadUtils.getBackgroundHandler()) { > + @Override > + public synchronized String doInBackground(Void... params) { > + SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(getContext()); Sidenote: why do we have two completely different sets of SharedPreferences? mobile/android/base/GeckoApp.java 246: return GeckoApp.sAppContext.getSharedPreferences(GeckoApp.PREFS_NAME, 0); 1716: return PreferenceManager.getDefaultSharedPreferences(this) IMO we should eliminate all uses of PreferenceManager: http://stackoverflow.com/questions/18204708/preferencemanager-getdefaultsharedpreferences-vs-getpreferences Do you agree? @@ +390,5 @@ > } > + > + // Listens for "CustomListPage:Added" > + @Override > + public void handleMessage(String event, JSONObject message) { I'd like to see this, and almost all of the rest of the code added to this file, moved into its own class -- CustomPageManager, for example. There should be a clear API on the homepager -- it's a thing that's instantiated, and another component is told when the homepager is ready, and uses that API to add pages to it. (That API might be push -- addTab -- or pull -- getAdditionalAdapter.) That way there's a clear separation of concerns. The CustomPageManager: * Owns the backing store for the list (SharedPreferences right now) * Listens for Gecko messages for adding and removing pages * Will expose the interface that the settings UI for this feature works with The CustomPageListView: * Displays the contents of an adapter The CustomPageListAdapter: * Fetches cached data, passing events ("hey, displayed, new data?") to the manager. * Abstracts over storage (JSON messages for now, DB later) via different implementations. That way the HomePager doesn't need to know anything about what a page consists of (as it shouldn't), the View doesn't need to know about the backing store for data, etc. etc.

Margaret: consider whether you want to split this bug into three -- management of custom panel add-ons; rendering of custom panel data; delivery of data from custom panel add-ons to Java?

Sometimes I feel like I have "MVC 4 LYFE" tattooed on my knuckles. Don't tell the OO purists that I'm a Lisper.

For very deep consideration: http://api.dashclock.googlecode.com/git/reference/com/google/android/apps/dashclock/api/DashClockExtension.html

(In reply to Richard Newman [:rnewman] from comment #5) > > > 1. Why SharedPreferences? We'd need to worry about profiles, ... > > Other than profile multiplexing, SharedPreferences is a near-perfect > solution to this problem. Android handles the concurrency, caching, editing, > persistence, reliable writes, and a nice API. > > More importantly, we already need to solve the problem of per-profile > SharedPreferences for our other settings and features: locale choices, > telemetry opt-in, etc. etc. etc. I believe telemetry opt-in is a gecko preference (just want to confirm that we're not currently shipping a feature with this problem). > Special-casing that solution just rolls the problem on down the hill. If > this is the feature that finally pushes us into adding a profile-aware > SharedPreferences multiplexer to our prefs accessor, then great! Let's raise > the flag and blow the horn! Let's file a separate bug for this. I'll just punt on moving away from SharedPreferences in the meantime. > ::: mobile/android/base/home/HomePager.java > @@ +179,5 @@ > > + // Read shared pref and create additonal pages if necessary > > + (new UiAsyncTask<Void, Void, String>(ThreadUtils.getBackgroundHandler()) { > > + @Override > > + public synchronized String doInBackground(Void... params) { > > + SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(getContext()); > > Sidenote: why do we have two completely different sets of SharedPreferences? Probably an oversight by different people implementing different SharedPreferences features? > mobile/android/base/GeckoApp.java > 246: return > GeckoApp.sAppContext.getSharedPreferences(GeckoApp.PREFS_NAME, 0); > 1716: return PreferenceManager.getDefaultSharedPreferences(this) > > IMO we should eliminate all uses of PreferenceManager: > > http://stackoverflow.com/questions/18204708/preferencemanager- > getdefaultsharedpreferences-vs-getpreferences > > Do you agree? I filed bug 940575 to investigate this.

(In reply to Richard Newman [:rnewman] from comment #5) Thinking about this proposed class structure, I have a few questions... > The CustomPageManager: > > * Owns the backing store for the list (SharedPreferences right now) > * Listens for Gecko messages for adding and removing pages > * Will expose the interface that the settings UI for this feature works with What should the lifecycle of this manager be? Should it be something that HomePager creates and owns? I feel like it would be good to avoid exposing it outside of the `home` package. > The CustomPageListView: > > * Displays the contents of an adapter I think this should just all happen in CustomListPage, since we'll still need some sort of Page class that extends HomeFragment. And if we're just using a basic list of TwoLinePageRows for this first iteration, we can just use a normal ListView in this page (similarly to what the current WIP does). > The CustomPageListAdapter: > > * Fetches cached data, passing events ("hey, displayed, new data?") to the > manager. > * Abstracts over storage (JSON messages for now, DB later) via different > implementations. I think what we really want here is to create a content provider, and the adapter can then be a really basic CursorAdapter, similar to what we use for some of the current pages, like ReadingListPage.

(In reply to :Margaret Leibovic from comment #10) > What should the lifecycle of this manager be? Should it be something that > HomePager creates and owns? I feel like it would be good to avoid exposing > it outside of the `home` package. That's pretty much my thinking, yes (so long as the HomePager continues to live when about:home is hidden). The public interface for it is largely decoupled Gecko messages, and I think it's fair to assume that HomePager settings control will be relatively tightly coupled with .home. > I think this should just all happen in CustomListPage, since we'll still > need some sort of Page class that extends HomeFragment. And if we're just > using a basic list of TwoLinePageRows for this first iteration, we can just > use a normal ListView in this page (similarly to what the current WIP does). WFM; I was mostly thinking that custom pages might want to swap out different views (lists, tiles), combine multiple views in one page (as we do with Top Sites), etc. Smushing them together is merging view and controller, from a traditional OO standpoint. > I think what we really want here is to create a content provider, and the > adapter can then be a really basic CursorAdapter, similar to what we use for > some of the current pages, like ReadingListPage. Yeah, that's probably gonna be the case, in that the ContentProvider just returns a Cursor, but it can back that Cursor with whatever it wants. Same kinda deal.

(In reply to :Margaret Leibovic from comment #9) > I believe telemetry opt-in is a gecko preference (just want to confirm that > we're not currently shipping a feature with this problem). To reply to this: yeah, telemetry is Gecko, my bad. Health Report is a native Android pref, though, and unlike Sync it doesn't handle its own profile disambiguation in the pref key. I punted on that in the expectation of needing a general solution before we offered multi-profile support (and here we are!). So you're correct about telemetry, but we do currently ship per-profile features without per-profile prefs, one of which is FHR.

Here's an updated approach to letting add-ons register themselves to add a page. Some thoughts: * I'm not sold on the `Home.list` API, maybe it should be `Home.page`, then add-ons can specify a type of page in the options? * I commented inline, but right now we need to use PreferenceManager if we're using SharedPreferences.jsm to set the pref * I started a stub of a ContentProvider, named `CustomPageProvider`, but I think we should move that work into a separate bug * We should think about a schema for the database, I'm thinking we could do a table per page type, then have page_id column that we use to filter on in our queries * We should think of what kind of API we want the add-on to use to update its data. Should it pass JSON to our JS, which we then stuff into a DB appropriately? This should also go in another bug :)

(In reply to Richard Newman [:rnewman] from comment #12) > So you're correct about telemetry, but we do currently ship per-profile > features without per-profile prefs, one of which is FHR. CrashReporter too, but that's not a SharedPreference issue

(In reply to Richard Newman [:rnewman] from comment #5) > > > 1. Why SharedPreferences? We'd need to worry about profiles, ... > > Other than profile multiplexing, SharedPreferences is a near-perfect > solution to this problem. Android handles the concurrency, caching, editing, > persistence, reliable writes, and a nice API. Shoving a large JSON string into a SharedPreference reminds of the same thing that used to happen in Gecko preferences. It's suboptimal and we don't recommend developers do it anymore.

(In reply to :Margaret Leibovic from comment #13) > Created attachment 8335032 [details] [diff] [review] > WIP v2 - API to allow add-ons to register custom page > > Here's an updated approach to letting add-ons register themselves to add a > page. > > Some thoughts: > * I'm not sold on the `Home.list` API, maybe it should be `Home.page`, then > add-ons can specify a type of page in the options? I was thinking the same. "Home.list" doesn't feel right, but "Home.page" with a way to describe your pageType feels better. > * I started a stub of a ContentProvider, named `CustomPageProvider`, but I > think we should move that work into a separate bug Agreed > * We should think about a schema for the database, I'm thinking we could do > a table per page type, then have page_id column that we use to filter on in > our queries Agreed > * We should think of what kind of API we want the add-on to use to update > its data. Should it pass JSON to our JS, which we then stuff into a DB > appropriately? This should also go in another bug :) I strongly suggest we do not send any data over the bridge as JSON. We have to prepare for the cases where hundreds or thousands of rows of data exist and the data has large thumbnails. I'd like to explore a ContentProvider JS wrapper that saves directly to a DB from JS. And yes, this is a separate bug too.

(In reply to Mark Finkle (:mfinkle) from comment #15) > Shoving a large JSON string into a SharedPreference reminds of the same > thing that used to happen in Gecko preferences. It's suboptimal and we don't > recommend developers do it anymore. Are we talking about different problems? I'm referring to storing the list of custom tabs (which we need immediately, want to read from the home screen and write from a couple of places, etc.), not their data.

Morphing the title of the bug to match. Now is probably a good time to think about "pages" versus "panels" versus "lists" as terminology in the context of Hub, because this bug is heading towards descriptors. The current mockups imply that each addon provides one list, which appears as one panel in one new page, but I don't think that's required, or even desirable in all but the trivial case. Once we get up to five or six providers, swiping all the way from History to the sixth provider is going to really suck; discoverability of new content will become painful. And the very next thing a provider will want to do after displaying a list of things is to do something like top sites: displaying some things at the top of the screen in one way, and some other things underneath. (Pocket's featured items at the top, with the rest of your queue underneath, for example, or suggested items followed by your own items, or...) So I suggest that: * Pages are something the user manages. When they add a new provider to their home screen, it gets a page by default, but that's not necessarily the only way they can get to that content -- it could appear in Awesomebar results, for example, or via some other view similar to the Add List dialog itself. You can remove a page, re-add it, reorder pages, etc. * Panels are fragments within a page. I posit that these are initially suggested by the provider, but might one day be adjusted by the user. Typically there will be one. * Lists are collections of data displayed by panels. From this perspective, the user's pages refer to a subset of providers, a provider's page refers to a subset of possible panels, and N panels refer to <N lists. From the storage side, each list is a named section of a database, which is wrapped as a content provider-like thing for the provider to fill. For example: Provider = Pocket Page = "Pocket" Panels: Recommended: type = "squares", list = "com.getpocket.recommended" My Queue: type = "thumbnail-list", list = "com.getpocket.queue" Or, to shine a different light: Provider = Firefox Page = "Top Sites" Panels: Thumbnails: type = "squares", list = "org.mozilla.top" List: type = "icon-list", list = "org.mozilla.top", offset = 6 Thoughts?

(In reply to Richard Newman [:rnewman] from comment #17) > (In reply to Mark Finkle (:mfinkle) from comment #15) > > > Shoving a large JSON string into a SharedPreference reminds of the same > > thing that used to happen in Gecko preferences. It's suboptimal and we don't > > recommend developers do it anymore. > > Are we talking about different problems? > > I'm referring to storing the list of custom tabs (which we need immediately, > want to read from the home screen and write from a couple of places, etc.), > not their data. Right, I'm thinking the same. I just assume the size of the page data may not be small. Sure, it always starts small, but the slope is slippery.

(In reply to Mark Finkle (:mfinkle) from comment #19) > Right, I'm thinking the same. I just assume the size of the page data may > not be small. Sure, it always starts small, but the slope is slippery. The reason why I'm quite inclined to start with SharedPreferences is that this will already have been efficiently slurped off the disk and be resident in memory by the time about:home is shown. Dumping stuff into a file is a separate disk access. There is definitely a point at which storing more data in the same prefs file turns a perf win at this stage into a perf loss earlier in startup. So long as we're encapsulating the prefs access, and *versioning it* (either by key or by prepending in the value), I think we're good to evolve this later.

(In reply to Richard Newman [:rnewman] from comment #18) > Now is probably a good time to think about "pages" versus "panels" versus > "lists" as terminology in the context of Hub, because this bug is heading > towards descriptors. > > The current mockups imply that each addon provides one list, which appears > as one panel in one new page, but I don't think that's required, or even > desirable in all but the trivial case. > > Once we get up to five or six providers, swiping all the way from History to > the sixth provider is going to really suck; discoverability of new content > will become painful. I agree this is something important to think about in the future. I feel like the best initial solution to this is to allow users to modify their set of pages, so that they could hide the "Bookmarks" and "Reading List" pages, if there are other custom pages they find more useful. > And the very next thing a provider will want to do after displaying a list > of things is to do something like top sites: displaying some things at the > top of the screen in one way, and some other things underneath. (Pocket's > featured items at the top, with the rest of your queue underneath, for > example, or suggested items followed by your own items, or...) I would rather not worry about this right now. I agree we should be thoughtful to not stick ourselves with a poorly architected system that makes this difficult, but I think if we just s/page/panel with the current approach, we're alright. CustomListPage has "page" in its name, but as it's currently implemented, it's just a Fragment that doesn't depend on being a "page" in the HomePager. So maybe we can just rename it CustomListPanel. Looking through HomePager, it looks like we actually support any generic class as a page in TabsAdapter: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java#227 We need to do some refactoring around this concept of a HomePager page (bug 940565), so maybe as part of this we can make this relationship more explicit. > So I suggest that: > > * Pages are something the user manages. When they add a new provider to > their home screen, it gets a page by default, but that's not necessarily the > only way they can get to that content -- it could appear in Awesomebar > results, for example, or via some other view similar to the Add List dialog > itself. > > You can remove a page, re-add it, reorder pages, etc. I don't want to extend the scope of this bug to talk about "adding a provider" in a generic sense. What we're trying to do here is just provide a way for an add-on to add a panel to about:home. If that same add-on wants to do other things, like stick search results in the awesomescreen results, that should be another API. Or if you're talking about filtering results from the same list data, maybe that could be some flag they set. But I don't want to talk about this here :) > * Panels are fragments within a page. I posit that these are initially > suggested by the provider, but might one day be adjusted by the user. > Typically there will be one. For v1 here, I think we should keep a 1-1 panel-page ratio, but yes, in the future we could think about ways to let a provider have multiple panels in the same page. The way we're using Fragments, the current implementation shouldn't be hard to extend. > * Lists are collections of data displayed by panels. Agreed. > From this perspective, the user's pages refer to a subset of providers, a > provider's page refers to a subset of possible panels, and N panels refer to > <N lists. > > From the storage side, each list is a named section of a database, which is > wrapped as a content provider-like thing for the provider to fill. > > For example: > > Provider = Pocket > Page = "Pocket" > Panels: > Recommended: type = "squares", list = "com.getpocket.recommended" > My Queue: type = "thumbnail-list", list = "com.getpocket.queue" > > Or, to shine a different light: > > Provider = Firefox > Page = "Top Sites" > Panels: > Thumbnails: type = "squares", list = "org.mozilla.top" > List: type = "icon-list", list = "org.mozilla.top", offset = 6 > > Thoughts? This seems reasonable, but as I mentioned above, I don't think we should worry about this multi-panel case right now. All my other thoughts are mentioned above :)

Updating the summary here to try to get our page/panel/list jargon in order. Following recent conversations, our current definition is this: about:home has multiple "pages", which the user can rearrange (bug 942231), and within these pages are "panels", which hold "lists". Each provider can register to provide data to multiple lists (e.g. an RSS provider could create a different list for each RSS feed), but users (through Firefox) are the ones who control the different pages where they go. For v1 of this feature, each page will have one panel, and each panel will have one list, but we should architect this will future flexibility in mind.

(In reply to :Margaret Leibovic from comment #22) Good summary, but one tweak: > and within these pages are "panels", which hold "lists". Each provider can s/hold/display. In MVC jargon, panels are views, lists are models. You might say that a panel is "bound to" a list.

Margaret and Richard, instead of 'panel', how do you feel about calling it something like 'widget'? I just worry a little that panel might be more easily confused with pages. I know even now we often use the names interchangeably. Also, widget may better reflect the idea that it can make up a piece of a page and have a list inside of it. Just a thought.

(In reply to Ian Barlow (:ibarlow) from comment #24) > Margaret and Richard, instead of 'panel', how do you feel about calling it > something like 'widget'? Totally fine with choosing a different name. Widget has its own connotations, though! Perhaps "fragment", "pane", ?

(In reply to Richard Newman [:rnewman] from comment #23) > (In reply to :Margaret Leibovic from comment #22) > > Good summary, but one tweak: > > > and within these pages are "panels", which hold "lists". Each provider can > > s/hold/display. > > In MVC jargon, panels are views, lists are models. You might say that a > panel is "bound to" a list. I like the sound of this. I was a bit reluctant to accept that there was a distinction between a list and a panel, but if we say that the list is just a data structure, and the panel displays that data, that makes more sense. (In reply to Richard Newman [:rnewman] from comment #25) > (In reply to Ian Barlow (:ibarlow) from comment #24) > > Margaret and Richard, instead of 'panel', how do you feel about calling it > > something like 'widget'? > > Totally fine with choosing a different name. Widget has its own > connotations, though! Perhaps "fragment", "pane", ? I like "fragment", since it will likely be implemented as an Android Fragment. We may want to call it something else if we expose this concept to users, but this sounds like a good way to go for naming things in the implementation.

Comment on attachment 8335032 [details] [diff] [review] WIP v2 - API to allow add-ons to register custom page Review of attachment 8335032 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the crazy delay here. Looks good for a starter. Some thoughts... ::: mobile/android/base/home/CustomListPage.java @@ +69,5 @@ > + mList.setOnItemClickListener(new AdapterView.OnItemClickListener() { > + @Override > + public void onItemClick(AdapterView<?> parent, View view, int position, long id) { > + final TextView url = (TextView) view.findViewById(R.id.url); > + mUrlOpenListener.onUrlOpen(url.getText().toString(), EnumSet.noneOf(OnUrlOpenListener.Flags.class)); Hmm... I wonder if we'd rather have a custom listener here. I'd rather pass a position to the js addon. That lets it use this "url" field to show something that's not a url if they want (and maybe we'd change that to something like "description"... @@ +97,5 @@ > + protected void load() { > + getLoaderManager().initLoader(LOADER_ID_CUSTOM_LIST, null, mCursorLoaderCallbacks); > + } > + > + private class CustomListAdapter extends CursorAdapter { I'm not sure if we're pointing to a larger view here, but if we're just backing this with an array, we should use an ArrayAdapter. It doesn't look like this has a way to send a list yet though? ::: mobile/android/base/home/CustomPageManager.java @@ +23,5 @@ > + > +public class CustomPageManager implements GeckoEventListener { > + private static final String LOGTAG = "GeckoCustomPageManager"; > + > + public class PageInfo { I think this PageInfo class should probably be public and all of our pages should implement one. Then you can just replace the Page enum with an ArrayList of PageInfo objects. Its probably also going to become larger/subclassed to handle storing data for custom pages. The rest of this class I think should be merged into the HomePager since its very directly involved with the one thing the HomePager basically does. ::: mobile/android/base/home/HomePager.java @@ +97,5 @@ > public HomePager(Context context, AttributeSet attrs) { > super(context, attrs); > mContext = context; > > + mCustomPageManager = new CustomPageManager(context, this); This seems a little confusing to me. The HomePager should be keeping track of the list of pages to show, or the PageManager, but I don't think we need/want both to. Most of this is just because that dumb enum is still in place though, and its hard to see this with it... ::: mobile/android/base/resources/layout/home_custom_page_row.xml @@ +9,5 @@ > + android:minHeight="@dimen/page_row_height" > + android:gravity="center_vertical" > + android:orientation="horizontal"> > + > + <include layout="@layout/two_line_page_row"/> I don't totally understand why you even have this? ::: mobile/android/modules/Home.jsm @@ +181,5 @@ > + } > + > + // Bail if the page already exists > + if (page.id in this._pages) { > + return; Should probably log something here at least. @@ +215,5 @@ > + for (let id in this._pages) { > + let page = this._pages[id]; > + pages.push({ id: page.id, title: page.title}); > + } > + this._sharedPrefs.setCharPref(this.PREF_KEY, JSON.stringify(pages)); Neat!

I decided to reduce the scope of my patch to really just be about adding support for a new JS API to let add-ons register new lists, mainly to avoid overlapping with the work lucasr is doing in bug 942231. The main change I made is keeping this API strictly about "lists", not about "pages". However, we're going to have to figure out how this will integrate with the HomePager work in bug 942231. On first glance, it seems like the ListManager I created here is redundant with the HomeConfig stuff, but I think it would be wise to keep the list management separate from the home page management, partially for ease of maintenance, and partially because there may be a day where we we don't want a 1-1 list-page mapping. However, I still need to figure out where an instance of this ListManager will live.

Comment on attachment 8345456 [details] [diff] [review] WIP v3 (reduced scope) Review of attachment 8345456 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by: This patch seems to mix the notion of 'available lists' with the notion of 'the lists displayed in home pager'. They are separate things. IMO, ListManager should only be about fetching the list of available lists to be added to HomePager i.e. it would do the "Get the list of addon-based lists" part from my 'Notes on settings UI...' in bug 942231. For instance, I'd remove any reference to SharedPreferences here. This is probably going to be an implementation detail of our HomeConfigBackend to store the current HomePager configuration (if we decide to take the SharedPreferences route). More specifically, I'd expect the ListManager to only be used by the 'List chooser' UI (see mockup in bug 942875). The way I see this, the flow would *roughly* be like: - User opens 'Home page lists' settings UI + The settings UI will use a HomeConfigLoader to get the current configuration to display. + We trigger a HomeConfig.save() call off main thread every time the user changes the order of the lists, or settings a different default list, etc. + This will trigger the OnChangeListener in our HomeConfigBackend and cause HomePager to refresh its contents if it is visible in background. - User taps on 'Add list' + The 'Add list' UI would be triggered with something like startActivityForResult() + ListManager fetches the list of available addon-based lists - User taps on one of the available lists + The 'Add list' UI creates a PageEntry instance based on the selected item and passes that as a result to the 'Home page lists' UI. + The 'Home page lists' UI gets the chosen PageEntry, adds to the list, and triggers a HomeConfig.save() off main thread. + This will trigger the OnChangeListener in our HomeConfigBackend and cause HomePager to refresh its contents if it is visible in background. I'd probably rename ListManager to something like AddonListManager for clarity.

Comment on attachment 8345456 [details] [diff] [review] WIP v3 (reduced scope) Review of attachment 8345456 [details] [diff] [review]: ----------------------------------------------------------------- I like this! Lets get it landed :) ::: mobile/android/modules/Home.jsm @@ +171,5 @@ > + } > + > + // Bail if the list already exists > + if (list.id in this._lists) { > + return; I wonder if this should throw so that you know your guid was bad...

(In reply to Lucas Rocha (:lucasr) from comment #30) > Comment on attachment 8345456 [details] [diff] [review] > WIP v3 (reduced scope) > > Review of attachment 8345456 [details] [diff] [review]: > ----------------------------------------------------------------- > > Drive-by: > > This patch seems to mix the notion of 'available lists' with the notion of > 'the lists displayed in home pager'. They are separate things. IMO, > ListManager should only be about fetching the list of available lists to be > added to HomePager i.e. it would do the "Get the list of addon-based lists" > part from my 'Notes on settings UI...' in bug 942231. This raises a question about how we want to manage third-party lists. When starting out here, I assumed that having an add-on installed that added a list would always add a list to about:home, and a user would remove that list by un-installing the add-on. I imagined that we would get a list of services to populate the "Add a new list" dialog from somewhere like a.m.o. (or just some hard-coded list we ship), then adding that new list would be equivalent to installing an add-on. I feel like this approach is simpler because we won't have to worry about putting a list in some "disabled" state, but perhaps it is better to include this state so that users can choose to disable some of the built-in lists that we ship (bookmarks, reading list, etc). If we do support this "disabled" state, I think we should make sure these add-ons won't be syncing any data into the content provider, and maybe we should even delete their current data set when they're disabled (don't want to take up user's data space with stuff they don't use). > For instance, I'd remove any reference to SharedPreferences here. This is > probably going to be an implementation detail of our HomeConfigBackend to > store the current HomePager configuration (if we decide to take the > SharedPreferences route). Well, we chose to go with SharedPreferences here because it's a storage that both Java/Gecko can read/write. The ListManager will need to know about what lists are available before Gecko starts up, but Gecko is also going to need to know about those lists once it's up and running. > More specifically, I'd expect the ListManager to > only be used by the 'List chooser' UI (see mockup in bug 942875). > > The way I see this, the flow would *roughly* be like: > - User opens 'Home page lists' settings UI > + The settings UI will use a HomeConfigLoader to get the current > configuration to display. > + We trigger a HomeConfig.save() call off main thread every time the user > changes the order of the lists, or settings a different default list, etc. > + This will trigger the OnChangeListener in our HomeConfigBackend and cause > HomePager to refresh its contents if it is visible in background. > - User taps on 'Add list' > + The 'Add list' UI would be triggered with something like > startActivityForResult() > + ListManager fetches the list of available addon-based lists > - User taps on one of the available lists > + The 'Add list' UI creates a PageEntry instance based on the selected item > and passes that as a result to the 'Home page lists' UI. > + The 'Home page lists' UI gets the chosen PageEntry, adds to the list, and > triggers a HomeConfig.save() off main thread. > + This will trigger the OnChangeListener in our HomeConfigBackend and cause > HomePager to refresh its contents if it is visible in background. > > I'd probably rename ListManager to something like AddonListManager for > clarity. It seems to me like this flow assumes that new lists are only added by the user through this dialog, but I assumed that add-ons would also be able to just automatically add new lists themselves. Once again, this comes back to the assumption that adding/removing a list would be implemented by installing/uninstalling an add-on. Seems like we need to have a bit more discussion about how we all see this add-on implementation working.

This hooks into the work from bug 942231, and it builds on top of my patch from bug 941318 to get some test data. I made the ListManager live inside HomeConfigMemBackend, but I'm open to suggestions about how to re-architect this. One issue that needs to be addressed here is when lists get added while the app is running (e.g. first-run). Right now this just adds all available lists to the home page configuation, but we could evolve that as we work on the settings UI to manage these pages (to fix that, we're not going to want to just always add the built-in pages like we're doing right now, either).

Let's get something landed! This works on top of my patch for bug 941318. Here's an add-on that tests this: https://github.com/leibovic/home-demo/blob/master/bootstrap.js http://people.mozilla.org/~mleibovic/homedemo.xpi Right now you need to restart for changes to take effect, but adding/removing works.

Out of curiosity, what is PROVIDER_ID supposed to do? I am assuming it is a way for the add-on to specify the data in the DB that belongs to it. Is that right? If so, I don't think an integer is the way to go. All add-ons would just use the same value.

(In reply to Mark Finkle (:mfinkle) from comment #35) > Out of curiosity, what is PROVIDER_ID supposed to do? I am assuming it is a > way for the add-on to specify the data in the DB that belongs to it. Is that > right? If so, I don't think an integer is the way to go. All add-ons would > just use the same value. It's a string in this patch, right? PROVIDER_ID is supposed to correspond to the unique id that the add-on passes in Home.list.add(). The current plan is to store the list items for all add-ons in the same table in the DB, but then we can filter on PROVIDER_ID to return the ones we want for the given page. I suppose we need to be careful about add-ons not running into namespace collisions, but right now a second list with the same id will fail to be added. We haven't discussed the details of how add-ons will push their data into the content provider, but we should make sure we think about how to prevent a malicious add-on from pushing data into another add-on's list (e.g. by passing in that list's provider id).

(In reply to :Margaret Leibovic from comment #36) > (In reply to Mark Finkle (:mfinkle) from comment #35) > > Out of curiosity, what is PROVIDER_ID supposed to do? I am assuming it is a > > way for the add-on to specify the data in the DB that belongs to it. Is that > > right? If so, I don't think an integer is the way to go. All add-ons would > > just use the same value. > > It's a string in this patch, right? PROVIDER_ID is supposed to correspond to > the unique id that the add-on passes in Home.list.add(). The current plan is > to store the list items for all add-ons in the same table in the DB, but > then we can filter on PROVIDER_ID to return the ones we want for the given > page. I was thrown by this in the patch in bug 941318: > HomeListItems.PROVIDER_ID + " INTEGER," + But then I realized you are not creating the DB. Still might want to fix that. > I suppose we need to be careful about add-ons not running into namespace > collisions, but right now a second list with the same id will fail to be > added. Sounds good > We haven't discussed the details of how add-ons will push their data into > the content provider, but we should make sure we think about how to prevent > a malicious add-on from pushing data into another add-on's list (e.g. by > passing in that list's provider id). I don't think we need to stress too much. Add-ons will always be able to access other add-on's data if they try hard enough. Let's not design for Fort Knox. Let's design for simplicity.

Comment on attachment 8349602 [details] [diff] [review] patch Review of attachment 8349602 [details] [diff] [review]: ----------------------------------------------------------------- I like the idea of getting this stuff in and iterating on it in tree. It might be good to have a backout plan for when 29 hits aurora (if it won't be too hard...), just to be safe. ::: mobile/android/base/home/HomeConfig.java @@ +199,5 @@ > > public static HomeConfig getDefault(Context context) { > return new HomeConfig(new HomeConfigMemBackend(context)); > } > +} No op change here? ::: mobile/android/base/home/HomeConfigMemBackend.java @@ +16,5 @@ > +import org.mozilla.gecko.util.UiAsyncTask; > + > +import org.json.JSONArray; > +import org.json.JSONException; > +import org.json.JSONObject; Bunch of not used stuff here ::: mobile/android/base/home/ListManager.java @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +package org.mozilla.gecko.home; > + > +import org.mozilla.gecko.GeckoApp; Same with unused imports here. @@ +34,5 @@ > + public ListInfo(String id, String title) { > + this.id = id; > + this.title = title; > + } > + } Is it worth having this separate class and not reusing PageEntry here? I read through the bug a bit, and I'm having trouble remembering the difference between a list that's registered and a page that's registered. I assume that would help me :) @@ +46,5 @@ > + GeckoAppShell.getEventDispatcher().registerEventListener("HomeLists:Added", this); > + } > + > + /** > + * Reads list info from SharedPreferences. Don't call this on the main thread! I wonder if we should add an assert for these types of conditions and just use it. http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/ThreadUtils.java#116 @@ +63,5 @@ > + final JSONArray lists = new JSONArray(prefValue); > + for (int i = 0; i < lists.length(); i++) { > + final JSONObject list = lists.getJSONObject(i); > + final ListInfo info = new ListInfo(list.getString("id"), list.getString("title")); > + listInfos.add(info); I wonder if its worth wrapping this in a separate try-catch so that one bad list doesn't take down the rest.... @@ +80,5 @@ > + public void handleMessage(String event, JSONObject message) { > + try { > + final ListInfo info = new ListInfo(message.getString("id"), message.getString("title")); > + > + // Do something to update the set of list pages. Is the idea here just to notify anyone currently visible (a settings page or the home pager) that new lists are available? Do we even need to pass up the new data? ::: mobile/android/modules/Home.jsm @@ +138,5 @@ > }; > > +function List(options) { > + if ("id" in options) > + this.id = options.id; Do we want to autogenerate id's. I guess these need to be fairly static, so no :) @@ +153,5 @@ > + > + let prefValue = this._sharedPrefs.getCharPref(this.PREF_KEY); > + if (!prefValue) { > + return; > + } No braces unless rnewman is reviewing :) @@ +165,5 @@ > +HomeLists.prototype = { > + add: function(options) { > + let list = new List(options); > + if (!list.id || !list.title) { > + Cu.reportError("Can't create a home list without an id and title!"); I would rather throw in these cases so that callers have some indication things failed. Either that, or we return false?

(In reply to Mark Finkle (:mfinkle) from comment #37) > (In reply to :Margaret Leibovic from comment #36) > > (In reply to Mark Finkle (:mfinkle) from comment #35) > > > Out of curiosity, what is PROVIDER_ID supposed to do? I am assuming it is a > > > way for the add-on to specify the data in the DB that belongs to it. Is that > > > right? If so, I don't think an integer is the way to go. All add-ons would > > > just use the same value. > > > > It's a string in this patch, right? PROVIDER_ID is supposed to correspond to > > the unique id that the add-on passes in Home.list.add(). The current plan is > > to store the list items for all add-ons in the same table in the DB, but > > then we can filter on PROVIDER_ID to return the ones we want for the given > > page. > > > I was thrown by this in the patch in bug 941318: > > HomeListItems.PROVIDER_ID + " INTEGER," + > > But then I realized you are not creating the DB. Still might want to fix > that. Oh, good catch. Yeah, that's a mistake!

(In reply to Wesley Johnston (:wesj) from comment #38) > Comment on attachment 8349602 [details] [diff] [review] > patch > > Review of attachment 8349602 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the idea of getting this stuff in and iterating on it in tree. It > might be good to have a backout plan for when 29 hits aurora (if it won't be > too hard...), just to be safe. I think we should only worry about backing out if this causes some sort of regression, but I think our "backout"/"disable" plan should just be to not advertise that we support add-ons using this API. > ::: mobile/android/base/home/HomeConfig.java > @@ +199,5 @@ > > > > public static HomeConfig getDefault(Context context) { > > return new HomeConfig(new HomeConfigMemBackend(context)); > > } > > +} > > No op change here? Adding a new line at the end of the file (I know it's out of scope for this patch, but it ended up in my patch as I was working and figured it would be fine to just fix it here). > ::: mobile/android/base/home/HomeConfigMemBackend.java > @@ +16,5 @@ > > +import org.mozilla.gecko.util.UiAsyncTask; > > + > > +import org.json.JSONArray; > > +import org.json.JSONException; > > +import org.json.JSONObject; > > Bunch of not used stuff here > > ::: mobile/android/base/home/ListManager.java > @@ +4,5 @@ > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +package org.mozilla.gecko.home; > > + > > +import org.mozilla.gecko.GeckoApp; > > Same with unused imports here. > > @@ +34,5 @@ > > + public ListInfo(String id, String title) { > > + this.id = id; > > + this.title = title; > > + } > > + } > > Is it worth having this separate class and not reusing PageEntry here? I > read through the bug a bit, and I'm having trouble remembering the > difference between a list that's registered and a page that's registered. I > assume that would help me :) I was aiming to keep the concepts of lists and pages separate for the sake of modularity. Right now ListManager doesn't know anything about pages, but yeah, this ListInfo type basically has exactly the same data as PageEntry (except a type, although we do want to support different types of lists). I don't have a strong opinion one way or the other, but as our code complexity grows, it might be nice to have this layer of abstraction. > @@ +46,5 @@ > > + GeckoAppShell.getEventDispatcher().registerEventListener("HomeLists:Added", this); > > + } > > + > > + /** > > + * Reads list info from SharedPreferences. Don't call this on the main thread! > > I wonder if we should add an assert for these types of conditions and just > use it. > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/ > ThreadUtils.java#116 Good idea. > @@ +63,5 @@ > > + final JSONArray lists = new JSONArray(prefValue); > > + for (int i = 0; i < lists.length(); i++) { > > + final JSONObject list = lists.getJSONObject(i); > > + final ListInfo info = new ListInfo(list.getString("id"), list.getString("title")); > > + listInfos.add(info); > > I wonder if its worth wrapping this in a separate try-catch so that one bad > list doesn't take down the rest.... Really nothing in here should ever throw, since we're the ones creating the JSON that we're sending over, and we're verifying that a list must have a valid id and title on the JS side. > @@ +80,5 @@ > > + public void handleMessage(String event, JSONObject message) { > > + try { > > + final ListInfo info = new ListInfo(message.getString("id"), message.getString("title")); > > + > > + // Do something to update the set of list pages. > > Is the idea here just to notify anyone currently visible (a settings page or > the home pager) that new lists are available? Do we even need to pass up the > new data? I didn't totally think through what the best solution is here... if we just don't pass the new data, we'll need to re-read all the lists from shared preferences, but logic-wise I suppose it would be simpler to just say "hey, new list is here, let's re-load the home config". The HomeConfig APIs are definitely still in flux, but lucasr did stub out some listener methods that we should be able to use to notify consumers when the HomeConfigBackend storage changes. Maybe this handleMessage call here would notify the HomeConfigBackend to just add this new list to its data/storage model, then the backed can take care of notifying the rest of the world that it changed. > ::: mobile/android/modules/Home.jsm > @@ +138,5 @@ > > }; > > > > +function List(options) { > > + if ("id" in options) > > + this.id = options.id; > > Do we want to autogenerate id's. I guess these need to be fairly static, so > no :) Yeah, at one point I had a version auto-generating ids, but that doesn't work for bootstrapped add-ons, since we need them to use the same id between app restarts. However, I think we should encourage add-ons to use a guid, or maybe something else that should be unique, like their add-on id. > @@ +153,5 @@ > > + > > + let prefValue = this._sharedPrefs.getCharPref(this.PREF_KEY); > > + if (!prefValue) { > > + return; > > + } > > No braces unless rnewman is reviewing :) Braces 4eva!!! I've actually become a fan of single-line braces for the sake of consistency. > @@ +165,5 @@ > > +HomeLists.prototype = { > > + add: function(options) { > > + let list = new List(options); > > + if (!list.id || !list.title) { > > + Cu.reportError("Can't create a home list without an id and title!"); > > I would rather throw in these cases so that callers have some indication > things failed. Either that, or we return false? Throwing sounds good, I can update it to do that.

(In reply to :Margaret Leibovic from comment #32) > (In reply to Lucas Rocha (:lucasr) from comment #30) > > Comment on attachment 8345456 [details] [diff] [review] > > WIP v3 (reduced scope) > > > > Review of attachment 8345456 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Drive-by: > > > > This patch seems to mix the notion of 'available lists' with the notion of > > 'the lists displayed in home pager'. They are separate things. IMO, > > ListManager should only be about fetching the list of available lists to be > > added to HomePager i.e. it would do the "Get the list of addon-based lists" > > part from my 'Notes on settings UI...' in bug 942231. > > This raises a question about how we want to manage third-party lists. When > starting out here, I assumed that having an add-on installed that added a > list would always add a list to about:home, and a user would remove that > list by un-installing the add-on. I imagined that we would get a list of > services to populate the "Add a new list" dialog from somewhere like a.m.o. > (or just some hard-coded list we ship), then adding that new list would be > equivalent to installing an add-on. > > I feel like this approach is simpler because we won't have to worry about > putting a list in some "disabled" state, but perhaps it is better to include > this state so that users can choose to disable some of the built-in lists > that we ship (bookmarks, reading list, etc). I like the simplicity of this approach but it kinda forces a 1-1 match between add-ons and lists which can become a little inconvenient for both users and add-on developers. Suppose SomeNewsService wants to provide a few types of lists such as Politics, Technology, etc. With your suggested approach, SomeNewsService would have to publish to separate add-on for each type of content in order to allow users to pick only the ones they want in about:home (unless I'm misunderstanding your explanation above?). The model I had in mind was that enabled add-ons would be able to 'export' one or more lists to Fennec and the user would pick the ones they want from the Settings UI, directly from about:home, etc. The drawback of this approach is the maybe-not-so-obvious distinction between installing an add-on and adding lists to about:home. We can mitigate that by: 1. Auto-adding a list if the installed add-on only exports one (mimicking your approach for the most common case, at installation time) 2. Letting the user know when add-ons export multiple lists and where to find them in the UI. Even in case 2, for add-ons that export more than one list, we could let them define a primary/default one to be auto-added to about:home when the add-on is installed as a way to ensure that there's always something new in about:home when you install a new list add-on. The bottom line is: I think it's important to make a distinction between 'available lists' and 'lists currently displayed in about:home'. Whether we auto-add newly installed lists to about:home or not is a separate UX aspect. It would be nice to get some input from ibarlow on this. > If we do support this "disabled" state, I think we should make sure these > add-ons won't be syncing any data into the content provider, and maybe we > should even delete their current data set when they're disabled (don't want > to take up user's data space with stuff they don't use). Yep. > > For instance, I'd remove any reference to SharedPreferences here. This is > > probably going to be an implementation detail of our HomeConfigBackend to > > store the current HomePager configuration (if we decide to take the > > SharedPreferences route). > > Well, we chose to go with SharedPreferences here because it's a storage that > both Java/Gecko can read/write. The ListManager will need to know about what > lists are available before Gecko starts up, but Gecko is also going to need > to know about those lists once it's up and running. IMO, Gecko shouldn't need to know anything about how the about:home config is stored. I'd prefer to keep all the storage logic in one place (our HomeConfigBackend) to avoid confusion and duplication. Gecko should just send messages to Java if it wants to display something new in about:home. > > More specifically, I'd expect the ListManager to > > only be used by the 'List chooser' UI (see mockup in bug 942875). > > > > The way I see this, the flow would *roughly* be like: > > - User opens 'Home page lists' settings UI > > + The settings UI will use a HomeConfigLoader to get the current > > configuration to display. > > + We trigger a HomeConfig.save() call off main thread every time the user > > changes the order of the lists, or settings a different default list, etc. > > + This will trigger the OnChangeListener in our HomeConfigBackend and cause > > HomePager to refresh its contents if it is visible in background. > > - User taps on 'Add list' > > + The 'Add list' UI would be triggered with something like > > startActivityForResult() > > + ListManager fetches the list of available addon-based lists > > - User taps on one of the available lists > > + The 'Add list' UI creates a PageEntry instance based on the selected item > > and passes that as a result to the 'Home page lists' UI. > > + The 'Home page lists' UI gets the chosen PageEntry, adds to the list, and > > triggers a HomeConfig.save() off main thread. > > + This will trigger the OnChangeListener in our HomeConfigBackend and cause > > HomePager to refresh its contents if it is visible in background. > > > > I'd probably rename ListManager to something like AddonListManager for > > clarity. > > It seems to me like this flow assumes that new lists are only added by the > user through this dialog, but I assumed that add-ons would also be able to > just automatically add new lists themselves. Once again, this comes back to > the assumption that adding/removing a list would be implemented by > installing/uninstalling an add-on. > > Seems like we need to have a bit more discussion about how we all see this > add-on implementation working. Yep, let's discuss this today.