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

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

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?

(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 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 on attachment 534384 [details] [diff] [review] Mark a tab closing when it is being closed Thanks!

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

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

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

(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?

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