708266 - Support viewing and opening remote tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Madhava Enros [:madhava]]

As in the xul version of Fennec, we should let users pull up a list of the tabs they have open on other computers.

(yes, more detail is required - just getting the ball rolling, bug-wise)

Comment 1

[Madhava Enros [:madhava]]

I'm more and more leaning towards having this be connected to the tab list rather than the awesomescreen.

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: Remote tabs UI mocks

Hey guys, here are some mockups describing where remote tabs should live. They should be accessible from the Start Page, and from the bottom of the tabs menu as Madhava mentioned above. 

Have a look, and let me know if you have any questions!

Comment 3

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

Being able to see and open tabs from other computers is a blocker

Comment 4

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

(In reply to Ian Barlow (:ibarlow) from comment #2)
> Created attachment 600401 [details]
> Remote tabs UI mocks
> 
> Hey guys, here are some mockups describing where remote tabs should live.
> They should be accessible from the Start Page, and from the bottom of the
> tabs menu as Madhava mentioned above. 
> 
> Have a look, and let me know if you have any questions!

There is no reason to copy the XUL version approach, and your approach has a nice feel to it. Some feedback:
* "Tabs from your other computers" is borderline too big given l10n issues
* Showing the favicon (if we have it cached) and the URL might be helpful for the user who has 100 tabs open on there other computer.
* Even though it might have issues, the XUL approach does have support for many of the UI parts we could add (favicon and URL) as well as support for sectioned groups of things to navigate to. Any reason why the proposed approach is better in your view?

Comment 5

[Ian Barlow (:ibarlow)]

> * "Tabs from your other computers" is borderline too big given l10n issues

Ok, I can take another pass at that.

> * Showing the favicon (if we have it cached) and the URL might be helpful
> for the user who has 100 tabs open on there other computer.

I would love to get favicons -- my initial understanding was that we couldn't get them yet, which is why they weren't in the mocks. rnewman, sriram and I were talking about this last week, and they seemed confident that we actually could. So if that's the case we certainly should try.

As for showing the URL, I'd like to try keeping the list to 1 line if we can, for simplicity's sake. I'm not completely against showing both the title and the URL, but it would be great to start with less and see how it feels first.


> * Even though it might have issues, the XUL approach does have support for
> many of the UI parts we could add (favicon and URL) as well as support for
> sectioned groups of things to navigate to. Any reason why the proposed
> approach is better in your view?

The only major issue we have with the XUL approach is its location, really. It just makes more sense to group remote tabs into your tabs menu. If we can add favicons, great. URL, see above. Groups, that would be great too. It only seemed that initially, all that was feasible was a list of titles with no favicons, and while not ideal that was still better than not having synced tabs at all.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Already started working on it.. :)

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch shows tabs from other computers.

As discussed in IRC, "Sycned tabs" text is not shown if:
1. Sync is not set up
2. There are no tabs to display.

This also moves "background" to drawable-nodpi/ folder (where it should have been :( )
AsyncTask's are used for querying the db.

RemoteTabs opens in the range of 150ms to 300ms. Yaaay! :)

Comment 8

[Richard Newman [:rnewman]]

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

> As discussed in IRC, "Sycned tabs" text is not shown if:
> 1. Sync is not set up
> 2. There are no tabs to display.

Not sure I agree with the latter; it makes it hard to see if tab sync is enabled, and a user will start to question if things are working right.

I'd only hide the Synced Tabs UI if there are no remote clients at all. If there are, but some have empty tabs, it's a comforting affordance to see them. (Put the empty clients last.)

Comment 9

[Richard Newman [:rnewman]]

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

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

Drive-by.

::: mobile/android/base/LinkTextView.java
@@ +1,3 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0.

::: mobile/android/base/RemoteTabs.java
@@ +1,3 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0.

@@ +110,5 @@
> +        getWindowManager().getDefaultDisplay().getMetrics(metrics);
> +
> +        sChildItemHeight = (int) (CHILD_ITEM_HEIGHT * metrics.density);
> +        sGroupItemHeight = (int) (GROUP_ITEM_HEIGHT * metrics.density);
> +        sPreferredHeight = (int) (0.67 * metrics.heightPixels);

Extract that constant.

@@ +181,5 @@
> +
> +            if (childrenHeight <= sPreferredHeight) {
> +                restrictedHeightSpec = MeasureSpec.makeMeasureSpec(childrenHeight, MeasureSpec.EXACTLY);
> +            } else {
> +                restrictedHeightSpec = MeasureSpec.makeMeasureSpec(sPreferredHeight, MeasureSpec.EXACTLY);

int restrictedHeight = Math.min(childrenHeight, sPreferredHeight);
int restrictedHeightSpec = MeasureSpec.makeMeasureSpec(restrictedHeight, MeasureSpec.EXACTLY);

@@ +194,5 @@
> +    private class QueryRemoteTabsTask extends GeckoAsyncTask<Void, Void, Void> {
> +        @Override
> +        protected Void doInBackground(Void... unused) {
> +            mClientsList = new ArrayList <HashMap <String, String>>();
> +            mTabsList = new ArrayList < ArrayList <HashMap <String, String>>>();

Space before "ArrayList".

I recommend using the allocate-fill-set-immutable approach here, just like the static initializer in BrowserProvider…

@@ +200,5 @@
> +            Cursor tabs = getContentResolver().query(BrowserContract.Tabs.CONTENT_URI,
> +                                                     PROJECTION_COLUMNS,
> +                                                     BrowserContract.Tabs.CLIENT_GUID + " IS NOT NULL",
> +                                                     null,
> +                                                     null);

if (tabs == null) {
  return null;
}

@@ +203,5 @@
> +                                                     null,
> +                                                     null);
> +            
> +            String oldGuid = null;
> +            ArrayList <HashMap<String, String>> tabsForClient = new ArrayList < HashMap <String, String>>();

Why are you assigning here? You do it at the start of each `while` iteration. Just do it in the other branch of the `else`, and avoid the redundant allocation.

@@ +208,5 @@
> +            HashMap <String, String> client;
> +            HashMap <String, String> tab;
> +            
> +            try {
> +                while (tabs != null && tabs.moveToNext()) {

Eliminate null check.

@@ +216,5 @@
> +                    String name = tabs.getString(3);
> +            
> +                    if (oldGuid == null || !TextUtils.equals(oldGuid, guid)) {
> +                        client = new HashMap<String, String>();
> +                        client.put("name", name);

Gosh, I wish building this list didn't require a HashMap for each entry.

@@ +219,5 @@
> +                        client = new HashMap<String, String>();
> +                        client.put("name", name);
> +                        mClientsList.add(client);
> +
> +                        tabsForClient = new ArrayList < HashMap <String, String>>();

Nit: remove space before HashMap.

@@ +227,5 @@
> +                    }
> +            
> +                    tab = new HashMap<String, String>();
> +                    tab.put("title", title);
> +                    tab.put("url", url);

Handle nulls here?

@@ +231,5 @@
> +                    tab.put("url", url);
> +                    tabsForClient.add(tab);
> +                }
> +            } finally {
> +                tabs.close();

Return early above, because if tabs == null you'll NPE here.

@@ +252,5 @@
> +                                                           new int [] { R.id.client },
> +                                                           mTabsList,
> +                                                           R.layout.remote_tabs_child,
> +                                                           new String [] { "title" },
> +                                                           new int [] { R.id.tab });

Gosh, that's a confusing arguments list.

Zerothly, no space before [].

Firstly, break out the constants.

  private static int[] TAB_WHATEVER = new int[] { R.id.tab };
  private static String[] TABS_FOOBAR = new String[] { TABS_TITLE_KEY };

::: mobile/android/base/TabsTray.java
@@ +126,5 @@
>          tabs.refreshThumbnails();
>          onTabChanged(null, null);
> +
> +         // If sync is set up, query the database for remote clients
> +         if (AccountManager.get(getApplicationContext()).getAccountsByType("org.mozilla.firefox_sync").length > 0)

Cross-reference "Bug 734211 - Expose safe account creation API for profile migration". You eventually shouldn't need to do this yourself.

@@ +218,5 @@
> +                                                        null,
> +                                                        null,
> +                                                        null,
> +                                                        null);
> +           

if (clients == null)
    return false;

@@ +227,5 @@
> +                    clientsExist = true;
> +                else
> +                    clientsExist = false;
> +            } finally {
> +                clients.close();

This will NPE on a null cursor.

@@ +230,5 @@
> +            } finally {
> +                clients.close();
> +            }
> +
> +            return clientsExist;

Better version of this whole clause:

if (clients == null)
    return false;

try {
    return clients.moveToNext();
} finally {
    clients.close();
}

::: mobile/android/base/db/TabsProvider.java.in
@@ +527,5 @@
>                      debug("Using sort order " + sortOrder + ".");
>                  }
>  
>                  qb.setProjectionMap(TABS_PROJECTION_MAP);
> +                qb.setTables(TABLE_TABS + " LEFT OUTER JOIN " + TABLE_CLIENTS + " ON (" + TABLE_TABS + "." + Tabs.CLIENT_GUID + " = " + TABLE_CLIENTS + "." + Clients.GUID + ")");

Without getting more context on the file, my instinct here is that you should be doing this under a different URI.

Not every caller will warrant the expense of a LEFT OUTER JOIN here.

Comment 10

[Sriram Ramasubramanian [:sriram]]

(In reply to Richard Newman [:rnewman] from comment #9)
> @@ +203,5 @@
> > +                                                     null,
> > +                                                     null);
> > +            
> > +            String oldGuid = null;
> > +            ArrayList <HashMap<String, String>> tabsForClient = new ArrayList < HashMap <String, String>>();
> 
> Why are you assigning here? You do it at the start of each `while`
> iteration. Just do it in the other branch of the `else`, and avoid the
> redundant allocation.
> 

Java complains that tabsForClient might not have been initialized. That's why doing it once outside.

> @@ +252,5 @@
> > +                                                           new int [] { R.id.client },
> > +                                                           mTabsList,
> > +                                                           R.layout.remote_tabs_child,
> > +                                                           new String [] { "title" },
> > +                                                           new int [] { R.id.tab });
> 
> Gosh, that's a confusing arguments list.

That is going to make things more confusing. I would have to go up to lookup everything. This somehow feels convincing to me, in the long run.

> Better version of this whole clause:
> 
> if (clients == null)
>     return false;
> 
> try {
>     return clients.moveToNext();
> } finally {
>     clients.close();
> }

Won't we end up not closing clients at all?

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch takes rnewman's feedback into account.

Comment 12

[Richard Newman [:rnewman]]

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

> Java complains that tabsForClient might not have been initialized. That's
> why doing it once outside.

That means you're doing some work in the wrong scope, or tabsForClient is allowed to be null, and should be initialized to null.

Compiler warnings like that are hints that you're doing something in the wrong place. Allocating twice isn't the right answer.

> That is going to make things more confusing. I would have to go up to lookup
> everything. This somehow feels convincing to me, in the long run.

You're allocating three or more arrays afresh each time. Make them const, save GC pressure. By all means define them directly above the method, but naming them is clearer for readers who aren't you, and defining them once is more efficient.

> 
> > Better version of this whole clause:
> > 
> > if (clients == null)
> >     return false;
> > 
> > try {
> >     return clients.moveToNext();
> > } finally {
> >     clients.close();
> > }
> 
> Won't we end up not closing clients at all?

If it's null, it doesn't need closing. If it's not null, the finally clause will catch it.

Comment 13

[Sriram Ramasubramanian [:sriram]]

> 
> > That is going to make things more confusing. I would have to go up to lookup
> > everything. This somehow feels convincing to me, in the long run.
> 
> You're allocating three or more arrays afresh each time. Make them const,
> save GC pressure. By all means define them directly above the method, but
> naming them is clearer for readers who aren't you, and defining them once is
> more efficient.
> 

How many times are we going to create this afresh? Once every time RemoteTabs activity is launched. Which is same as what you propose. Expect that the variables are created upfront in your case, and I'm creating them only when needed.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Moar changes. :(

Comment 15

[Richard Newman [:rnewman]]

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

> How many times are we going to create this afresh? Once every time
> RemoteTabs activity is launched. Which is same as what you propose. Expect
> that the variables are created upfront in your case, and I'm creating them
> only when needed.

No, it'll happen in the static initializer for the class, which is run once prior to any use of the class.

http://stackoverflow.com/questions/3499214/java-static-class-initialization

So long as Fennec is in memory, no additional work will occur.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Made the last recommended change too.

Comment 17

[Richard Newman [:rnewman]]

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

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

::: mobile/android/base/RemoteTabs.java
@@ +148,5 @@
> +            int childrenHeight = groupCount * sGroupItemHeight;
> +            for (int i = 0; i < groupCount; i++)
> +                 childrenHeight += adapter.getChildrenCount(i) * sChildItemHeight;
> +
> +            int restrictedHeightSpec = MeasureSpec.makeMeasureSpec(Math.min(childrenHeight,sPreferredHeight), MeasureSpec.EXACTLY);

Nit: space after comma.

Also, if you assign to a `final` local variable and only use it once, the compiler will generate exactly the same bytecode, but you won't have a 120-character line :)

::: mobile/android/base/TabsTray.java
@@ +232,5 @@
> +        }
> +
> +        @Override
> +        protected void onPostExecute(Boolean clientsExist) {
> +            if (clientsExist.booleanValue() == true)

`== true` is redundant.

Comment 18

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

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


>diff --git a/mobile/android/base/RemoteTabs.java b/mobile/android/base/RemoteTabs.java
>+    private static final String LOGTAG = "RemoteTabs";

nit: use "GeckoRemoteTabs". We prefix "Gecko" for easier logcat filtering.

>+        mList.setOnChildClickListener(this);
>+        
>+        LinearLayout container = (LinearLayout) findViewById(R.id.container);

nit: the blank line has trailing spaces

>+    }
>+    
>+    @Override

nit: same and some others in the rest of the file

>diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java
>         setContentView(R.layout.tabs_tray);
>-
>+        

nit: don't add the spaces

r+ with these nits addressed (and rnewman's too)

Comment 19

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7d7ddd2fe6d8

Comment 20

[Richard Newman [:rnewman]]

*dons sheriff hat, sets metadata*

https://wiki.mozilla.org/Tree_Rules/Inbound

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7d7ddd2fe6d8

Comment 22

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 15.0a1 (2012-04-30)
Device: Samsung Captivate
OS: Android 2.2

Comment 23

[Adrian Tamas (:AdrianT)]

Opening the list from about:home is covered by the existing test case: https://moztrap.mozilla.org/manage/cases/_detail/5494/

Opening the list from the Tabs Menu is covered by the new test case https://moztrap.mozilla.org/manage/cases/_detail/6359/ created in the BFTs run in the Firefox Sync testsuite.