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

VERIFIED FIXED

Status

P2
normal
VERIFIED FIXED
8 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

8 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

8 years ago
Assignee: nobody → ian
(Reporter)

Updated

8 years ago
Blocks: 597043
(Reporter)

Updated

8 years ago
Assignee: ian → anant

Updated

8 years ago
Blocks: 606435
(Assignee)

Comment 3

8 years ago
Taking this one.
Assignee: anant → raymond
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Depends on: 595521, 599626
(Assignee)

Comment 4

8 years ago
Created attachment 486310 [details] [diff] [review]
v1
Attachment #486310 - Flags: feedback?(ian)
(Assignee)

Comment 5

8 years ago
Created attachment 486312 [details] [diff] [review]
v1

Minor update.
Attachment #486310 - Attachment is obsolete: true
Attachment #486312 - Flags: feedback?(ian)
Attachment #486310 - Flags: feedback?(ian)
(Reporter)

Comment 6

8 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

8 years ago
Created attachment 486534 [details] [diff] [review]
v1

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

8 years ago
Attachment #486534 - Flags: feedback?
(Reporter)

Updated

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

Comment 8

8 years ago
Created attachment 490792 [details] [diff] [review]
v1

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

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

Comment 9

8 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

8 years ago
Duplicate of this bug: 613133
(Assignee)

Comment 11

8 years ago
Created attachment 492268 [details] [diff] [review]
v1

Passed try. r=Ian
Attachment #490792 - Attachment is obsolete: true
Attachment #492268 - Flags: approval2.0?
Attachment #490792 - Flags: approval2.0?
(Assignee)

Comment 12

8 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

8 years ago
Created attachment 492701 [details] [diff] [review]
Patch for check-in
Attachment #492268 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/041430229f5e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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

8 years ago
Status: REOPENED → ASSIGNED

Comment 17

8 years ago
Moving to b9
Blocks: 598154

Updated

8 years ago
No longer blocks: 597043
(Assignee)

Comment 18

8 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

8 years ago
Created attachment 498283 [details] [diff] [review]
v2

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

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

Comment 21

8 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

8 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

8 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

8 years ago
Created attachment 503495 [details] [diff] [review]
Patch for check-in

Passed Try with some intermittent oranges but no leaks.
Attachment #498283 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 28

8 years ago
http://hg.mozilla.org/mozilla-central/rev/bb28d0d278d5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 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.