615271 - Ghost tab in the undo panel when opening a local page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP v0.1

Steps to reproduce:
 * launch fennec
 * open a new tab
 * load about:home
 * slide to the right to see the left sidebar

Actual result:
 * There is 2 tabs and a black tab in the undo panel

Expected result:
 * there is 2 tabs

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + tests

The patch add a new parameter to the Browser.closeTab function in order to prevent a tab to appears in the undo box in the UI. The "cant undo" boolean value is also sent into a UIEvent used by tab.

Comment 2

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

Comment on attachment 493953 [details] [diff] [review]
Patch + tests

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -529,17 +529,17 @@ var Browser = {
>     // new "remote" tabs if opening web pages from a "local" about: page.
>     let currentURI = browser.currentURI.spec;
>     let useLocal = Util.isLocalScheme(aURI);
>     let hasLocal = Util.isLocalScheme(currentURI);
> 
>     if (hasLocal != useLocal) {
>       let oldTab = this.selectedTab;
>       if (currentURI == "about:blank" && !browser.canGoBack && !browser.canGoForward) {
>-        this.closeTab(oldTab);
>+        this.closeTab(oldTab, true);

I'd rather add a property to the "tabs" binding than pass a parameter. The parameter just needs to be passed to the "tabs" binding anyway, so let's do it more explicitly:

        Elements.tabs.allowUndo = false;
        try {
          this.closeTab(oldTab);
        } catch (e) { Cu.reportError(e); }
        Elements.tabs.allowUndo = true;

>-  closeTab: function(tab) {
>-    if (tab instanceof XULElement)
>-      tab = this.getTabFromChrome(tab);
>+  closeTab: function(aTab, aCantUndo) {

No need for aCantUndo

>+    let tab = aTab;
>+    if (aTab instanceof XULElement)
>+      tab = this.getTabFromChrome(aTab);
> 
>     if (!tab)
>       return;

Can you change this check to:

     if (!tab || this._tabs.length < 2)
       return;

That would help fix bug 615404

>-    let event = document.createEvent("Events");
>-    event.initEvent("TabClose", true, false);
>+    let event = document.createEvent("UIEvents");
>+    event.initUIEvent("TabClose", true, false, window, !!aCantUndo);

No need for this change

>   destroy: function destroy() {
>-    document.getElementById("tabs").removeTab(this._chromeTab);

Leave this. I don't want to depend on the event. You could add "tabs" to the Elements object ( Elements.tabs.removeTab(...) )

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

>+    <handlers>
>+      <handler event="TabClose">
>+        <![CDATA[
>+          this._container.removeTab(this, event.detail);
>+        ]]>
>+      </handler>
>+    </handlers>

Don't do this. But add the "allowUndo" property instead

>       <method name="removeTab">
>         <parameter name="aTab"/>
>+        <parameter name="aCantUndo"/>

No need for aCantUndo. Check the this.allowUndo property instead

Update any tests that were depending on the extra param in closeTab or the TabClose event

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I've hesitate to add a property to the tab object itself first, I guess it make things much simpler.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 + tests

The patch addressed comments but do not use Elements.tabs since it is already used for the tab sidebar.

Comment 5

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/d1a74d739c0c

Comment 6

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101210 Namoroka/4.0b8pre Fennec/4.0b3pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101210 Namoroka/4.0b8pre Fennec/4.0b3pre