Last Comment Bug 651114 - Clean up browser_tabview_bug612470.js
: Clean up browser_tabview_bug612470.js
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 6
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-19 06:35 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (3.84 KB, patch)
2011-04-19 06:37 PDT, Tim Taubert [:ttaubert]
ian: review+
raymond: feedback+
Details | Diff | Splinter Review
patch for checkin (3.97 KB, patch)
2011-04-22 11:06 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-04-19 06:35:01 PDT
The test times out in single mode and we could use some of the new head.js functions.
Comment 1 Tim Taubert [:ttaubert] 2011-04-19 06:37:57 PDT
Created attachment 526971 [details] [diff] [review]
patch v1
Comment 3 Ian Gilman [:iangilman] 2011-04-22 10:03:46 PDT
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] 2011-04-22 11:06:34 PDT
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.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-04-23 02:40:02 PDT
In my queue.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2011-04-24 05:15:12 PDT
http://hg.mozilla.org/mozilla-central/rev/7b7a77e74c78

Note You need to log in before you can comment on or make changes to this bug.