695307 - Awesome bar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

* Needs UI design

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Factor out awesome bar search into AwesomeBarTabs

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (2/4) Factor our method to open URL and close awesome bar

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Implement Bookmarks tab in AwesomeBar

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Implement history tab in AwesomeBar

Comment 5

[Lucas Rocha (:lucasr)]

Those patches are an initial implementation of the current awesome bar features (frecency + bookmarks + history). There's a lot of details to work on though.

Possible follow-up patches:
- Show favicon on all entries in awesomebar
- Handle search entry focus properly when switching tabs
- Dismiss keyboard when it makes sense to do so
- Every other bug that we haven't seen yet ;-)

Comment 6

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

Comment on attachment 569149 [details] [diff] [review]
(3/4) Implement Bookmarks tab in AwesomeBar

I have seen devs using AsyncQueryHandler, instead of AsyncTask for queries, but it looks like you have things covered OK:
http://developer.android.com/reference/android/content/AsyncQueryHandler.html
http://kurtchen.com/blog/2010/05/18/use-asyncqueryhandler-to-do-async-query/

As long as we don't block on the main UI thread.

Comment 7

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

Comment on attachment 569145 [details] [diff] [review]
(1/4) Factor out awesome bar search into AwesomeBarTabs

This looks OK, but I have a question about the query: Why not run the query in an AsyncTask? Does this code block the UI?

I'd still be OK with landing this, even if we need to file a followup to get the All Pages query in an AsyncTask. Getting the basic structure in-place is pretty important.

Comment 8

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

Comment on attachment 569150 [details] [diff] [review]
(4/4) Implement history tab in AwesomeBar

My remedial knowledge of expandable lists and adpaters says this is OK, but I am alittle worried about the amount of work happening in onPostExecute. We need to make sure we don't block the main thread too long, since onPostExecute is executed on the main UI thread. Check logcat for any "strict mode" violations from this code.

We could always lower the MAX_RESULTS a bit if we run into trouble too.

Comment 9

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

(In reply to Lucas Rocha (:lucasr) from comment #5)
> Those patches are an initial implementation of the current awesome bar
> features (frecency + bookmarks + history). There's a lot of details to work
> on though.
> 
> Possible follow-up patches:
> - Show favicon on all entries in awesomebar
> - Handle search entry focus properly when switching tabs
> - Dismiss keyboard when it makes sense to do so
> - Every other bug that we haven't seen yet ;-)

Please file the followup bugs after landing these patches

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 569145 [details] [diff] [review] [diff] [details] [review]
> (1/4) Factor out awesome bar search into AwesomeBarTabs
> 
> This looks OK, but I have a question about the query: Why not run the query
> in an AsyncTask? Does this code block the UI?
> 
> I'd still be OK with landing this, even if we need to file a followup to get
> the All Pages query in an AsyncTask. Getting the basic structure in-place is
> pretty important.

CursorAdapter implements Filterable interface and uses Filter to filter list items. From the documentation, calls to filter.filter(String constraint), which is used in CursorAdapter in this case, run on a separate thread. See:

http://developer.android.com/reference/android/widget/Filter.html

Comment 11

[Lucas Rocha (:lucasr)]

Reported follow-up bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=697120
https://bugzilla.mozilla.org/show_bug.cgi?id=697121
https://bugzilla.mozilla.org/show_bug.cgi?id=697122

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 569150 [details] [diff] [review] [diff] [details] [review]
> (4/4) Implement history tab in AwesomeBar
> 
> My remedial knowledge of expandable lists and adpaters says this is OK, but
> I am alittle worried about the amount of work happening in onPostExecute. We
> need to make sure we don't block the main thread too long, since
> onPostExecute is executed on the main UI thread. Check logcat for any
> "strict mode" violations from this code.
> 
> We could always lower the MAX_RESULTS a bit if we run into trouble too.

I think it's perfectly possible to move this code to the doInBackground method as we're only dealing with the cursor data and no UI is involved. I'll do it in a follow-up patch.

Comment 13

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/projects/birch/rev/e7d33f2753f4
http://hg.mozilla.org/projects/birch/rev/7ff119c4da91
http://hg.mozilla.org/projects/birch/rev/e1766b866859
http://hg.mozilla.org/projects/birch/rev/aaf33b728e58

Comment 14

[Lucas Rocha (:lucasr)]

Forgot to resolve the bug, sorry for the spam.

Comment 15

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1