Closed Bug 708266 Opened 10 years ago Closed 10 years ago

Support viewing and opening remote tabs

Categories

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

ARM
Android
defect

Tracking

(firefox15 verified, blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 13
Tracking Status
firefox15 --- verified
blocking-fennec1.0 --- beta+

People

(Reporter: madhava, Assigned: sriram)

References

Details

(Keywords: uiwanted)

Attachments

(2 files, 3 obsolete files)

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)
Assignee: nobody → madhava
Priority: -- → P5
Priority: P5 → P4
Assignee: madhava → nobody
Assignee: nobody → lucasr.at.mozilla
I'm more and more leaning towards having this be connected to the tab list rather than the awesomescreen.
Keywords: uiwanted
Attached image 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!
Being able to see and open tabs from other computers is a blocker
Priority: P4 → P2
Summary: let users see list of remote tabs → Support viewing and opening remote tabs
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
(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?
> * "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.
OS: Mac OS X → Android
Hardware: x86 → ARM
Already started working on it.. :)
Assignee: lucasr.at.mozilla → sriram
OS: Android → Mac OS X
Hardware: ARM → x86
Attached patch Patch (obsolete) — Splinter Review
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! :)
Attachment #604175 - Flags: review?(mark.finkle)
(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.)
OS: Mac OS X → Android
Hardware: x86 → ARM
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.
Attachment #604175 - Flags: feedback+
(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?
Attached patch Patch (obsolete) — Splinter Review
This patch takes rnewman's feedback into account.
Attachment #604175 - Attachment is obsolete: true
Attachment #604175 - Flags: review?(mark.finkle)
Attachment #604206 - Flags: review?(mark.finkle)
(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.
> 
> > 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.
Attached patch Patch (obsolete) — Splinter Review
Moar changes. :(
Attachment #604206 - Attachment is obsolete: true
Attachment #604206 - Flags: review?(mark.finkle)
Attachment #604218 - Flags: review?(mark.finkle)
(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.
Attached patch PatchSplinter Review
Made the last recommended change too.
Attachment #604218 - Attachment is obsolete: true
Attachment #604218 - Flags: review?(mark.finkle)
Attachment #604252 - Flags: review?(mark.finkle)
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.
Attachment #604252 - Flags: review+
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)
Attachment #604252 - Flags: review?(mark.finkle) → review+
Depends on: 734425
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d7ddd2fe6d8
OS: Android → Mac OS X
Hardware: ARM → x86
*dons sheriff hat, sets metadata*

https://wiki.mozilla.org/Tree_Rules/Inbound
OS: Mac OS X → Android
Hardware: x86 → ARM
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/7d7ddd2fe6d8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 734876
Depends on: 734893
Depends on: 734880
Verified fixed on:

Firefox 15.0a1 (2012-04-30)
Device: Samsung Captivate
OS: Android 2.2
Status: RESOLVED → VERIFIED
Flags: in-litmus?(fennec)
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.
Flags: in-litmus?(fennec) → in-moztrap+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.