584627 - Tests for group

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Raymond Lee [:raymondlee]]

* Open / Close empty group by using the "x" button.
* Open a group, add item by using the "+" button, remove item using the "x" button and the group should disappear automatically.
* Open a group, add few items using the "+" buttons, and remove the group using the "x" button. All browser tabs and tab items in this group should be removed.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

This covers the cases in comment 0.

@Mardak: The "close" subscriber event is sent before the tab item or group is destroyed so I use setTimeout with 0ms delay before comparing the num change of tab items in a group and num change in groups.  is it OK since I also see some other tests using setTimeout with 0ms delay?

Comment 2

[Ed Lee :Mardak]

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

That should be fine. setTimeout(0) is better than setTimeout(arbitrary wait). It's a fragile thing to depend on especially if other code depends on the code doing setTimeout as the depdendent code might need to do setTimeout(setTimeout(0), 0), etc.

Probably not as necessary here, but other places have an onbeforeclose-like event. For things that want to inspect the group going away, they can watch onbeforeclose so that the object exists to access properties, etc. A final close event happens when the group actually goes away and shouldn't be accessed. It's a little blurry for js objects vs c++ as object references will keep them alive..

Comment 3

[Ian Gilman [:iangilman]]

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

Looks good, let's do it. 

Comments:

* Needs a license block.
* Perhaps the close events should be modified to be sent after the item has been removed from the master list and from its parent, but before it's fully destroyed? That seems like the right time anyway, and it would get rid of the need for the setTimeout.

Comment 4

[Raymond Lee [:raymondlee]]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/383716aa20b6

* Added license block
* Moved the close events to the appropriate places for groupItem and tabItem

Comment 5

[Ian Gilman [:iangilman]]

Raymond, some comments (sorry, I think I should have caught some of these earlier):

* We shouldn't have a "tabRemoved" event sent from TabItem.close; that routine is only one of the ways that tabs get closed (it doesn't account for tabs getting closed from the tab bar, for instance, even though those tabs are also removed from the Tab Candy UI). You should just be using the "close" event sent from TabItems.unlink; this accounts for all possible ways that tabs could be closed. Can you take care of this?

* I don't think we need both "beforeclose" and "close" events. I've done a little refactoring, and brought it back down to just one event, in this change: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/c648f6347add

... and the group tests seem to still work. Let me know if you see any problems with it. 

* I've improved the TabView start up procedure in this change:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/ff6a08989141

... so you might want to take advantage of that in your tests. Take a look at the new _initFrame with callback, as well as the explicit show method.

Comment 6

[Ian Gilman [:iangilman]]

Note that addressing the above comments is lower priority than writing unit tests for areas outside of the Tab Candy frame.

Comment 7

[Ian Gilman [:iangilman]]

This test has been temporarily disabled (in browser/base/content/test/tabview/Makefile.in), as it's failing. Please re-enable once it's fixed.

Comment 8

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Re-enable the test with some changes

Comment 10

[Ian Gilman [:iangilman]]

Comment on attachment 467343 [details] [diff] [review]
Re-enable the test with some changes

Looks good to me.

Comment 11

[Ian Gilman [:iangilman]]

Looks like this patch needs to land after bug 582677 (due to the changes in Makefile.in). Is there a good way to indicate that?

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 467343 [details] [diff] [review]
Re-enable the test with some changes


>+++ b/browser/base/content/test/tabview/Makefile.in	Thu Aug 19 16:19:35 2010 +0800
>@@ -48,8 +48,8 @@
> _BROWSER_FILES = \
>                  browser_tabview_launch.js \
>                  browser_tabview_dragdrop.js \
>-$(NULL)
>-#                 browser_tabview_group.js \
>+                 browser_tabview_group.js \
>+    $(NULL)

Fix alignment of $(NULL) so it matches other items.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: re-enable the test with some changes [r=dolske a=dolske]

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1f9925cb9bf8