Last Comment Bug 695152 - Tabs, Separate activity, some ui spec needed
: Tabs, Separate activity, some ui spec needed
Status: RESOLVED FIXED
: feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: Firefox 11
Assigned To: Sriram Ramasubramanian [:sriram]
:
Mentors:
: 695808 (view as bug list)
Depends on: 697095 697098
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 14:13 PDT by Erin Lancaster [:elan]
Modified: 2012-01-10 10:32 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Initial Patch (58.68 KB, patch)
2011-10-21 09:11 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review-
Details | Diff | Review
Patch for tabs tray (65.50 KB, patch)
2011-10-24 12:23 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Review
Preserving tabs's title and favicon (1.40 KB, patch)
2011-10-24 12:37 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Review

Description Erin Lancaster [:elan] 2011-10-17 14:13:36 PDT
* Separate activity
* some ui spec needed
Comment 1 Sriram Ramasubramanian [:sriram] 2011-10-21 09:11:46 PDT
Created attachment 568668 [details] [diff] [review]
Initial Patch

This patch moves the tabs to a popupwindow in same activity.
Since "Tabs" is held separate, the popup shows even when Gecko is loading.
The tabs's title, url, favicon (and even new tabs opening) are updated in the popup when it is showing (for such events happening in the background).
Close tabs work by switching to previous tab in the list.

Some followups:
1. There are 2 data structures used: one for fast retrieval, the other for preserving the order. This needs to be optimized (ArrayList<Tab> is same as ArrayList<Integer> though).
2. The close tabs does something with gecko when the tab closed is the selected tab. I'm failing to understand the reason. The java side sends proper messages.
3. The tab popup has a height of 50% on portrait mode. This need to be changed with the list's height. I would like to take this as a followup bug with the actual styling for the popupwindow.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-24 10:19:23 PDT
Comment on attachment 568668 [details] [diff] [review]
Initial Patch

This patch is pretty much ready to land except:
* Move the "New Tab" string into a resource (don't use "+ add tab" as the text either)
* I want to use a convention for our JS->Java messages, so for the tab stuff:
** "onAddTab" -> "Tab:Added"
** "onCloseTab" -> "Tab:Closed"
** "onSelectTab" -> "Tab:Selected"

While we are doing this, change the Java->JS names:
* "tab-add" -> "Tab:Add"
* "tab-load" -> "Tab:Load"
* "tab-select" -> "Tab:Select"
* "tab-close" -> "Tab:Close"

I think we'll be r+ ready with those changes
Comment 3 Sriram Ramasubramanian [:sriram] 2011-10-24 12:23:44 PDT
Created attachment 569136 [details] [diff] [review]
Patch for tabs tray

Made the required changes.
This also fixes the "close-tab" problem.
  - When selected tab is closed, a tab is explicitly selected _before_ closing the tab.
  - When a background tab is closed, a tab is explicitly selected _after_ closing the tab.
Comment 4 Sriram Ramasubramanian [:sriram] 2011-10-24 12:37:57 PDT
Created attachment 569139 [details] [diff] [review]
Preserving tabs's title and favicon

This patch preserves the tabs count, title and favicon of the selected tab on the BrowserToolbar on rotation.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-24 13:50:18 PDT
Comment on attachment 569136 [details] [diff] [review]
Patch for tabs tray

Just some questions:
* Do we need to keep Tabs.java order array? or can we try to remove it in the future?
* the screen shot at http://cl.ly/0w3I311s2a3w3g0r2B3C shows the "tabs" button is a little offset vertically from the rest of the URL bar. We can file a followup for fixing that, but I wanted to point it out.
* File a followup to fix the "prompt" message handler


r+
Comment 6 Doug Turner (:dougt) 2011-10-24 17:08:31 PDT
*** Bug 695808 has been marked as a duplicate of this bug. ***
Comment 7 Sriram Ramasubramanian [:sriram] 2011-10-24 17:18:54 PDT
http://hg.mozilla.org/projects/birch/rev/8db2fba86f75 - fixed the issue.

Note You need to log in before you can comment on or make changes to this bug.