The default bug view has changed. See this FAQ.

Add a "closing" property to closing tabs

RESOLVED FIXED in Firefox 7

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ithinc, Assigned: ithinc)

Tracking

Trunk
Firefox 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Summary: Add "removing" attribute to a removing tab instead of pushing it to _removingTabs array → Add a "removing" attribute to a removing tab instead of pushing it to _removingTabs array

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug]
(Assignee)

Comment 1

6 years ago
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?
Attachment #534376 - Flags: review?(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?
Attachment #534376 - Flags: review?(dao) → review-
(Assignee)

Comment 3

6 years ago
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.
Attachment #534376 - Attachment is obsolete: true
Attachment #534381 - Flags: review?(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?
(Assignee)

Comment 5

6 years ago
Created attachment 534384 [details] [diff] [review]
Mark a tab closing when it is being closed
Attachment #534381 - Attachment is obsolete: true
Attachment #534384 - Flags: review?(dao)
Attachment #534381 - Flags: review?(dao)
Comment on attachment 534384 [details] [diff] [review]
Mark a tab closing when it is being closed

Thanks!
Attachment #534384 - Flags: review?(dao) → review+
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 10

6 years ago
(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?

Updated

6 years ago
Assignee: nobody → ithinc
Keywords: checkin-needed
Summary: Add a "removing" attribute to a removing tab instead of pushing it to _removingTabs array → Add a "closing" property to closing tabs
http://hg.mozilla.org/mozilla-central/rev/059d3632e2c4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7

Updated

6 years ago
Blocks: 660446

Comment 12

6 years ago
Hi guys. How can this be tested? Is there a testcase ore some STR?
Thanks.
You need to log in before you can comment on or make changes to this bug.