Last Comment Bug 614881 - Add a "closing" property to closing tabs
: Add a "closing" property to closing tabs
Status: RESOLVED FIXED
[good first bug]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: ithinc
:
Mentors:
Depends on:
Blocks: 660446
  Show dependency treegraph
 
Reported: 2010-11-25 17:40 PST by ithinc
Modified: 2011-08-29 06:13 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add "removing" attribute instead of pushing to _removingTabs (14.78 KB, patch)
2011-05-23 00:05 PDT, ithinc
dao+bmo: review-
Details | Diff | Review
Add "removing" attribute instead of pushing to _removingTabs (11.34 KB, patch)
2011-05-23 01:38 PDT, ithinc
no flags Details | Diff | Review
Mark a tab closing when it is being closed (10.73 KB, patch)
2011-05-23 02:06 PDT, ithinc
dao+bmo: review+
Details | Diff | Review

Description ithinc 2010-11-25 17:40:10 PST
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 2011-05-23 00:05:51 PDT
Created attachment 534376 [details] [diff] [review]
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] 2011-05-23 01:06:46 PDT
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 2011-05-23 01:38:57 PDT
Created attachment 534381 [details] [diff] [review]
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] 2011-05-23 01:46:37 PDT
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 2011-05-23 02:06:57 PDT
Created attachment 534384 [details] [diff] [review]
Mark a tab closing when it is being closed
Comment 6 Dão Gottwald [:dao] 2011-05-23 03:36:43 PDT
Comment on attachment 534384 [details] [diff] [review]
Mark a tab closing when it is being closed

Thanks!
Comment 7 ithinc 2011-05-23 04:15:15 PDT
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] 2011-05-23 05:03:26 PDT
No, closed would be misleading -- the tab is in the process of being closed.
Comment 9 Dão Gottwald [:dao] 2011-05-23 05:07:19 PDT
And this can't replace parentNode checks either, as the tab binding wouldn't be there anymore without a parentNode.
Comment 10 ithinc 2011-05-23 22:29:18 PDT
(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] 2011-05-25 00:36:40 PDT
http://hg.mozilla.org/mozilla-central/rev/059d3632e2c4
Comment 12 Vlad [QA] 2011-08-29 06:13:48 PDT
Hi guys. How can this be tested? Is there a testcase ore some STR?
Thanks.

Note You need to log in before you can comment on or make changes to this bug.