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

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Ian Gilman [:iangilman]]

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.)

Comment 3

[Raymond Lee [:raymondlee]]

Taking this one.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

Minor update.

Comment 6

[Ian Gilman [:iangilman]]

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.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v1

f=ian. Updated the patch per Ian's comments

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v1

Sent it to try 2839413df1af and waiting for the result.

Comment 9

[Raymond Lee [:raymondlee]]

(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.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v1

Passed try. r=Ian

Comment 12

[Raymond Lee [:raymondlee]]

Could someone give approval2.0 to this please?

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 492268 [details] [diff] [review]
v1

a=beltzner

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 15

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/041430229f5e

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 17

[Kevin Hanes]

Moving to b9

Comment 18

[Raymond Lee [:raymondlee]]

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});
          }

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: 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
+        }

Comment 20

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 21

[Kevin Hanes]

bugspam (removing b9)

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 23

[Raymond Lee [:raymondlee]]

It still has leak on OSX debug.  Re-sending it to try to confirm that.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 25

[Raymond Lee [:raymondlee]]

(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.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Passed Try with some intermittent oranges but no leaks.

Comment 28

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/bb28d0d278d5

Comment 29

[Tracy Walker [:tracy]]

verified with nightly build of minefield.