Don't allow the last group to close if there are app tabs

VERIFIED FIXED

Status

defect
P2
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: iangilman, Assigned: raymondlee)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

9 years ago
Follow up to bug 595943. 

* We do not allow you to close the last group if there are app-tabs (and we
remove the close icon for that last group). If there is a group and an orphaned
tab we also do not let you close the last group. (Alternatively, we can let you
close the group and instantly put the orphaned tab into a group.)
Duplicate of this bug: 597299
Duplicate of this bug: 597328
Reporter

Updated

9 years ago
Assignee: nobody → ian
Reporter

Updated

9 years ago
Blocks: 597043
Reporter

Updated

9 years ago
Assignee: ian → anant

Updated

9 years ago
Blocks: 606435
Assignee

Comment 3

9 years ago
Taking this one.
Assignee: anant → raymond
Status: NEW → ASSIGNED
Assignee

Updated

9 years ago
Depends on: 595521, 599626
Assignee

Comment 4

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #486310 - Flags: feedback?(ian)
Assignee

Comment 5

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Minor update.
Attachment #486310 - Attachment is obsolete: true
Attachment #486312 - Flags: feedback?(ian)
Attachment #486310 - Flags: feedback?(ian)
Reporter

Comment 6

9 years ago
Comment on attachment 486312 [details] [diff] [review]
v1

Looks good. A couple comments: 

* I think you should check 

>   close: function GroupItem_close() {
>+    if (GroupItems.getUnclosableGroupItemWithAppTab())
>+      return;

I think you should put this check at the top of closeAll as well.

>+  // Function: getUnclosableGroupItemWithAppTab
>+  // Gets the only group item which contains app tab.  If there are more 
>+  // than one group item contains app tab, it returns null.

This comment feels a bit confusing. How about "If there's only one (non-hidden) group, and there are app tabs present, returns that group."

For that matter, perhaps it should just be called getUnclosableGroupItem.
Attachment #486312 - Flags: feedback?(ian) → feedback+
Assignee

Comment 7

9 years ago
Posted patch v1 (obsolete) — Splinter Review
f=ian. Updated the patch per Ian's comments
Attachment #486312 - Attachment is obsolete: true
Attachment #486534 - Flags: review?(dolske)
Attachment #486534 - Flags: feedback?
Assignee

Updated

9 years ago
Attachment #486534 - Flags: feedback?
Reporter

Updated

9 years ago
Attachment #486534 - Flags: review?(dolske)
Attachment #486534 - Flags: review+
Attachment #486534 - Flags: approval2.0?
Assignee

Comment 8

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Sent it to try 2839413df1af and waiting for the result.
Attachment #486534 - Attachment is obsolete: true
Attachment #490792 - Flags: approval2.0?
Attachment #486534 - Flags: approval2.0?
Assignee

Updated

9 years ago
Attachment #490792 - Attachment is patch: true
Attachment #490792 - Attachment mime type: application/octet-stream → text/plain
Assignee

Comment 9

9 years ago
(In reply to comment #8)
> Created attachment 490792 [details] [diff] [review]
> v1
> 
> Sent it to try 2839413df1af and waiting for the result.

There are leaks on all debug builds when running tests.

Updated

9 years ago
Duplicate of this bug: 613133
Assignee

Comment 11

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Passed try. r=Ian
Attachment #490792 - Attachment is obsolete: true
Attachment #492268 - Flags: approval2.0?
Attachment #490792 - Flags: approval2.0?
Assignee

Comment 12

9 years ago
Could someone give approval2.0 to this please?
Comment on attachment 492268 [details] [diff] [review]
v1

a=beltzner
Attachment #492268 - Flags: approval2.0? → approval2.0+
Assignee

Comment 14

9 years ago
Posted patch Patch for check-in (obsolete) — Splinter Review
Attachment #492268 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Keywords: checkin-needed
Reporter

Comment 15

9 years ago
http://hg.mozilla.org/mozilla-central/rev/041430229f5e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
This bug was part of a mass backout to fix the permanent leak on OS X 64 that this push caused.

http://hg.mozilla.org/mozilla-central/rev/b014423f755b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

9 years ago
Status: REOPENED → ASSIGNED

Comment 17

9 years ago
Moving to b9
Blocks: 598154

Updated

9 years ago
No longer blocks: 597043
Assignee

Comment 18

9 years ago
After some investigations, I've narrowed down the code which is causing the leak


 +      if (!this._children.length && !this.locked.close && !this.getTitle() && 
 +          !options.dontClose && !GroupItems.getUnclosableGroupItemId()) {
             this.close();
          } else if (!options.dontArrange) {
             this.arrange({animate: !options.immediately});
          }
Assignee

Comment 19

9 years ago
Posted patch v2 (obsolete) — Splinter Review
I've spent a long time on this patch but still couldn't find the cause of the leak.  I've improved the patch and also added a comment to the patch about the leak. See below.


-      if (!this._children.length && !this.locked.close && !this.getTitle() && !options.dontClose) {
-        this.close();
+      if (this._children.length == 0 && !this.locked.close && !this.getTitle() && 
+          !options.dontClose) {
+        if (!GroupItems.getUnclosableGroupItemId()) {
+          this.close();
+        } else {
+          // this.close();  this line is causing the leak but the leak doesn't happen after re-enabling it
+        }
Attachment #492701 - Attachment is obsolete: true

Comment 20

9 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 21

9 years ago
bugspam (removing b9)
No longer blocks: 598154
Bug 624722 which fixed a couple other leaks of ours seems to have fixed this leak as well, at least on my local machine! Sending to try now.
Depends on: 624722
Assignee

Comment 23

9 years ago
It still has leak on OSX debug.  Re-sending it to try to confirm that.
I just found out that the behavior of bug 613541 which I'm fixing right now badly conflicts with this one. In fact, bug 613541 is obsolete when this one lands because the last group just can't be closed.
Assignee

Comment 25

9 years ago
(In reply to comment #24)
> I just found out that the behavior of bug 613541 which I'm fixing right now
> badly conflicts with this one. In fact, bug 613541 is obsolete when this one
> lands because the last group just can't be closed.

This patch doesn't allow the last group to close if one or more app tabs exists.  Therefore, your patch for bug 613541 still require because when no app tabs exists, closing the last group should open a new tab.
Ahhh sorry I was too quick. You're right - I can just ignore the number of app tabs because you take care of them. Thanks for the clarification :)
Assignee

Comment 27

9 years ago
Passed Try with some intermittent oranges but no leaks.
Attachment #498283 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Keywords: checkin-needed
Reporter

Comment 28

9 years ago
http://hg.mozilla.org/mozilla-central/rev/bb28d0d278d5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.