Clean up browser_tabview_bug612470.js

RESOLVED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 6

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
The test times out in single mode and we could use some of the new head.js functions.
(Assignee)

Comment 1

6 years ago
Created attachment 526971 [details] [diff] [review]
patch v1
Attachment #526971 - Flags: feedback?(raymond)
Attachment #526971 - Flags: feedback?(raymond) → feedback+
(Assignee)

Updated

6 years ago
Attachment #526971 - Flags: review?(ian)
(Assignee)

Comment 2

6 years ago
Comment on attachment 526971 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=08df77ceca79
Comment on attachment 526971 [details] [diff] [review]
patch v1

> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> // Tests that groups behave properly when closing all tabs but app tabs.

I really like that there's a comment at the top here describing what the test is for... might be a good habit to get into.

>+  newWindowWithTabView(function (tvwin) {
>+    registerCleanupFunction(function () tvwin.close());
> 
>-  addEventListener("tabviewshown", createGroup, false);
>-  TabView.toggle();
>+    win = tvwin;
>+    cw = win.TabView.getContentWindow();
>+    executeSoon(function () onShow());
>+  }, onLoad);

I found it a little confusing that of the two functions passed into newWindowWithTabView, one was defined right here and the other was defined above and passed only by name. I think passing them both by name would be clearer (otherwise it's easier to forget which function is for what), though I suppose a middle ground would be to name the functions inline: 

newWindowWithTabView(function onShow (tvwin) {
  ...
}, function onLoad () {
  ...
});

Anyway, food for thought.
Attachment #526971 - Flags: review?(ian) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 527811 [details] [diff] [review]
patch for checkin

(In reply to comment #3)
> I really like that there's a comment at the top here describing what the test
> is for... might be a good habit to get into.

True. Noted :)

> I found it a little confusing that of the two functions passed into
> newWindowWithTabView, one was defined right here and the other was defined
> above and passed only by name. I think passing them both by name would be
> clearer (otherwise it's easier to forget which function is for what)

Fixed.
Attachment #526971 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
In my queue.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/7b7a77e74c78
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.