Closed Bug 584627 Opened 9 years ago Closed 9 years ago

Tests for group

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 4.0b5

People

(Reporter: raymondlee, Assigned: raymondlee)

References

Details

Attachments

(1 file, 2 obsolete files)

* 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.
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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?
Attachment #463110 - Flags: review?(ian)
Attachment #463110 - Flags: feedback?(edilee)
Attachment #463110 - Attachment is patch: true
Attachment #463110 - Attachment mime type: application/octet-stream → text/plain
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..
Attachment #463110 - Flags: feedback?(edilee) → feedback+
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.
Attachment #463110 - Flags: review?(ian) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note that addressing the above comments is lower priority than writing unit tests for areas outside of the Tab Candy frame.
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.
Depends on: 574217
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Version: unspecified → Trunk
Attachment #467343 - Flags: review?(dolske)
Attachment #467343 - Flags: feedback?(ian)
Comment on attachment 467343 [details] [diff] [review]
Re-enable the test with some changes

Looks good to me.
Attachment #467343 - Flags: feedback?(ian) → feedback+
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?
QA Contact: tabcandy → tabcandy
Attachment #463110 - Attachment is obsolete: true
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.
Attachment #467343 - Flags: review?(dolske)
Attachment #467343 - Flags: review+
Attachment #467343 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/1f9925cb9bf8
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.