Bug 610242 will land newWindowWithTabView into head.js. Other tabview tests which run open new windows should also be modified to use newWindowWithTabView.

Comment on attachment 514825 [details] [diff] [review] v1 Passed try http://tbpl.mozilla.org/?tree=MozillaTry&rev=1b9ae82cd1dc

Comment on attachment 514825 [details] [diff] [review] v1 >+ newWindowWithTabView(function(newWin) { >+ registerCleanupFunction(function () { >+ if (!newWin.closed) >+ newWin.close(); >+ }); >+ >+ win = newWin; Just call the argument win instead. Or would there be a problem with that? Maybe we need to set win in a larger scope using this to register a cleanupfunction? But then we could register that within the first newWindowWithTabView callback too, right? >+ function(newWin) { >+ win = newWin; Again, let's just make it win from the beginning. >+ function(newWin) { >+ win = newWin And again... >+ newWindowWithTabView(onTabViewWindowLoaded, function(win) { >+ newWin = win; Wait, why don't you just use the win... >+function newWindowWithTabView(tabViewShownCallback, loadCallback, width, height) { Just call this shownCallback. >+ let winWidth = width ? width: 800; >+ let winHeight = height ? height : 800; Nit: just use winWidth = width || 800. Please flag Ian for review on your next version.

(In reply to comment #3) > Comment on attachment 514825 [details] [diff] [review] > v1 > > > >+ newWindowWithTabView(function(newWin) { > >+ registerCleanupFunction(function () { > >+ if (!newWin.closed) > >+ newWin.close(); > >+ }); > >+ > >+ win = newWin; > > Just call the argument win instead. Or would there be a problem with that Yes, there would be a scope problem if we call the argument "win". "win" would be undefined outside that function. > > Maybe we need to set win in a larger scope using this to register a > cleanupfunction? But then we could register that within the first > newWindowWithTabView callback too, right? > > >+ function(newWin) { > >+ win = newWin; > > Again, let's just make it win from the beginning. The same reason as above. > > >+ function(newWin) { > >+ win = newWin > > And again... The same reason as above. > > >+ newWindowWithTabView(onTabViewWindowLoaded, function(win) { > >+ newWin = win; > > Wait, why don't you just use the win... The same reason as above. > > >+function newWindowWithTabView(tabViewShownCallback, loadCallback, width, height) { > > Just call this shownCallback. Fixed > > >+ let winWidth = width ? width: 800; > >+ let winHeight = height ? height : 800; > > Nit: just use winWidth = width || 800. Fixed

Comment on attachment 515569 [details] [diff] [review] v2 Thanks for cleaning this up! >+ function(newWin) { >+ win = newWin Nit: missing semicolon r+ with that addressed.

(In reply to comment #6) > Created attachment 516783 [details] [diff] [review] > Patch for check-in Passed try http://tbpl.mozilla.org/?tree=MozillaTry&rev=e8fafa71f70d

bugspam