436064 - Support for multiple documents

Status: VERIFIED FIXED

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

Description

[Christian Sejersen]

Comment 1

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

I assume Christian wanted to assign to Neil Deakin

Comment 2

[Christian Sejersen]

Yes, thanks for fixing Mark.

Comment 3

[Madhava Enros [:madhava]]

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.

Comment 4

[Neil Deakin]

Attachment: initial implementation

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

Comment 5

[Neil Deakin]

Attachment: updated patch which also restores text field values

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 7

[Neil Deakin]

> 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.

Comment 8

[Neil Deakin]

Attachment: address comments

Comment 9

[Neil Deakin]

Checked the last patch in. File other bugs for additional work to be done.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

from comment #4, this is verified with beta3