Closed Bug 596781 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

Attachments

(1 file, 7 obsolete files)

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
Assignee: nobody → ian
Blocks: 597043
Assignee: ian → anant
Blocks: 606435
Taking this one.
Assignee: anant → raymond
Status: NEW → ASSIGNED
Depends on: 595521, 599626
Attached patch v1 (obsolete) — Splinter Review
Attachment #486310 - Flags: feedback?(ian)
Attached patch v1 (obsolete) — Splinter Review
Minor update.
Attachment #486310 - Attachment is obsolete: true
Attachment #486312 - Flags: feedback?(ian)
Attachment #486310 - Flags: feedback?(ian)
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+
Attached 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?
Attachment #486534 - Flags: feedback?
Attachment #486534 - Flags: review?(dolske)
Attachment #486534 - Flags: review+
Attachment #486534 - Flags: approval2.0?
Attached 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?
Attachment #490792 - Attachment is patch: true
Attachment #490792 - Attachment mime type: application/octet-stream → text/plain
(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.
Duplicate of this bug: 613133
Attached 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?
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+
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #492268 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/041430229f5e
Status: ASSIGNED → RESOLVED
Closed: 10 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 → ---
Status: REOPENED → ASSIGNED
Moving to b9
Blocks: 598154
No longer blocks: 597043
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});
          }
Attached 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
bugspam (moving b9 to b10)
Blocks: 608028
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
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.
(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 :)
Passed Try with some intermittent oranges but no leaks.
Attachment #498283 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/bb28d0d278d5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 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.