Closed Bug 623673 Opened 9 years ago Closed 9 years ago

Consolidate code for "new window" tests into newWindowWithTabView

Categories

(Firefox Graveyard :: Panorama, defect, P4)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 5

People

(Reporter: mitcho, Assigned: raymondlee)

References

Details

(Whiteboard: [qa-][cleanup][test])

Attachments

(1 file, 2 obsolete files)

Bug 610242 will land newWindowWithTabView into head.js. Other tabview tests which run open new windows should also be modified to use newWindowWithTabView.
Blocks: 585689
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #514825 - Flags: feedback?(mitcho)
Priority: -- → P4
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.
Attachment #514825 - Flags: feedback?(mitcho) → feedback+
Attached patch v2 (obsolete) — Splinter Review
(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
Attachment #514825 - Attachment is obsolete: true
Attachment #515569 - Flags: review?(ian)
Comment on attachment 515569 [details] [diff] [review]
v2

Thanks for cleaning this up!

>+    function(newWin) {
>+      win = newWin

Nit: missing semicolon

r+ with that addressed.
Attachment #515569 - Flags: review?(ian) → review+
Attachment #515569 - Attachment is obsolete: true
No longer blocks: 585689
Blocks: 603789
bugspam
http://hg.mozilla.org/mozilla-central/rev/b58e01e89961
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Target Milestone: Firefox5 → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.