Closed Bug 436064 Opened 12 years ago Closed 12 years ago

Support for multiple documents

Categories

(Firefox for Android Graveyard :: General, enhancement, P1)

Other
Maemo
enhancement

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0m6

People

(Reporter: christian.bugzilla, Assigned: enndeakin)

References

Details

Attachments

(1 file, 2 obsolete files)

 
Summary: Tab Support → Support for multiple documents
Assignee: nobody → neil
Priority: -- → P1
Assignee: neil → nobody
I assume Christian wanted to assign to Neil Deakin
Assignee: nobody → enndeakin
Yes, thanks for fixing Mark.
Flags: blocking-fennec1.0+
This is my suggested UI for switching between and opening new tabs:

http://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI#Tabs

You may need to look at more of that page for some context.
Target Milestone: Fennec M4 → Fennec M5
Attached patch initial implementation (obsolete) — Splinter Review
This patch adds:
- a tabbar on the left
- a plus button may be used to add a tab
- the Clean button is for testing. It selects the oldest accessed tab and clears out the browser, marking the thumbnail with an X. Clicking the tab again will reload it. This would be used automatically when memory is tight to delete child browsers.

Still to do:
- restore other aspects of the tab such as form field values
- sometimes the thumbnails don't draw properly
Target Milestone: Fennec M5 → Fennec M6
Attachment #330046 - Attachment is obsolete: true
Attachment #330937 - Flags: review?(gavin.sharp)
Comment on attachment 330937 [details] [diff] [review]
updated patch which also restores text field values

Looks like this is against an old revision, hopefully merging won't be too much trouble.

Would be better to use Date.now() wherever you're using new Date() to avoid the unnecessary creation of Date object.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   _titleChanged : function(aEvent) {
>-    if (aEvent.target != this.content.browser.contentDocument)
>+    var browser = this.content.browser;
>+    if (!browser || aEvent.target != browser.contentDocument)
>       return;

When would browser be null? Do we allow 0 tabs?

>+      case "cmd_closeTab":
>+        this.content.closeBrowser(this.content.browser);

Seems a bit weird for the public API for closing tabs to be based on browsers.

>+  newTab: function() {
>+    this.content.createBrowser(true, null);

Similarly, seems like deckbrowser itself should be calling createBrowser for new tabs, rather than the consumer code doing it.

> ProgressController.prototype = {

>     // FIXME: until we can get proper canvas repainting hooked up, update the canvas every 300ms
>     var tabbrowser = this._tabbrowser;
>     this._refreshInterval = setInterval(function () {
>       tabbrowser.updateCanvasState();
>     }, 400);

Should probably move this out to browser init now that we can have multiple progress controllers (doesn't matter too much since updateCanvasState() batches updates, and only updates the current browser, but avoiding all these short-interval timers firing unnecessarily would be good).

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+    <command id="cmd_newTab" label="New Tab" oncommand="CommandUpdater.doCommand(this.id);"/>
>+    <command id="cmd_closeTab" label="Close Tab" oncommand="CommandUpdater.doCommand(this.id);"/>

Might as well make these localizable now given that we already have dtds (same for the other hardcoded strings).

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml

>   <binding id="deckbrowser">
>     <content>
>-      <xul:deck flex="1">
>+      <xul:vbox class="tab-list-container" collapsed="true">
>+        <xul:richlistbox anonid="tab-list" class="tab-list"
>+                         onselect="this.parentNode.parentNode.selectTab(event.target.selectedItem)"/>

this.selectedItem?

>+        <xul:button class="newtab-button" label="+" xbl:inherits="oncommand=onnewtab"/>

>+        <xul:button label="Clean" oncommand="this.parentNode.parentNode.destroyEarliestBrowser()"/>

more hardcoded strings

>       <property name="browser" readonly="true">
>         <getter>
>-          return document.getAnonymousElementByAttribute(this, "anonid", "browser");
>+          <![CDATA[
>+            var displayList = this.displayList;
>+            var display = displayList.childNodes[displayList.selectedIndex];

displayList.selectedPanel

>+      <property name="tabListVisible" onget="return !this.tabList.parentNode.collapsed"
>+                                      onset="this.tabList.parentNode.collapsed = !val"/>

return from the setter, too?

>+          if (contentWidth > 0)
>+            this._zoomLevel = canvasWidth / contentWidth;

Do you know in which cases this happens? This method was written to assume it wouldn't be called early enough for there to be no contentWidth.

>+      <method name="updateBrowser">

>+          var event = domWin.document.createEvent("Events");
>+          event.initEvent("pageshow", true, false);
>+          domWin.dispatchEvent(event);

Why is this needed? Doesn't the content document fire this too (same question for "pagehide" in destroyBrowser)?

>+          var tab = this.getTabForDisplay(display);
>+          if (tab) {
>+            tab.updateTab(browser);
>+
>+            var canvas = display.firstChild;
>+            if (canvas.localName == "canvas")
>+              display.removeChild(canvas);

Why are you calling updateTab but then removing the canvas?

>+      <method name="selectTab">

>+          var browser = display ? this.getBrowserForDisplay(display) : null;

Could simplify this caller and one other if getBrowserForDisplay just returned null if passed a null display.
 
>+      <method name="saveBrowserState">
>+      <method name="restoreBrowserState">

We're going to  want to use the Firefox session restore code for this, I think. I think the deckbrowser API in general will probably need to be revisited as well, but we can do that in followups.
Attachment #330937 - Flags: review?(gavin.sharp) → review+
> Why are you calling updateTab but then removing the canvas?

It is removing the browser canvas that gets drawn while a page that has been unloaded in loading. The tab's canvas thumbnail is never removed until the tab is removed.

> >+      <method name="saveBrowserState">
> >+      <method name="restoreBrowserState">
> 
> We're going to  want to use the Firefox session restore code for this, I think.

It is very much hardcoded to use a <tabbrowser> and isn't easily adaptable.

Status: NEW → ASSIGNED
Attached patch address commentsSplinter Review
Attachment #330937 - Attachment is obsolete: true
Attachment #331120 - Flags: review?(gavin.sharp)
Attachment #331120 - Flags: review?(gavin.sharp) → review+
Checked the last patch in. File other bugs for additional work to be done.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
from comment #4, this is verified with beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.