783312 - Make top sites customizable on about:home

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

As part of our customization / quick comfortable access to stuff stories, we should enable more user control over the content that is displayed on about:home. 

By default, the top sites thumbnails should match to the highest ranked items in the awesomebar. But in addition, we should build on this by allowing users to

P1
- Add specific sites to the start page
- Remove sites from the start page
- Pin sites to a specific position

P2
- Decide how many sites to show on the start page (4, 6, 8, 10 etc)
- Customize top site layout grid? (2x2, 3x3 etc)

Given this addition of customization, I wonder it might also make sense to remove or change the "Top Sites" title to something else.

UX flows / mocks to come soon.

Comment 1

[Wesley Johnston (:wesj)]

I'm implementing something that is basically like:

http://cl.ly/image/1F1R2J1W1d1c

I have a basic framework working. I'll try to get a WIP patch up today. Lots of pieces that I need to pick apart into some easy to review bugs, but I like keeping things together while I'm still working out how to make things happen.

There are some open questions about what things like "Delete" do on sites that not pinned (does it remove them from history or just hide them from top sites somehow?) and does "Clear All Sites" remove everything from about:home or just pinned items.

Comment 2

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #1)

> There are some open questions about what things like "Delete" do on sites
> that not pinned (does it remove them from history or just hide them from top
> sites somehow?) and does "Clear All Sites" remove everything from about:home
> or just pinned items.

You should see what desktop did for the new tab page (you can probably ask ttaubert), since this is really similar. I think it would make sense for these pages to behave consistently.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Just saving my place. This shows placeholders for empty spaces, and has basic support for pinning/editing. CursorWrapper occasionally seems to mess up. Working on fixing that and adding moving.

If the user adds a site that's already present in top sites, probably should only show it once too. Lots of little edge cases to work around.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: WIP v2

Another WIP. This has drag and drop reorganizing (doesn't work for the first item for some reason). Still a few features that need dealing with and plenty of paper cuts, but a build for UX is at:

https://dl.dropbox.com/u/72157/fennec-customize.apk

Comment 5

[Ian Barlow (:ibarlow)]

Thanks for the build Wes. Having played with it a bit, it's easy to see that this is going to be an *incredibly* useful feature. I've been sitting here obsessing about just what sites I want to see on the home page, and having that feeling of control is pretty great.

I have a (partial) list of comments so far on the build from comment 4. Some paper cuts I've noticed, and some rules I hadn't considered but that might make sense now that I have tried this in real life:

----------

Errors/papercuts

- After using this for a while, it seems like the awesomebar algorithm has a fairly negative impact on the arrangement of Top Sites tiles. Every time I come back to about:home everything is in a different spot. I realize we had talked about pinning, but I actually think we just need to decouple the start page from the awesomebar list, and allow the about:home tiles to be controlled exclusively by the user. (They can still access the awesomebar list through the "new" or "edit" commands)
- Can't always tap on thumbnails
- Can't always long press on thumbnails
- Page thumbnails don't appear reliably after visiting a site
- Defining a new tile sometimes creates two thumbnails 
- UX can provide some styling guidelines for the "Edit" context menu, and the long press menu

----------

Long press menu

*Edit*
When editing an existing thumbnail, display entire awesomebar list. Right now the list gets filtered down to the current URL. A simple way to fix this could be by adding a clear field element to the url field.

*Move*
This might work better as a modal experience, where you can rearrange things at your leisure and then click "done". Moving one thumbnail at a time feels a little cumbersone.

*Delete*
Rename to "Clear"
Should clear thumbnail and leave empty box with + symbol

*Clear All*
Should remove all titles and thumbnails, leaving 6 empty boxes with + symbols

----------

As an aside, the new overscroll colour for websites is brilliant.

Comment 6

[Wesley Johnston (:wesj)]

A quick status update:

I've got this running now where you're either in a top-sites mode or a pinned mode, not mixed. As soon as you pin a site, I pin all of the other current top sites for you. It uses a special hidden bookmark folder and some url trickery to store them. I also have the clear button in the textbox up, favicons, and a bunch of other tweaks. I'll try to get a build together tomorrow.

In another patch I've also got a slightly different UI where we use an Action Mode on Honeycomb and ICS. I wonder if you'd be willing to have different experiences on those devices ian? Getting action modes working on Gingerbread will be a bit more work.

That gives us the more "modal" move around your sites experience. I figured I would show "Edit" and "Remove" in the actionbar, "Edit" disabled when you have more than one site selected. Gingerbread also doesn't support the drag & drop stuff I was using that gives us nice pretty easy drag images either, so I'm putting together a fallback there.

Comment 7

[Lucas Rocha (:lucasr)]

Wondering: shouldn't we to implement this using a contextual action bar? Long tapping on top sites would trigger action mode with available options. Ian, what do you think?

For reference:
http://developer.android.com/guide/topics/ui/menus.html#CAB

Comment 8

[Wesley Johnston (:wesj)]

To  be clear, that's what I meant by "Action Mode".

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #6)

> I've got this running now where you're either in a top-sites mode or a
> pinned mode, not mixed. As soon as you pin a site, I pin all of the other
> current top sites for you. It uses a special hidden bookmark folder and some
> url trickery to store them. I also have the clear button in the textbox up,
> favicons, and a bunch of other tweaks. I'll try to get a build together
> tomorrow.

Is there a way to blend this one step further, where you can pin sites and those stay fixed in their position, but that your other unpinned top sites can change based on frecency? This would also mean that clearing or unpinning a page would change it back to a frecency tile. 


> In another patch I've also got a slightly different UI where we use an
> Action Mode on Honeycomb and ICS. I wonder if you'd be willing to have
> different experiences on those devices ian? Getting action modes working on
> Gingerbread will be a bit more work.

Yes, I wouldn't try to roll the action bar back to GB. Let's use it on more modern versions of Android, and we'll probably want to stick to floating menus on GB.

Comment 10

[Wesley Johnston (:wesj)]

Here's a build with my latest changes. This won't work at all on pre-Honeycomb devices, but wanted to get UX feedback:

https://dl.dropbox.com/u/72157/fennec-customize.apk

I'm curious if the interaction makes sense, so I'll avoid saying much about it and instead let it delight you with surprises.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP Patch 1/3

Three of these, but they aren't separated into pieces. just saving my place.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: WIP 2/3

Comment 13

[Wesley Johnston (:wesj)]

Attachment: WIP 3/3

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 1/5

I'm not quite done with this, but wanted to get started on reviews. These initial patches just set up the empty rows, a pinning backend, and the edit dialog.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5

Oops. That first patch just creates a cursor wrapper so that we'll always have 6/9 entries in the grid. Empty entries show up with a + sign an no title.

This patch adds a backend for pinning sites using a bookmark folder.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5

And this lets us use that backend. We have to do some trickery to make all this play out.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5

This turns on the edit dialog when you click empty rows.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

This was mostly an experiment to use a SearchView in the edit dialog instead of a custom EditText/button. There are issues here where SearchView's don't actually support textColor and stuff that I hacked around. I... don't really think this is worth the extra complexity it adds.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 686152 [details] [diff] [review]
Patch 1/5

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

Looks good.

::: mobile/android/base/AboutHomeContent.java
@@ +797,5 @@
> +            if (TextUtils.isEmpty(title)) {
> +                titleView.setVisibility(View.GONE);
> +            } else {
> +                titleView.setVisibility(View.VISIBLE);
> +            }

nits: add empty line here. Maybe the {}'s are not needed?

@@ +819,5 @@
> +     * will return empty strings and zero.
> +     */
> +    private class TopSitesCursorWrapper extends CursorWrapper {
> +        int mIndex = -1;
> +        Cursor mCursor = null;

nit: add empty line here.

@@ +825,5 @@
> +            super(cursor);
> +            mCursor = cursor;
> +        }
> +
> +        public int getPosition() { return mIndex; }

nits: This is a bit inconsistent with our coding style. We always indent methods the same way, even if they are one-liners.

@@ +833,5 @@
> +        public boolean isLast() { return mIndex == mNumberOfTopSites - 1; }
> +        public boolean moveToNext() { return moveToPosition(mIndex + 1); }
> +        public boolean moveToPrevious() { return moveToPosition(mIndex - 1); }
> +
> +        public boolean moveToPosition(int position) {

You probably want to add the @Override annotation to all overridden methods from CursorWrapper.

@@ +835,5 @@
> +        public boolean moveToPrevious() { return moveToPosition(mIndex - 1); }
> +
> +        public boolean moveToPosition(int position) {
> +            mIndex = position;
> +            if (position > -1 && position < mCursor.getCount())

Isn't -1 is a valid position for mCursor? I wonder if you should just check for position < mCursor.getCount() here.

@@ +836,5 @@
> +
> +        public boolean moveToPosition(int position) {
> +            mIndex = position;
> +            if (position > -1 && position < mCursor.getCount())
> +                super.moveToPosition(position);

nit: add empty line here.

@@ +842,5 @@
> +        }
> +
> +        public long getLong(int columnIndex) {
> +            if (mIndex > -1 && mIndex < mCursor.getCount())
> +                return super.getLong(columnIndex);

nit: add empty line here.

@@ +848,5 @@
> +        }
> +
> +        public String getString(int columnIndex) {
> +            if (mIndex > -1 && mIndex < mCursor.getCount())
> +                return super.getString(columnIndex);

nit: add empty line here.

Comment 20

[Lucas Rocha (:lucasr)]

Just wondering: maybe instead of checking is url or title are null everywhere you could add a method to your CursorWrapper like isEmptySpot() that encapsulates that logic for you? Would probably make the code easier to understand at first glance.

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 686153 [details] [diff] [review]
Patch 2/5

I meant to put the rest of these up for review/feedback, but forgot. I know there are bugs around some of this. I'm getting overwhelmed by this enough that I think its better to start reviews here though and put up additional patches with fixes that I find.

This is the backend using a bookmark folder. I think we may need to batch/cache this to do a better job. Feedback welcome.

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 686154 [details] [diff] [review]
Patch 3/5

This uses the backend to fill in sites. This is where I'm a bit worried about the caching aspects. i.e. I query on each slot "Is this pinned?". Performance seems fine on all my devices, but we can be smarter. I think this is a good start.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 686155 [details] [diff] [review]
Patch 4/5

This sets up the edit dialog that gets shown when you click an empty slot. I'll post a screenshot of this tomorrow for UX to see.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 6/5 - Drag and drop

This adds basic support for DND sites on HC+ using DND API's. Its fairly smart and doesn't actually update the pinned sites until you're done in edit mode. Depends on the ActionMode stuff in bug 817125.

There are still a few quirks to work out here as well with the animations.

Comment 25

[Ian Barlow (:ibarlow)]

I still owe interaction feedback, sorry.

I did want to ask though -- is there anything we can do to make the action mode feel a bit more integrated with the Firefox UI? Like adding a pale blue background, and maybe a different accent colour?

Comment 26

[Wesley Johnston (:wesj)]

Heh. Yes! What you saw was all native. We're implementing something custom for GB though, and we can use it on newer android's. Currently I just use a few images I took from the Android source.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Show edit dialog

Fixed some issues on Gingerbread. Added a custom layout for the rows and moved all the strings to be localizable.

Comment 28

[Wesley Johnston (:wesj)]

Dang. Missed one "Enter search or Address" string. Fixed locally.

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Patch 6/5 - Provide a simple context menu

Talked to mfinkle. The drag and drop stuff is what's causing me headaches here. I think the simplest way to move forward is to just provide a context menu for this first round, and move DND to a follow up bug. This does that.

I'll reupload everything to make sure it all applies, but leave them separate for now. If you'd rather I fold them together, I can do that.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #29)

> Talked to mfinkle. The drag and drop stuff is what's causing me headaches
> here. I think the simplest way to move forward is to just provide a context
> menu for this first round, and move DND to a follow up bug. This does that.

Just to followup. I'd like to get the bulk of this feature landed so Wes does not need to keep unbitrotting code. We should be able to back anything out or disable, if we can't get it all landed for Fx20 - before the merge.

Comment 31

[Wesley Johnston (:wesj)]

Attachment: Patch7/5 - backend part 2

This simplifies the backend a lot and moves all the db calls to a bg thread. It also hides the Clear All menu item if nothing is pinned.

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Patch 8/5 - Keep thumbnails for pinned sites

This keeps thumbnails for pinned sites.

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5 - Backend part 1

This is the backend patch again. Just want to make sure its up to date. This is changed heavily in the backend part 2 patch.

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5 - Use backend

This uses the backend. This is heavily changed in patch 7/5 which moves all the db calls to be async.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Show edit dialog

This implements the edit dialog bits.

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Use a SearchView on ICS

This is the searchview stuff I don't think we want to use.

Comment 37

[Lucas Rocha (:lucasr)]

Comment on attachment 689484 [details] [diff] [review]
Patch 2/5 - Backend part 1

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

Looks good but I'd to see new/cleaner version of this patch before giving r+.

::: mobile/android/base/db/BrowserDB.java
@@ +97,5 @@
>          public int getCount(ContentResolver cr, String database);
> +
> +        public void pinSite(ContentResolver cr, String url, String title, int position);
> +        public void unpinSite(ContentResolver cr, int position);
> +        public void unpinSites(ContentResolver cr);

unpinAllSites?

@@ +102,5 @@
> +        public boolean hasPinnedSites(ContentResolver cr);
> +        public Cursor getPinnedSites(ContentResolver cr, int limit);
> +        public Cursor getPinnedSite(ContentResolver cr, int position);
> +        public int isPinned(ContentResolver cr, String url);
> +        public int isPinned(ContentResolver cr, int position);

Checking if a position is pinned sounds a bit weird :-)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1088,5 @@
> +
> +    public void pinSite(ContentResolver cr, String url, String title, int position) {
> +        ContentValues values = new ContentValues();
> +        final long now = System.currentTimeMillis();
> +        values.put(Browser.BookmarkColumns.TITLE, title);

Just Bookmarks.TITLE is enough, no?

@@ +1100,5 @@
> +                  Bookmarks.PARENT + " == ? AND " + Bookmarks.URL + " == ?",
> +                  new String[] {
> +                      String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID),
> +                      url
> +                  });

Why do you need to delete first? What happens if there's already a pinned site for this position?

@@ +1116,5 @@
> +    }
> +
> +    public Cursor getPinnedSites(ContentResolver cr, int limit) {
> +        return cr.query(bookmarksUriWithLimit(limit),
> +                           new String[] { Combined._ID,

nit: slightly off indentation

@@ +1117,5 @@
> +
> +    public Cursor getPinnedSites(ContentResolver cr, int limit) {
> +        return cr.query(bookmarksUriWithLimit(limit),
> +                           new String[] { Combined._ID,
> +                                          Combined.URL,

If you're querying the bookmarks uri, you should use Bookmarks.* columns.

@@ +1127,5 @@
> +    }
> +
> +    public Cursor getPinnedSite(ContentResolver cr, int position) {
> +        return cr.query(bookmarksUriWithLimit(1),
> +                           new String[] { Combined._ID,

nit: slightly off indentation here.

@@ +1128,5 @@
> +
> +    public Cursor getPinnedSite(ContentResolver cr, int position) {
> +        return cr.query(bookmarksUriWithLimit(1),
> +                           new String[] { Combined._ID,
> +                                          Combined.URL,

Same here, use Bookmarks.* whenever you're querying with the bookmarks uri.

@@ +1160,5 @@
> +                             new String[] { Combined.URL },
> +                             Bookmarks.PARENT + " == ?",
> +                             new String[] { String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) },
> +                             null);
> +        boolean ret = c != null && c.getCount() > 0;

Use the finally idiom to close the cursor here.

@@ +1179,5 @@
> +        Cursor c = null;
> +        int pinned = -1;
> +        try {
> +            c = cr.query(bookmarksUriWithLimit(1),
> +                                 new String[] { Bookmarks.POSITION },

nit: indentation is a bit off here.

@@ +1180,5 @@
> +        int pinned = -1;
> +        try {
> +            c = cr.query(bookmarksUriWithLimit(1),
> +                                 new String[] { Bookmarks.POSITION },
> +                                 Bookmarks.PARENT + " == ? " + (where != null ? "AND " + where : ""),

Use DBUtils.concatenateWhere() here?

@@ +1185,5 @@
> +                                 new String[] { String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID),
> +                                                bindArg },
> +                                 null);
> +            if (c != null && c.getCount() > 0) {
> +                c.moveToFirst();

"if (c != null && c.moveToFirst())" would be simpler.

@@ +1191,5 @@
> +            }
> +        } finally {
> +            if (c != null)
> +                c.close();
> +        }

nit: add empty line here.

@@ +1195,5 @@
> +        }
> +        return pinned;
> +    }
> +
> +    public int getPinnedBefore(ContentResolver cr, int position) {

Maybe getPinnedSitesBefore would be more obvious.

@@ +1209,5 @@
> +            count = c.getCount();
> +        } finally {
> +            if (c != null)
> +                c.close();
> +        }

nit: add empty line here.

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 689485 [details] [diff] [review]
Patch 3/5 - Use backend

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

I feel that the approach for loading of topsites should be more batch-oriented than per-spot oriented. Maybe load all the topsites/pinnedsites from DB in one task and load thumbnails just after (in a separate task)? This would allow you to execute all these queries for pinned sites directly instead of running an asynctask for each spot in about:home.

::: mobile/android/base/AboutHomeContent.java
@@ -89,5 @@
>      }
>  
>      private Context mContext;
>      private BrowserApp mActivity;
> -    private Cursor mCursor;

How is this related?

@@ +157,5 @@
>          mTopSitesGrid = (TopSitesGridView)findViewById(R.id.top_sites_grid);
>          mTopSitesGrid.setOnItemClickListener(new AdapterView.OnItemClickListener() {
>              public void onItemClick(AdapterView<?> parent, View v, int position, long id) {
> +                TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();
> +                String spec = holder.url;

How is this related?

@@ -277,4 @@
>                  }
>  
> -                if (mTopSitesAdapter.getCount() > 0)
> -                    loadTopSitesThumbnails(resolver);

I assume this is not necessary anymore because TopSitesCursorWrapper always has count > 0?

@@ +751,5 @@
>              // our history database is updated.
>              return;
>          }
>  
> +        private PinnedSite getPinnedSite(int position) {

This is running on main thread, no? You should run this in a background thread.

@@ +785,2 @@
>              } else {
> +                viewHolder = (TopSitesViewHolder)convertView.getTag();

nit: add space between casting paren and convertView.getTag()

@@ +802,5 @@
> +                    }
> +        
> +                    @Override
> +                    public void onPostExecute(byte[] bitmap) {
> +                        updateTopSitesThumbnail(v, bitmap);

The original approach for loading thumbnails had the advantage of touching the disk only once for loading the images? Why do you feel this change is better? I'm fine with changing this as long as there's a good reason for it.

@@ +828,5 @@
> +            } else {
> +                // If this site isn't pinned, check if we should be showing an empty slot
> +                // or a top site result, by looking at the slotnumber - the number of pinned sites before this slot
> +                final ContentResolver resolver = mActivity.getContentResolver();
> +                int before = BrowserDB.getPinnedBefore(resolver, position);

This should not be run on the main thread.

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 689487 [details] [diff] [review]
Patch 4/5 - Show edit dialog

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

I'd like to a new version of this patch with the factored out edit dialog.

::: mobile/android/base/AboutHomeContent.java
@@ +936,5 @@
> +        });
> +    }
> +
> +    MyAdapter mAdapter = null;
> +    View editBox = null;

Declare all  properties at the top of the class, just like the others.

@@ +940,5 @@
> +    View editBox = null;
> +    /* Shows a dialog that lets the user select a site
> +     * Takes a url to use for the initial filter, and a Callback to be called when the dialog returns
> +     */
> +    private void editSite(String url, final Handler.Callback callback) {

This code should probably live in a separate helper class.

@@ +947,5 @@
> +        View innerView = mInflater.inflate(R.layout.abouthome_topsites_edit, null);
> +
> +        // Create a cursor to fill in the listView in the dialog with top/matching sites
> +        Cursor cursor = BrowserDB.filter(mActivity.getContentResolver(), "", 10);
> +        mAdapter = new MyAdapter(mActivity, R.layout.abouthome_topsites_edit_row, cursor,

MyAdapter is a bit generic. AboutHomeEditAdapter?

@@ +973,5 @@
> +                    button.setVisibility(View.GONE);
> +                } else {
> +                    button.setVisibility(View.VISIBLE);
> +                }
> +            }

How does this dialog look? Could you share a test build?

@@ +1039,5 @@
> +                    Bitmap b = BrowserDB.getFaviconForUrl(mActivity.getContentResolver(), url);
> +                    if (b == null) {
> +                        return b;
> +                    }
> +                    return Bitmap.createScaledBitmap(b, sFaviconSize, sFaviconSize, false);

You should probably use the Favicons API here. I guess you'll have to add API to load favicons with a specific size.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +196,5 @@
>  <!ENTITY abouthome_top_sites_title "Top sites">
> +<!ENTITY abouthome_topsites_edit_addsite "Add Site">
> +<!ENTITY abouthome_topsites_edit_ok "OK">
> +<!ENTITY abouthome_topsites_edit_cancel "Cancel">
> +<!ENTITY abouthome_topsites_edit_searchprompt "Enter an address">

It would good to get these strings by UX guys.

::: mobile/android/base/resources/layout/abouthome_topsites_edit.xml
@@ +16,5 @@
> +
> +    </LinearLayout>
> +
> +   <ListView android:id="@+id/top_sites_edit_list"
> +             style="@style/AboutHome.Edit.List"/>

Wouldn't it be better if this entry work more like an autocompletion thingy instead of this listview?

::: mobile/android/base/resources/layout/abouthome_topsites_edit_row.xml
@@ +15,5 @@
> +
> +    <TextView android:id="@+id/url"
> +              android:layout_below="@id/title"
> +              android:layout_toRightOf="@id/favicon"
> +              style="@style/AboutHome.Edit.Row.Url"/>/>

No way to reuse whatever layout we use in awesome screen?

Comment 40

[Lucas Rocha (:lucasr)]

Comment on attachment 689488 [details] [diff] [review]
Patch 5/5 - Use a SearchView on ICS

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

Stepping back a bit. I really think we should try to come up with a design where we just reuse the awesome screen to select pinned sites instead of creating custom UIs (dialog, searchview, etc). This will greatly simplify the code. I feels wrong that we're somehow re-implementing awesome screen here.

Comment 41

[Ian Barlow (:ibarlow)]

Wes, is there an updated build you could post here? I am guessing the one in comment 4 is out of date by now :)

Comment 42

[Lucas Rocha (:lucasr)]

Comment on attachment 689466 [details] [diff] [review]
Patch 6/5 - Provide a simple context menu

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

This is touch disk from the main thread. Needs work.

::: mobile/android/base/AboutHomeContent.java
@@ +202,5 @@
> +                MenuInflater inflater = mActivity.getMenuInflater();
> +                inflater.inflate(R.menu.abouthome_topsites_menu, menu);
> +
> +                final ContentResolver resolver = mActivity.getContentResolver();
> +                Cursor c = BrowserDB.getPinnedSite(resolver, info.position);

Database query in main thread? I guess you'll have to find a way to avoid this.

@@ +724,5 @@
>          onLightweightThemeChanged();
>      }
>  
>      public static class TopSitesGridView extends GridView {
> +        int mSelected = -1;

You don't seem to be actually using this value anywhere. In the next patch I guess? If that's the case, move this change to whatever patch you use this new API for the first time.

@@ +961,5 @@
> +    }
> +
> +    public void clearAllSites() {
> +        final ContentResolver resolver = mActivity.getContentResolver();
> +        // To make this clearer to 

Incomplete comment?

@@ +967,5 @@
> +            View v = mTopSitesGrid.getChildAt(i);
> +            TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();
> +            clearThumbnail(holder);
> +        }
> +        BrowserDB.unpinSites(resolver);

Running DB command in main thread? You have to execute this in a background thread.

@@ +979,5 @@
> +        // quickly update the view so that there isn't as much lag between the request and response
> +        clearThumbnail(holder);
> +
> +        final ContentResolver resolver = mActivity.getContentResolver();
> +        BrowserDB.unpinSite(resolver, position);

Running DB command in main thread? You have to execute this in a background thread.

::: mobile/android/base/BrowserApp.java
@@ +955,5 @@
> +    public boolean onContextItemSelected(MenuItem item) {
> +        switch (item.getItemId()) {
> +            case R.id.abouthome_topsites_edit:
> +                mAboutHomeContent.editSite();
> +                return true;

nit: add empty line here.

@@ +958,5 @@
> +                mAboutHomeContent.editSite();
> +                return true;
> +            case R.id.abouthome_topsites_clear:
> +                mAboutHomeContent.clearSite();
> +                return true;

nit: add empty line here.

@@ +965,5 @@
> +                return true;
> +        }
> +        return false;
> +    }
> +    

nit: remove trailing space here.

::: mobile/android/base/Makefile.in
@@ +1036,5 @@
>    res/menu/gecko_app_menu.xml \
>    res/menu/tabs_menu.xml \
>    res/menu/titlebar_contextmenu.xml \
>    res/menu-v11/tabs_menu.xml \
> +  res/menu/abouthome_topsites_menu.xml \

The menu xml file doesn't seem be included in the patch.

Comment 43

[Lucas Rocha (:lucasr)]

Comment on attachment 689483 [details] [diff] [review]
Patch 8/5 - Keep thumbnails for pinned sites

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

Good.

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch v8

Forgot to qref

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Patch 1/5 - Show empty slots

Combined some stuff to get this back down to 5 patches. Combined the changes that were in backend 2 before with the earlier patches (so everything should be OMT now).

Comment 46

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5 - backend pieces

Greatly simplified

Comment 47

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5 - Use the backend

This causes us to show pinned sites in the frontend. I combined the followup from the previous series so that this basically does one request for a cursor, which the main cursor adapter stores and intermixes with the normal top sites query.

Comment 48

[Wesley Johnston (:wesj)]

Attachment: Path 4/5 - Show edit dialog

This shows the edit dialog when you tap on empty rows. Creates a PickSiteDialog.java component so that we could potentially reuse this somewhere if we want. That was a good idea!

Comment 49

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Show a context menu

This shows a context menu when you long tap on rows. I'll post a screenshot and build is at:

people.mozilla.com/~wjohnston/customize.apk

Comment 50

[Wesley Johnston (:wesj)]

Attachment: Screenshot of the dialog

Comment 51

[Wesley Johnston (:wesj)]

Lucas - I tried to catch all your comments but I wound up jumping around a bit trying to combine patches. Tell me if I missed anything. Some of them like the mSelected = -1 I need to answer (we need some way to store what grid entry brought up the context menu).

Similarly, I wrote a custom awesomebar row view because the current one is designed to show on a white background. On ICS+ I could work around that by forcing a HOLO_LIGHT dialog. On Gingerbread, I couldn't change the theme of GeckoApp (without just changing the theme of GeckoApp). I chose this as a way to make things work.

Comment 52

[Lucas Rocha (:lucasr)]

Comment on attachment 689976 [details] [diff] [review]
Patch 2/5 - backend pieces

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

Questions:
- Would it make sense to add pinned site bits to testBrowserProvider? 
- What's the sync story for pinned sites?

This looks good and clean overall. Just want questions and answered and the isBookmark issue fixed before giving r+.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1083,5 @@
>          public int getColumnIndexOrThrow(String columnName) {
>              return super.getColumnIndexOrThrow(translateColumnName(columnName));
>          }
>      }
> +

You have to change isBookmark() method in LocalBrowserDB to account for pinned items in addition to reading list items. Otherwise pinned items will show up as starred pages.

@@ +1088,5 @@
> +
> +    public void pinSite(ContentResolver cr, String url, String title, int position) {
> +        ContentValues values = new ContentValues();
> +        final long now = System.currentTimeMillis();
> +        values.put(Browser.BookmarkColumns.TITLE, title);

Just Bookmarks.TITLE would be enough here.

@@ +1095,5 @@
> +        values.put(Bookmarks.DATE_MODIFIED, now);
> +        values.put(Bookmarks.POSITION, position);
> +        values.put(Bookmarks.IS_DELETED, 0);
> +
> +        cr.delete(mBookmarksUriWithProfile,

I still don't understand why you need to delete first.

@@ +1111,5 @@
> +                                               String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) });
> +
> +        if (updated == 0) {
> +            cr.insert(mBookmarksUriWithProfile, values);
> +        }

So, there's no need to handle multiple items on the same position? Wouldn't this make the sorting of pinned items a bit unpredictable?

Comment 53

[Lucas Rocha (:lucasr)]

Comment on attachment 689977 [details] [diff] [review]
Patch 3/5 - Use the backend

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

Looking good overall but the adapter code needs some improvements. I'm a bit confused about how the top and pinned sites are loaded. My impression is that they should be loaded all at once (in getTopSites() I guess) and AboutHomeContent should only care about the wrapped cursor with some empty spots.

::: mobile/android/base/AboutHomeContent.java
@@ +157,5 @@
>  
>          mTopSitesGrid = (TopSitesGridView)findViewById(R.id.top_sites_grid);
>          mTopSitesGrid.setOnItemClickListener(new AdapterView.OnItemClickListener() {
>              public void onItemClick(AdapterView<?> parent, View v, int position, long id) {
> +                TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();

nit: add space between casting and v.getTag()

@@ +209,5 @@
> +        if (cursor != null && !cursor.isClosed())
> +            cursor.close();
> +
> +        // close the pinned sites cursor
> +        ((TopSitesCursorAdapter)mTopSitesAdapter).setPinnedSites(null);

Any reason to not simply change the type of mTopSiteAdapter to TopSitesCursorAdapter?

@@ +285,4 @@
>                      mTopSitesGrid.setAdapter(mTopSitesAdapter);
>                  } else {
> +                    oldCursor = mTopSitesAdapter.getCursor();
> +                    mTopSitesAdapter.changeCursor(cursor);

Won't this first refresh the UI with the top sites then get the pinned sites later? Isn't this flickering a bit on refresh? My impression is that the logic for blending top sites with pinned sites should be hidden behind getTopSites somehow. This probably means moving TopSitesCursorWrapper to LocalBrowserDB.

This way you'd only need to care about empty spots here and there.

@@ +289,1 @@
>                  }

nit: add empty line here.

@@ +326,5 @@
> +        List<String> urls = new ArrayList<String>();
> +
> +        for (int i = 0; i < mTopSitesGrid.getChildCount(); i++) {
> +            View view = mTopSitesGrid.getChildAt(i);
> +            TopSitesViewHolder holder = (TopSitesViewHolder)view.getTag();

nit: add space between casting and view.getTag()

@@ +357,5 @@
>  
>          mTopSitesGrid.invalidate();
>      }
>  
> +    public Map<String, Bitmap> getThumbnailsFromCursor(Cursor c) {

Renaming this seems unrelated btw.

@@ +762,5 @@
>              super.onMeasure(widthMeasureSpec, heightMeasureSpec);
>          }
>      }
>  
> +    /* Simple view holder to store the url stored at this position

No need for this comment I guess. View holders are pretty much self-explanatory.

@@ +830,4 @@
>              }
>  
> +            // walk throught he cursor and look for items that match this position
> +            synchronized (mPinnedSites) {

Isn't it possible to get null mPinnedSites in some cases (e.g. just after destroying the activity, while pinned sites haven't been loaded yet)?

@@ +851,5 @@
> +            if (!hasPinnedSites()) {
> +                return numFound;
> +            }
> +
> +            synchronized (mPinnedSites) {

Same comment about possible null mPinnedSites here.

@@ +879,5 @@
> +
> +            viewHolder.titleView.setVisibility(TextUtils.isEmpty(title) ? View.INVISIBLE : View.VISIBLE);
> +            viewHolder.titleView.setText(title);
> +            viewHolder.url = url;
> +            convertView.setTag(viewHolder);

This setTag() call is only be necessary when you first inflate the view (i.e. when convertView is null). Move it up inside the if.

@@ +881,5 @@
> +            viewHolder.titleView.setText(title);
> +            viewHolder.url = url;
> +            convertView.setTag(viewHolder);
> +
> +            convertView.setLayoutParams(new AbsListView.LayoutParams(mTopSitesGrid.getColumnWidth(),

I feel like this explicit layout param tweak deserves a comment.

@@ +882,5 @@
> +            viewHolder.url = url;
> +            convertView.setTag(viewHolder);
> +
> +            convertView.setLayoutParams(new AbsListView.LayoutParams(mTopSitesGrid.getColumnWidth(),
> +                        Math.round(mTopSitesGrid.getColumnWidth()*ThumbnailHelper.THUMBNAIL_ASPECT_RATIO)));

nit: add empty line here.

@@ +891,5 @@
> +        public View getView(int position, View convertView, ViewGroup parent) {
> +            String url = "";
> +            String title = "";
> +
> +            PinnedSite site = getPinnedSite(position);

This looks a bit inefficient. getView() are called tons of times in AdapterViews and you're going through the list of pinned times using a cursor every time it gets called. Given that the list of pinned sites is guaranteed to be relatively small (up to 10 items?) maybe you should simply keep them in memory in, say, SparseArray (using position as key) for quick retrieval here instead using a cursor. That would also avoid the extra cursor just for pinned sites that needs to get explicitly closed later.

@@ +897,5 @@
> +                url = site.url;
> +                title = site.title;
> +            } else {
> +                // If this site isn't pinned, check if we should be showing an empty slot
> +                // or a top site result, by looking at the slotnumber - the number of pinned sites before this slot

Just wondering: shouldn't this logic be hidden behind LocalBrowserDB somehow? I mean, you'd get a cursor with some empty spots, some pinned items and some top sites. Then you'd only need to handle these simple cases here.

@@ +899,5 @@
> +            } else {
> +                // If this site isn't pinned, check if we should be showing an empty slot
> +                // or a top site result, by looking at the slotnumber - the number of pinned sites before this slot
> +                final ContentResolver resolver = mActivity.getContentResolver();
> +                int before = getPinnedBefore(position);

This is going through the list of pinned sites again in getView().

::: mobile/android/base/db/LocalBrowserDB.java
@@ +142,5 @@
>      private Cursor filterAllSites(ContentResolver cr, String[] projection, CharSequence constraint,
>              int limit, CharSequence urlFilter) {
>          String selection = "";
>          String[] selectionArgs = null;
> +        return filterAllSites(cr, projection, constraint, limit, urlFilter, selection, selectionArgs);

Just use literal values directly here instead of declaring these variables?

@@ +148,3 @@
>  
> +    private Cursor filterAllSites(ContentResolver cr, String[] projection, CharSequence constraint,
> +            int limit, CharSequence urlFilter, String selection, String[] selectionArgs) {

filterAllSites look different in latest central. Need to be rebase I guess?

@@ +228,5 @@
>      }
>  
>      public Cursor getTopSites(ContentResolver cr, int limit) {
> +        // Filter out sites that are pinned
> +        String selection = "";

Why creating a empty string to then concatenate the final value later? I guess you just assign the final value directly here?

@@ +229,5 @@
>  
>      public Cursor getTopSites(ContentResolver cr, int limit) {
> +        // Filter out sites that are pinned
> +        String selection = "";
> +        String[] selectionArgs = new String[0];

Ditto.

Comment 54

[Lucas Rocha (:lucasr)]

Comment on attachment 689979 [details] [diff] [review]
Patch 5/5 - Show a context menu

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

Looks good overall but the blocking calls on main thread are no-no.

::: mobile/android/base/AboutHomeContent.java
@@ +189,5 @@
>          });
>  
> +        mTopSitesGrid.setOnCreateContextMenuListener(new View.OnCreateContextMenuListener() {
> +            public void onCreateContextMenu(ContextMenu menu, View v, ContextMenu.ContextMenuInfo menuInfo) {
> +                AdapterView.AdapterContextMenuInfo info = (AdapterView.AdapterContextMenuInfo)menuInfo;

nit: add space between casting and menuInfo

@@ +196,5 @@
> +                MenuInflater inflater = mActivity.getMenuInflater();
> +                inflater.inflate(R.menu.abouthome_topsites_contextmenu, menu);
> +
> +                // if nothing is pinned at all, hide both clear items
> +                if (!((TopSitesCursorAdapter)mTopSitesAdapter).hasPinnedSites()) {

I mentioned that before I think: maybe mTopSitesAdapter should just have type TopSitesCursorAdapter. This way you can avoid these type castings.

@@ +200,5 @@
> +                if (!((TopSitesCursorAdapter)mTopSitesAdapter).hasPinnedSites()) {
> +                    menu.findItem(R.id.abouthome_topsites_clearall).setVisible(false);
> +                    menu.findItem(R.id.abouthome_topsites_clear).setVisible(false);
> +                } else {
> +                    // if there's nothing pinned here, hide the clear item

if -> If

@@ +1022,5 @@
> +
> +        // Clear the view quickly to make things appear responsive
> +        for (int i = 0; i < mTopSitesGrid.getChildCount(); i++) {
> +            View v = mTopSitesGrid.getChildAt(i);
> +            TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();

nit: add space between casting and v.getTag()

@@ +1024,5 @@
> +        for (int i = 0; i < mTopSitesGrid.getChildCount(); i++) {
> +            View v = mTopSitesGrid.getChildAt(i);
> +            TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();
> +            clearThumbnail(holder);
> +        }

nit: add empty line here.

@@ +1025,5 @@
> +            View v = mTopSitesGrid.getChildAt(i);
> +            TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();
> +            clearThumbnail(holder);
> +        }
> +        BrowserDB.unpinAllSites(resolver);

This should not run in main thread.

@@ +1032,5 @@
> +
> +    public void clearSite() {
> +        int position = mTopSitesGrid.getSelectedPosition();
> +        View v = mTopSitesGrid.getChildAt(position);
> +        TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();

nit: add space between casting and v.getTag()

@@ +1033,5 @@
> +    public void clearSite() {
> +        int position = mTopSitesGrid.getSelectedPosition();
> +        View v = mTopSitesGrid.getChildAt(position);
> +        TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();
> +        // quickly update the view so that there isn't as much lag between the request and response

quickly -> Quickly

@@ +1037,5 @@
> +        // quickly update the view so that there isn't as much lag between the request and response
> +        clearThumbnail(holder);
> +
> +        final ContentResolver resolver = mActivity.getContentResolver();
> +        BrowserDB.unpinSite(resolver, position);

This should not run on main thread.

@@ +1044,5 @@
> +    }
> +
> +    public void editSite() {
> +        int position = mTopSitesGrid.getSelectedPosition();
> +        View v = mTopSitesGrid.getChildAt(position);

nit: add empty line here.

@@ +1045,5 @@
> +
> +    public void editSite() {
> +        int position = mTopSitesGrid.getSelectedPosition();
> +        View v = mTopSitesGrid.getChildAt(position);
> +        TopSitesViewHolder holder = (TopSitesViewHolder)v.getTag();

nit: add space between casting and v.getTag()

Comment 55

[Ian Barlow (:ibarlow)]

Just wanted to chime in here about the design for selecting sites to pin -- my original thought was to make this feel as close to the awesomebar UI as possible, basically with the addition of an OK / Cancel button row along the bottom. 

I notice we're talking about using dialogs, and I don't think that's the path we want to be taking. It feels like more UI we don't need to be adding since we already have a model for finding / going to websites. 

Original rough sketch here: http://cl.ly/image/1F1R2J1W1d1c

Note: I realize that isn't a lot to go on, if you need some higher fidelity mockups to refer to let me know.

Comment 56

[Wesley Johnston (:wesj)]

Attachment: Patch 1/5

Moved this to BrowserDB. Other than that is should be the same.

Comment 57

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5

Backend. I tried to get all the minor comments as I went through these and then came back and did some major moving of code around. I'm nervous I missed some stuff, but wanted to get this up now.

Comment 58

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5

This hooks the backend up to our cursorwrapper, now in BrowserDB. It also switches us to not use a ViewBinder anymore. I like having the ViewHolder instead.

Comment 59

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5

This is the edit site dialog.

Comment 60

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Show a context menu

This is the context menu patch again (all moved OMT, which I thought I'd done before but lost somehow).

Comment 61

[Wesley Johnston (:wesj)]

Comment on attachment 692534 [details] [diff] [review]
Patch 2/5

Lucas is out for Christmas, moving these reviews.

Comment 62

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 692534 [details] [diff] [review]
Patch 2/5

>diff --git a/mobile/android/base/db/LocalBrowserDB.java b/mobile/android/base/db/LocalBrowserDB.java

>     public boolean isBookmark(ContentResolver cr, String uri) {

>                                 new String[] { uri,
>-                                               String.valueOf(Bookmarks.FIXED_READING_LIST_ID) },
>+                                               String.valueOf(Bookmarks.FIXED_READING_LIST_ID),
>+                                               String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID)},

nit: add a space
                                                 String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) },

About Lucas' questions:
* We should add tests for this, but since we a re a little under the gun to get the core in for Fx20, let's do tests in a followup. Please file the bug.
* No syncing of pinned sites. Yet.
* We delete the current pinned site so two sites can't be in the same pinned spot (which answers his next question too)

r+ with nit fixed

Comment 63

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 692535 [details] [diff] [review]
Patch 3/5

>diff --git a/mobile/android/base/AboutHomeContent.java b/mobile/android/base/AboutHomeContent.java

>     private void loadTopSites() {

>                 if (mTopSitesAdapter.getCount() > 0)
>-                    loadTopSitesThumbnails(resolver);
>+                    loadTopSitesThumbnails(resolver, newCursor);

Wondering why we pass the newCursor to loadTopSitesThumbnails. Looking below, we don't seem to use it.

>+    private List<String> getTopSitesUrls(Cursor cursor) {

Except for taking a Cursor, this looks identical to the existing getTopSitesUrls(). We don't even seem to use the Cursor passed into the method. Can we remove this?


>-    public Map<String, Bitmap> getTopSitesThumbnails(Cursor c) {
>+    public Map<String, Bitmap> getThumbnailsFromCursor(Cursor c) {

I can live with this rename

>-    private void loadTopSitesThumbnails(final ContentResolver cr) {
>-        final List<String> urls = getTopSitesUrls();
>+    private void loadTopSitesThumbnails(final ContentResolver cr, final Cursor cursor) {
>+        final List<String> urls = getTopSitesUrls(cursor);

>diff --git a/mobile/android/base/db/BrowserDB.java b/mobile/android/base/db/BrowserDB.java

>     public static Cursor getTopSites(ContentResolver cr, int limit) {
>-        return new TopSitesCursorWrapper(sDb.getTopSites(cr, limit), limit);
>+        Cursor topSites = sDb.getTopSites(cr, limit);
>+        Cursor pinnedSites = sDb.getPinnedSites(cr, limit);
>+        return new TopSitesCursorWrapper(pinnedSites, topSites, limit);

Because this is not a simple pass through anymore, you should probably add a comment noting that we are mixing two datasources

r- just until we find out if the Cursor param in AboutHomeContent.java is needed

Comment 64

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5 - Use the backend

Thanks for the catch. I think I lost track of the new vs old getTopSiteUrls method in iterations.

Comment 65

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 694469 [details] [diff] [review]
Patch 3/5 - Use the backend

>                 if (mTopSitesAdapter.getCount() > 0)
>-                    loadTopSitesThumbnails(resolver);
>+                    loadTopSitesThumbnails(resolver, newCursor);

This change is not needed right? You don't change loadTopSitesThumbnails to take a cursor.

r+ with that fixed too

Comment 66

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 692537 [details] [diff] [review]
Patch 5/5 - Show a context menu

>diff --git a/mobile/android/base/AboutHomeContent.java b/mobile/android/base/AboutHomeContent.java

>+        mTopSitesGrid.setOnCreateContextMenuListener(new View.OnCreateContextMenuListener() {

>+                // if nothing is pinned at all, hide both clear items

Lucas likes starting with a captial: // If

>+                TopSitesCursorWrapper cursor = (TopSitesCursorWrapper)mTopSitesAdapter.getCursor();

no TopSitesCursorWrapper cast needed now

>+                    // if there's nothing pinned here, hide the clear item

Captial: // If

>+    public void clearAllSites() {

>+            clearThumbnail(holder);
>+        }
>+        (new GeckoAsyncTask<Void, Void, Void>(GeckoApp.mAppContext, GeckoAppShell.getHandler()) {

nit: Add blank line

>+    public void clearSite() {
>+        final int position = mTopSitesGrid.getSelectedPosition();
>+        View v = mTopSitesGrid.getChildAt(position);
>+        TopSitesViewHolder holder = (TopSitesViewHolder) v.getTag();
>+        // Quickly update the view so that there isn't as much lag between the request and response

nit: Add blank line before comment

>+        clearThumbnail(holder);
>+        (new GeckoAsyncTask<Void, Void, Void>(GeckoApp.mAppContext, GeckoAppShell.getHandler()) {

nit: Add blank line

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>+    public boolean onContextItemSelected(MenuItem item) {

I assume this naturally wants to live in BrowseApp and not AboutHomeContent?

r+ with nits fixed

Comment 67

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Show edit dialog

This is a much simpler version that just shows the awesomescreen. If you actually select something in there, we get a callback and push it back to AboutHome. If you hit back or something to leave, we just close it.

Comment 68

[Wesley Johnston (:wesj)]

In fact, there are still some strings we don't need in there either. I left them thinking I would change the urlbar hint text and add some "OK" "Cancel" buttons, but in the name of keeping things simple, we don't really need either.

Comment 69

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Edit Dialog

This removes PickSiteDialog altogether and replaces it by just building the intent and building a custom ActivityResultHandler in AboutHomeContent.java. I also realized that if we're using this, we need to get the title back from the awesomescreen, so made some modifications to allow that.

Comment 70

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Show a context menu

Updated context menu patch

Comment 71

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 695001 [details] [diff] [review]
Patch 4/5 - Edit Dialog

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java

>-    public static enum Target { NEW_TAB, CURRENT_TAB };
>+    public static enum Target { NEW_TAB, CURRENT_TAB, PICK_SITE };

PICK_SITE -> CHOOSE_SITE

Thanks for re-working this patch

Comment 72

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 695002 [details] [diff] [review]
Patch 5/5 - Show a context menu

You unbitrotted this, but did not make any changes in comment 66

r+ with those nits fixed

Comment 73

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch set is big enough that you should probably submit them all to Try Server before landing. Just to be safe.

Comment 74

[Wesley Johnston (:wesj)]

Off to try: https://tbpl.mozilla.org/?tree=Try&rev=7b2f81930da5

Comment 75

[Wesley Johnston (:wesj)]

Try was... interesting: https://tbpl.mozilla.org/?tree=Try&rev=eb09bf4733c7

I'm going to retrigger a bunch of robocop runs to see if I was unlucky or if this is making things more orangey.

Comment 76

[Wesley Johnston (:wesj)]

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/48cd86fc9dfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ffc0cb25398
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74b5611f5d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c914a1870b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c2f983a4ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/96daad5f3d98

Comment 77

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/48cd86fc9dfe
https://hg.mozilla.org/mozilla-central/rev/3ffc0cb25398
https://hg.mozilla.org/mozilla-central/rev/e74b5611f5d1
https://hg.mozilla.org/mozilla-central/rev/c2c914a1870b
https://hg.mozilla.org/mozilla-central/rev/a9c2f983a4ea
https://hg.mozilla.org/mozilla-central/rev/96daad5f3d98

Comment 78

[Geoff Brown [:gbrown]]

Causing intermittent crashes in tests.

Do we need a guard for a null mTopSitesAdapter in AboutHomeContent.onDestroy()?

Comment 79

[Wesley Johnston (:wesj)]

(In reply to Geoff Brown [:gbrown] from comment #78)
> Causing intermittent crashes in tests.
> 
> Do we need a guard for a null mTopSitesAdapter in
> AboutHomeContent.onDestroy()?

I filed bug 825992 for this fix.

Comment 80

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Geoff Brown [:gbrown] from comment #78)
> Causing intermittent crashes in tests.
> 
> Do we need a guard for a null mTopSitesAdapter in
> AboutHomeContent.onDestroy()?

Since we have bugs filed for the cleanup and crash, I'm going to re-FIXED this bug and let the other bugs carry the work.

Comment 81

[Ioana Chiorean]

Attachment: screenshots

The top sites from about:home is now customizable. 
Firefox 20.0a1 (2013-01-02)
Device: Galaxy Nexus 
OS: Android 4.1.1

Comment 82

[Nick Alexander :nalexander [he/him]]

This ticket touches code that needs to be kept in sync with the Android services git repo, namely sync_strings.dtd and services/strings.xml.in.  I will update the git repo, but please try to keep Android Services (rnewman, me, or liuche) in the loop around such things.