614881 - Add a "closing" property to closing tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[ithinc]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 (.NET CLR 3.5.30729)

I have read that in tabbrowwser.xml _removingTabs.indexOf method is frequently called. It would be better to have a "removing" attribute on removing tabs.

Reproducible: Always

Comment 1

[ithinc]

Attachment: Add "removing" attribute instead of pushing to _removingTabs

There are another two references of _removingTabs in tabview files, should I modify them directly?

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 534376 [details] [diff] [review]
Add "removing" attribute instead of pushing to _removingTabs

Please use "closing" instead of "removing" as the property name.

>-      <field name="_removingTabs">
>-        []
>-      </field>
>+      <property name="_removingTabs" readonly="true">
>+        <getter>
>+          return Array.filter(this.tabs, function(tab) tab.removing);
>+        </getter>
>+      </property>

I wonder if it would be more efficient to let _beginRemoveTab and _endRemoveTab still update this array rather than building it on demand.

>-              while (this._removingTabs.length)
>-                this._endRemoveTab(this._removingTabs[0]);
>+              this._removingTabs.forEach(this._endRemoveTab, this);

You're changing the behavior here, since _removingTabs could contain new tabs while you're in the loop.

>-        tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser.removeTab,
>+        tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser._endRemoveTab,
>                                               tabs.tabbrowser);

Avoid this change.

>+      <property name="removing" readonly="true">
>+        <getter>
>+          return this.hasAttribute("removing");
>+        </getter>
>+      </property>

What do we need the attribute behind the scenes for?

Comment 3

[ithinc]

Attachment: Add "removing" attribute instead of pushing to _removingTabs

(In reply to comment #2)
> Please use "closing" instead of "removing" as the property name.

Thanks for your comments. Then need it be "_closing" or "mClosing"? I'm not clear about the conventions.

> I wonder if it would be more efficient to let _beginRemoveTab and
> _endRemoveTab still update this array rather than building it on demand.
> 
> >-              while (this._removingTabs.length)
> >-                this._endRemoveTab(this._removingTabs[0]);
> >+              this._removingTabs.forEach(this._endRemoveTab, this);
> 
> You're changing the behavior here, since _removingTabs could contain new
> tabs while you're in the loop.

Addressed and fixed.

> >-        tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser.removeTab,
> >+        tabs.tabbrowser._removingTabs.forEach(tabs.tabbrowser._endRemoveTab,
> >                                               tabs.tabbrowser);
> 
> Avoid this change.

OK. I took it as a mistake.

> >+      <property name="removing" readonly="true">
> >+        <getter>
> >+          return this.hasAttribute("removing");
> >+        </getter>
> >+      </property>
> 
> What do we need the attribute behind the scenes for?

Not really need it. Removed.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 534381 [details] [diff] [review]
Add "removing" attribute instead of pushing to _removingTabs

>@@ -1597,16 +1598,17 @@
> 
>             this._lastRelatedTab = null;
> 
>             // update the UI early for responsiveness
>             aTab.collapsed = true;
>             this.tabContainer._fillTrailingGap();
>             this._blurTab(aTab);
> 
>+            aTab.closing = false;
>             this._removingTabs.splice(this._removingTabs.indexOf(aTab), 1);
> 
>             if (aCloseWindow) {
>               this._windowIsClosing = true;
>               while (this._removingTabs.length)
>                 this._endRemoveTab(this._removingTabs[0]);
>             } else if (!this._windowIsClosing) {
>               if (aNewTab)

Isn't this unnecessary?

Comment 5

[ithinc]

Attachment: Mark a tab closing when it is being closed

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 534384 [details] [diff] [review]
Mark a tab closing when it is being closed

Thanks!

Comment 7

[ithinc]

I feel "closed" might be more suitable than "closing". Do you agree?
This property can also be used where tab.parentNode is checked.

Comment 8

[Dão Gottwald [:dao]]

No, closed would be misleading -- the tab is in the process of being closed.

Comment 9

[Dão Gottwald [:dao]]

And this can't replace parentNode checks either, as the tab binding wouldn't be there anymore without a parentNode.

Comment 10

[ithinc]

(In reply to comment #9)
> And this can't replace parentNode checks either, as the tab binding wouldn't
> be there anymore without a parentNode.

Thanks! What's the next step for me? Ask for approval or just add "checkin-needed" keyword?

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/059d3632e2c4

Comment 12

[Vlad [QA]]

Hi guys. How can this be tested? Is there a testcase ore some STR?
Thanks.