734425 - Show most recently used tabs in about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

about:home should show the "most recently used" tabs from "most recently used client". This is a part of the bug 708266.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch shows "most recently modified top 10" sites from "most recently used" client.

I'm not using a HashMap ;) RemoteTab is just for that particular AsyncTask.

Somehow my query for "select * from clients where last_modified = (select max(last_modified) from clients)" didn't work. I have to pull all the clients, take the first one and pull all the tabs.
Also, though SQLiteQueryBuilder support "limit" parameter in query(), ContentProvider doesn't. So, I have to stop at a max of 10 tabs in AboutHomeContent.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 604555 [details] [diff] [review]
Patch

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

r- for:

* inappropriate coupling between display/controller code and data access code
* issuing two queries where one would do
* magic numbers everywhere.

Aim for having a class that can asynchronously give you a collection of tabs (e.g., in a delegate method callback, or an asynchronous task) such that you could test it without ever instantiating any UI components. Parameterize it, so I don't have to see only ten tabs on my 12" tablet. And make it efficient by pushing joins and limits down into the content provider.

::: mobile/android/base/AboutHomeContent.java
@@ +633,5 @@
> +        (new QueryRemoteTabs()).execute(activity);
> +    }
> +
> +    // AsyncTask to query the database
> +    private class QueryRemoteTabs extends GeckoAsyncTask<Activity, Void, Void> 

I don't believe this belongs in AboutHomeContent.java. Until now, AboutHomeContent has worked at the "ask for a cursor" level of abstraction: BrowserDB.getTopSites().

Lifting this into a separate class (along with whatever other "open tab" utility code exists) makes sense to me, and will probably expose some assumptions and implicit interfaces that you're not seeing. Yes, that'll mean you need to define an interface (TabQueryDelegate?) and avoid mutating shared member variables from an async task. I view that as a good thing.

At the *very* least, consider making this a static class, and see how many red 'x'es you get in Eclipse.

@@ +673,5 @@
> +
> +            try {
> +                if (client.moveToNext()) {
> +                    mGuid = client.getString(0);
> +                    mClient = client.getString(1);

These magic indices are getting to be a habit, and they're going to bite one day. Lift these to TABS_COLUMN_TITLE = 0, TABS_COLUMN_URL = 1. Or wait, should that be CLIENTS_COLUMN_GUID = 0...

... exactly my point. Name things, please.

@@ +685,5 @@
> +            Cursor tabs = resolver.query(BrowserContract.Tabs.CONTENT_URI,
> +                                         TABS_PROJECTION_COLUMNS,
> +                                         TABS_SELECTION,
> +                                         new String[] { mGuid },
> +                                         null);

You're issuing two queries here, and it's unnecessary. Do the work in the content provider, passing URI parameters as required.

@@ +695,5 @@
> +            mTabsList = new ArrayList <RemoteTab>();
> +        
> +            try {
> +                int i = 0;
> +                while (tabs.moveToNext() && (i < 10)) {

Magic constant. Lift that somewhere... ideally, it should be a parameter to the QueryRemoteTabs constructor.

Additionally, add a ?limit parameter to the CONTENT_URI, and have the content provider parse that out and use it as a LIMIT to the SQL query. No sense creating a query plan to fetch all rows when you're only fetching 10.

@@ +698,5 @@
> +                int i = 0;
> +                while (tabs.moveToNext() && (i < 10)) {
> +                    tab = new RemoteTab();
> +                    tab.title = tabs.getString(0);
> +                    tab.url = tabs.getString(1);

Same with these: name those indices.

@@ +701,5 @@
> +                    tab.title = tabs.getString(0);
> +                    tab.url = tabs.getString(1);
> +                    tab.title = TextUtils.isEmpty(tab.title) ? tab.url : tab.title;
> +                    mTabsList.add(tab);
> +                    i++;

You're not using i within the body, so use

  while (tabs.moveToNext() && (i++ < tabLimit)) {

… or trust the query limit that you're going to add, mm?

@@ +743,5 @@
> +            } catch (Exception e) {
> +                Log.e(LOGTAG, "error building JSON arguments");
> +            }
> +
> +            Log.i(LOGTAG, "Sending message to Gecko: " + SystemClock.uptimeMillis() + " - Tab:Add");

Debug, not info.

Comment 3

[Richard Newman [:rnewman]]

Further reading:

Views:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#301

Selecting a table or view based on projection argument:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#1124

Limit params:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#1095

Comment 4

[Sriram Ramasubramanian [:sriram]]

> Aim for having a class that can asynchronously give you a collection of tabs
> (e.g., in a delegate method callback, or an asynchronous task) such that you
> could test it without ever instantiating any UI components. Parameterize it,
> so I don't have to see only ten tabs on my 12" tablet. And make it efficient
> by pushing joins and limits down into the content provider.

> 
> I don't believe this belongs in AboutHomeContent.java. Until now,
> AboutHomeContent has worked at the "ask for a cursor" level of abstraction:
> BrowserDB.getTopSites().
> 

Agreed that data/view controller is in the same part of code. However, the logic for "storing open tabs" is not done yet! And, this piece of code will eventually move in there -- after which, | Cursor cursor = TabsAccessor.getMeMostRecentTabs(); | will fit in the way you want it. This can be asynchronous too. But, to have the logic done and working, and getting in to Beta "without open tabs logic", I don't find other option. I am all up for MVC pattern -- but, this doesn't feel to me like the right time for it.

Also, I don't understand the "instantiating any UI components" part. When one wants to get hold of some UI views, one needs to hold them in some variables. The whole AboutHomeContent works that way. I didn't want to bring in a new logic two days before Beta cut-off. Calling "findViewById()" everytime is not efficient. Also, whether you have tabs are not, all these are inflated from XML. I am really not up for "inflating based on need", as LayoutInflater.inflate() is a super expensive function.

> Lifting this into a separate class (along with whatever other "open tab"
> utility code exists) makes sense to me, and will probably expose some
> assumptions and implicit interfaces that you're not seeing. Yes, that'll
> mean you need to define an interface (TabQueryDelegate?) and avoid mutating
> shared member variables from an async task. I view that as a good thing.
> 
> At the *very* least, consider making this a static class, and see how many
> red 'x'es you get in Eclipse.
> 

I don't use Eclipse, and hence can't see 'x'es. :D
I have addressed your concern above.

> @@ +673,5 @@
> > +
> > +            try {
> > +                if (client.moveToNext()) {
> > +                    mGuid = client.getString(0);
> > +                    mClient = client.getString(1);
> 
> These magic indices are getting to be a habit, and they're going to bite one
> day. Lift these to TABS_COLUMN_TITLE = 0, TABS_COLUMN_URL = 1. Or wait,
> should that be CLIENTS_COLUMN_GUID = 0...
> 

The column indices are mentioned in comments along with projections. If we want a new convention, it should be done in all places to be consistent.
Inspiration: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#1310

> 
> You're issuing two queries here, and it's unnecessary. Do the work in the
> content provider, passing URI parameters as required.
> 

I didn't want to do two queries, but "max(column_name)" didn't work somehow. I would try moving this as arguments into TabsProvider anyways.

> @@ +695,5 @@
> > +            mTabsList = new ArrayList <RemoteTab>();
> > +        
> > +            try {
> > +                int i = 0;
> > +                while (tabs.moveToNext() && (i < 10)) {
> 
> Magic constant. Lift that somewhere... ideally, it should be a parameter to
> the QueryRemoteTabs constructor.

I don't find a need to use a constant here. I checked with Ian, and he asked me to have it as 10. And honestly, this is not going to change. This piece of code is not going to be reused. Why would we need a "parameterized" call then?

> 
> Additionally, add a ?limit parameter to the CONTENT_URI, and have the
> content provider parse that out and use it as a LIMIT to the SQL query. No
> sense creating a query plan to fetch all rows when you're only fetching 10.
> 

I can do that. I'll change the code to work that way.

> 
> You're not using i within the body, so use
> 
>   while (tabs.moveToNext() && (i++ < tabLimit)) {
> 
> … or trust the query limit that you're going to add, mm?

I am still old school, and don't like "i++" in the "if clause" :) Anyways, query limit will remove the need for 'i'.

> 
> @@ +743,5 @@
> > +            } catch (Exception e) {
> > +                Log.e(LOGTAG, "error building JSON arguments");
> > +            }
> > +
> > +            Log.i(LOGTAG, "Sending message to Gecko: " + SystemClock.uptimeMillis() + " - Tab:Add");
> 
> Debug, not info.

Copied from: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#2564 , to be consistent.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Also, I wouldn't call it "inappropriate coupling of display/controller logic". AsyncTasks are meant to work in that fashion.

Do some work in doInBackground() -- controlller.
Use the results to update UI thread in onPostExecute() -- display logic :)

I could factor out the controller logic out, but I am waiting to complete the "storing fennec open tabs in database" part, to move the code from here and "open tabs UI" there.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> Also, I wouldn't call it "inappropriate coupling of display/controller
> logic". AsyncTasks are meant to work in that fashion.

The same file both updates UI and composes SQL queries. I don't see how you could get more coupled!

> Do some work in doInBackground() -- controlller.
> Use the results to update UI thread in onPostExecute() -- display logic :)

You don't think it makes more sense to have an explicit "get me N tabs from other computers" method, abstracting away the database?

> I could factor out the controller logic out, but I am waiting to complete
> the "storing fennec open tabs in database" part, to move the code from here
> and "open tabs UI" there.

I'm not concerned about blurring of view and controller (to use MVC terminology). I'm concerned about blurring of model and controller.

That is, you have a single class that's doing everything from UI layout to computing, executing, and processing SQL queries, including knowing about which columns are in which database tables. That ain't blurring, that's PHP!


(In reply to Sriram Ramasubramanian [:sriram] from comment #4)

> > I don't believe this belongs in AboutHomeContent.java. Until now,
> > AboutHomeContent has worked at the "ask for a cursor" level of abstraction:
> > BrowserDB.getTopSites().
> > 
> 
> Agreed that data/view controller is in the same part of code. However, the
> logic for "storing open tabs" is not done yet! And, this piece of code will
> eventually move in there -- after which, | Cursor cursor =
> TabsAccessor.getMeMostRecentTabs(); | will fit in the way you want it. This
> can be asynchronous too. But, to have the logic done and working, and
> getting in to Beta "without open tabs logic", I don't find other option. I
> am all up for MVC pattern -- but, this doesn't feel to me like the right
> time for it.

I don't see why writing TabsAccessor, and lifting this data access task into it, is a big deal. You don't need to have the rest of the tabs code done. You can add *that* to the TabsAccessor file when it's done.

(If you don't want to do it, I'll do it!)

> Also, I don't understand the "instantiating any UI components" part. When
> one wants to get hold of some UI views, one needs to hold them in some
> variables. The whole AboutHomeContent works that way. I didn't want to bring
> in a new logic two days before Beta cut-off.

My point was that the code which fetches tabs from the database should not be muddled with the same code that builds views: the former should be separated "such that you could test it without ever instantiating any UI components".

The UI code should be calling a single method to get a collection of tabs, then building the UI from that. You could just as easily write an integration test that populated the DB then verified that you got the right collections of tabs by calling that method -- no UI required.

Phrased differently: how would you get 100% JUnit coverage of your data access code?

I'm trying to get you to think about building code that you won't be scared to touch in three years. Separation of concerns, designing for testability, intrinsic documentation, no magic numbers… all of this is well-understood.


> Calling "findViewById()"
> everytime is not efficient. Also, whether you have tabs are not, all these
> are inflated from XML. I am really not up for "inflating based on need", as
> LayoutInflater.inflate() is a super expensive function.

I wasn't talking about that at all.


> I don't use Eclipse, and hence can't see 'x'es. :D

Ah, that explains why you're so resistant to refactoring! ;)

I'm a lifetime Vim user, but writing Java without an IDE is just masochism.


> The column indices are mentioned in comments along with projections. If we
> want a new convention, it should be done in all places to be consistent.
> Inspiration:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> BrowserProvider.java.in#1310

Ahem:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#589

Elsewhere in BrowserProvider, the appropriate getColumnIndex convention is used instead.

The BrowserProvider example you cite has the projection within the same method as the magic numbers that refer to it, and uses the magic numbers for perf reasons. You're defining the projection at class scope, and using the indices outside of a reasonable proximity.

Oh, and BrowserProvider can be assumed to be somewhat brittle around schema changes! That's not true of view code.

Use final statics: save yourself the bugs later, take advantage of the free documentation.

Srsly, consistency is not an argument for bad code. Either break the convention and do it right, or fix the place from which you're copying.


> I didn't want to do two queries, but "max(column_name)" didn't work somehow.
> I would try moving this as arguments into TabsProvider anyways.

You don't need a MAX and a subselect; you need an ORDER BY and a LIMIT, which will be much more efficient. All of these features can only be provided by the CP, which is why I suggest:

* Adding a content URI that does a limited, ordered, joined query on your behalf, and
* Adding a data access class that knows about the structure of the DB and the CP URIs, so your calling code doesn't.

> I don't find a need to use a constant here. I checked with Ian, and he asked
> me to have it as 10. And honestly, this is not going to change. This piece
> of code is not going to be reused. Why would we need a "parameterized" call
> then?

You can't conceive of a single instance in the future in which you:

* might want to use a different value, e.g., to support everything from 3" to 12" screens;
* might want to rely on that value elsewhere in code (e.g., additional layout computations) without hard-coding the same magic number in two pieces of code;
* might have a coworker who's trying to figure out what this code does, and why that "10" is important?

The only values that are acceptable to hard-code are universal constants, like milliseconds per second. Everything else is subject to change.

The better way to look at this is "do I have a need to inline this value in the code?".

> Copied from:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#2564 , to be consistent.

Fix it there, too, in a single Part 0. info messages should not be frequent and only of interest to people debugging!

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch addresses the follwing things:
* A new "TabsAccessor" class to do the database related task
** This doesn't return the Cursor, but an ArrayList<RemoteTab>.
* RemoteTabs, TabsTray, AboutHomeContent no longer handles Cursors.
* Magic numbers are replaced with enums
* Logs are changed for Tab additions.

Still there are AsyncTasks. I'm working on delegations, and hope to post a new patch with that tomorrow.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Do we need delegations, or can we live with AsyncTasks? I started working on it, and found that certain things are better done in background thread, when we get the results.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

On top of previous patch:
* Delegations are added in this patch.
* Anything running on TabsAccessor now uses AsyncTask, and others don't have to care about it:
    Reason: To ensure thread safety, as AboutHomeContent starts the task in a background thread, while RemoteTabs calls it from UI thread.

Few doubts/suggestions-required:
* More than reusability, I suck at names. I have used "Listeners" as that's the convention used in Fennec (also Android for Event delegation). And the name of the Listeners might be odd. I am happy for suggestions.
* Fennec code uses X<Y> somewhere and X <Y> somewhere. I took the X<Y> approach. I can change it if needed.

@rnewman: Thanks for reminding me my good old days of using "function pointers in C" to write way too many decoders. :)

Not marking the other patch obsolete, to take a call on if we delegation or not.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

I just realized that showing top 10 tabs in about:home is wrong -- as it can be from more than one client. This patch adds the required fix in 'for loop'.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 604842 [details] [diff] [review]
Patch

I'm guessing this one is obsolete…

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 604966 [details] [diff] [review]
Patch

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

Looks good, modulo changes below and one follow-up.

::: mobile/android/base/RemoteTabs.java
@@ +162,5 @@
>  
> +        String oldGuid = null;
> +        ArrayList <HashMap <String, String>> tabsForClient = null;
> +        HashMap <String, String> client;
> +        HashMap <String, String> tab;

Y'know, if you implement the Map interface for RemoteTab, you can avoid some of this copying and allocation… just pass the RemoteTabs directly into the ListAdapter.

File a follow-up?

@@ +185,5 @@
> +        
> +        if (clients.size() == 0) {
> +            finishActivity();
> +            return;
> +        }

Make this

  if (remoteTabs == null || remoteTabs.size() == 0) {
      finishActivity();
      return;
  }

and move it to the top of the method. You're never going to have an empty clients array unless you have no tabs, right?

::: mobile/android/base/TabsAccessor.java
@@ +17,5 @@
> +
> +public class TabsAccessor {
> +    private static final String LOGTAG = "GeckoTabsAccessor";
> +    private static final TabsAccessor sInstance = new TabsAccessor();
> +    private static ContentResolver mResolver;

This can be final, too, no?

@@ +37,5 @@
> +
> +    private static final String TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL";
> +
> +    private TabsAccessor() {
> +        mResolver = GeckoApp.mAppContext.getContentResolver();

This feels wrong to me — you're implicitly tying the TabsAccessor lifecycle to GeckoApp, and there's no other dependency on it in this class.

Make your static methods take a ContentResolver (or a Context) argument instead, and pass one in when you call them.

(Alternatively, make this class non-static, and pass one of those two things in the public constructor.)

@@ +48,5 @@
> +        public String name;
> +    }
> +
> +    public interface OnQueryTabsCompleteListener {
> +        public void onQueryTabsComplete(ArrayList<RemoteTab> tabs);

Make this List<RemoteTab> instead. No reason to tie yourself to a concrete implementation.

@@ +95,5 @@
> +
> +    // This method returns limited number of tabs from all remote clients, 
> +    // ordered by most recent client first, most recent tab first 
> +    public static void getTabs(final int limit, final OnQueryTabsCompleteListener listener) {
> +        // If there is no listener, no meaning in doing work

s/meaning/point

Punctuation.

Though a good general metaphysical point ;)

@@ +101,5 @@
> +            return;
> +
> +        (new GeckoAsyncTask<Void, Void, ArrayList<RemoteTab>> () {
> +            @Override
> +            protected ArrayList<RemoteTab> doInBackground(Void... unused) {

List<RemoteTab>

@@ +135,5 @@
> +                } finally {
> +                    cursor.close();
> +                }
> +
> +                return tabs;

return Collections.unmodifiableList(tabs);

Unless you want to document that the return value is a mutable copy, and maintain that forever and ever. Returning an unmodifiable list gives you more options around memoization etc.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +27,5 @@
>  
>  <!ENTITY awesomebar_default_text "Enter Search or Address">
>  
>  <!ENTITY remote_tabs "Synced Tabs">
> +<!ENTITY all_remote_tabs "Show all tabs">

Are we allowed to add new strings at this stage of the game?

Comment 14

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

(In reply to Richard Newman [:rnewman] from comment #13)

> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +27,5 @@
> >  
> >  <!ENTITY awesomebar_default_text "Enter Search or Address">
> >  
> >  <!ENTITY remote_tabs "Synced Tabs">
> > +<!ENTITY all_remote_tabs "Show all tabs">
> 
> Are we allowed to add new strings at this stage of the game?

Only for a day or two yet.

Comment 15

[Sriram Ramasubramanian [:sriram]]

(In reply to Richard Newman [:rnewman] from comment #13)
> Comment on attachment 604966 [details] [diff] [review]
> Patch
> 
> Review of attachment 604966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, modulo changes below and one follow-up.
> 
> ::: mobile/android/base/RemoteTabs.java
> @@ +162,5 @@
> >  
> > +        String oldGuid = null;
> > +        ArrayList <HashMap <String, String>> tabsForClient = null;
> > +        HashMap <String, String> client;
> > +        HashMap <String, String> tab;
> 
> Y'know, if you implement the Map interface for RemoteTab, you can avoid some
> of this copying and allocation… just pass the RemoteTabs directly into the
> ListAdapter.
> 
> File a follow-up?

Do you want me to have RemoteTab as Map?

> 
> @@ +185,5 @@
> > +        
> > +        if (clients.size() == 0) {
> > +            finishActivity();
> > +            return;
> > +        }
> 
> Make this
> 
>   if (remoteTabs == null || remoteTabs.size() == 0) {
>       finishActivity();
>       return;
>   }
> 
> and move it to the top of the method. You're never going to have an empty
> clients array unless you have no tabs, right?

Agreed. 2-in-the-night-can't-think-better syndrome ;) Will change it.

> 
> ::: mobile/android/base/TabsAccessor.java
> @@ +17,5 @@
> > +
> > +public class TabsAccessor {
> > +    private static final String LOGTAG = "GeckoTabsAccessor";
> > +    private static final TabsAccessor sInstance = new TabsAccessor();
> > +    private static ContentResolver mResolver;
> 
> This can be final, too, no?

Same problem. 2-in-the-night :D Was about to change it to final. And then left it.

> 
> @@ +37,5 @@
> > +
> > +    private static final String TABS_SELECTION = BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL";
> > +
> > +    private TabsAccessor() {
> > +        mResolver = GeckoApp.mAppContext.getContentResolver();
> 
> This feels wrong to me — you're implicitly tying the TabsAccessor lifecycle
> to GeckoApp, and there's no other dependency on it in this class.
> 
> Make your static methods take a ContentResolver (or a Context) argument
> instead, and pass one in when you call them.
> 
> (Alternatively, make this class non-static, and pass one of those two things
> in the public constructor.)

This felt pretty wrong to me too. I was trying the non-static approach. But I had to get it from GeckoApp everytime, which I didn't feel comfortable. I'll pass Context everytime. That seems better to me.

> 
> @@ +48,5 @@
> > +        public String name;
> > +    }
> > +
> > +    public interface OnQueryTabsCompleteListener {
> > +        public void onQueryTabsComplete(ArrayList<RemoteTab> tabs);
> 
> Make this List<RemoteTab> instead. No reason to tie yourself to a concrete
> implementation.
> 

I've been warned by my friends not to force particular implementation of List or Map in returning. Still haven't changed :D

> 
> @@ +135,5 @@
> > +                } finally {
> > +                    cursor.close();
> > +                }
> > +
> > +                return tabs;
> 
> return Collections.unmodifiableList(tabs);

So, I don't have to declare tabs as final?

> 
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +27,5 @@
> >  
> >  <!ENTITY awesomebar_default_text "Enter Search or Address">
> >  
> >  <!ENTITY remote_tabs "Synced Tabs">
> > +<!ENTITY all_remote_tabs "Show all tabs">
> 
> Are we allowed to add new strings at this stage of the game?

The exact point of rushing with this patch is to get it before beta-cutoff.

Comment 16

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

Comment on attachment 604966 [details] [diff] [review]
Patch

Overall this looked good to me. I see rnewman found a few nice changes. Mine are mostly nits

>+    private void setRemoteTabsVisibility(boolean visible) {
>+        int visibility = visible ? View.VISIBLE : View.GONE;
>+        findViewById(R.id.remote_tabs_title).setVisibility(visibility);
>+        findViewById(R.id.remote_tabs_client).setVisibility(visibility);
>+        findViewById(R.id.remote_tabs).setVisibility(visibility);
>+        findViewById(R.id.all_remote_tabs_text).setVisibility(visibility);
>+    }

I see in the XML that we default to visbility=GONE, which is what I hoped

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

>+    public interface OnClientsAvailabilityListener {

nit: Can we name this "OnClientsAvailableListener" ?

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

> <!ENTITY remote_tabs "Synced Tabs">
>+<!ENTITY all_remote_tabs "Show all tabs">

nits:
  <!ENTITY remote_tabs_title "Synced Tabs">
  <!ENTITY remote_tabs_show_all "Show all tabs">

Comment 17

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #15)

> > File a follow-up?
> 
> Do you want me to have RemoteTab as Map?

It'll be more efficient if you implement the Map interface rather than subclassing a concrete class (because field accesses are faster than hashing a string and looking it up in the hashmap), but either works. The point is that if RemoteTab implements Map, then you can use a RemoteTab anywhere that expects a Map… which includes the Android UI classes. 

> > return Collections.unmodifiableList(tabs);
> 
> So, I don't have to declare tabs as final?

final fields are still modifiable… just not assignable. Both.

Comment 18

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2fbbd922e826

pushed with required changes.

Comment 20

[Paul [pwd]]

But this doesn't solve the issue of if you don't have about:home open and you don't have multiple tabs open. How do you access your sync'd tabs then?

Comment 21

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

(In reply to Paul [sabret00the] from comment #20)
> But this doesn't solve the issue of if you don't have about:home open and
> you don't have multiple tabs open. How do you access your sync'd tabs then?

Don't say "add a menu"

Comment 22

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #21)

> Don't say "add a menu"

Heh.

How about having Synced Tabs accessible somehow through the tab "+" menu? That is, I want to open a new tab by picking one that's open elsewhere. (This is kinda how XUL Fennec did it.)

Or wire it in as a magic set of folders in the Bookmarks pane? "Synced tabs" with a special icon as the first row?

Comment 23

[Paul [pwd]]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Don't say "add a menu"

Haha, I considered it. My initial thought was to reintegrate it back into the AwesomeScreen.

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/2fbbd922e826

Comment 26

[Adrian Tamas (:AdrianT)]

Behavior covered in the test case:

https://moztrap.mozilla.org/manage/cases/_detail/6293/

The test has been included in the BFT run under the Firefox Sync feature suite

Comment 27

[Cristian Nicolae (:xti)]

Changes were applied on the latest Nightly build. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-24)
Device: Galaxy Note
OS: Android 4.0.4