651114 - Clean up browser_tabview_bug612470.js

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

The test times out in single mode and we could use some of the new head.js functions.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 526971 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=08df77ceca79

Comment 3

[Ian Gilman [:iangilman]]

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In my queue.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/7b7a77e74c78