This is copied directly from a review by @mfer and Nathan Smith at #610234: Overlay implementation, but copied to its own issue now that the original patch was committed:

+++ modules/overlay/overlay-child.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,153 @@
+      setTimeout(function () {
+        // We need to store the parent variable locally because it will
+        // disappear as soon as we close the iframe.
+        var p = parent;
+        p.Drupal.overlay.close(settings.args, settings.statusMessages);
+        if (typeof settings.redirect == 'string') {
+          p.Drupal.overlay.redirect(settings.redirect);
+        }
+      }, 1);

Can you explain why the setTimeout of 1ms is here for? This may have been written earlier and there may be a good reason. Just sniffing at what's going on.

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+      setTimeout(function () { self.close(); }, 1);

Why is this wrapped in anonymous function? Won't setTimeout(self.close();, 1); work?

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+        setTimeout(function () { $target.focus(); }, 10);

Again, is there a reason this is wrapped in an anonymous function?

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+            setTimeout(function () { $closeButton.focus(); }, 10);

Anonymous function again? Is there a reason?

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+            setTimeout(function () { $closeButton.focus(); }, 10);

Oh, again a few lines later.

+++ modules/overlay/overlay-parent.js	29 Nov 2009 02:45:34 -0000
@@ -0,0 +1,885 @@
+          setTimeout(function () { self.close(); }, 10);

Same thing.

Gotta give a shout out to Nathan Smith for taking a look at this part of it with me.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee’s picture

Status: Active » Needs review
FileSize
834 bytes

Can you explain why the setTimeout of 1ms is here for?

I clarified the inline comment that was trying to explain this. :)

Why is this wrapped in anonymous function? Won't setTimeout(self.close();, 1); work?

You would think it would, but if you try it you'll get a JavaScript error saying that self is not defined. If you pass it a function, however, the browser keeps around a reference to self. Closures are tricky little beasts.

David_Rothstein’s picture

After scratching my head a bit, I guess I understood why setTimeout() necessarily starts a different thread, but the real thing I'm not quite understanding here is why does the overlay closing have to happen immediately? What bad thing happens if we try to do it in the current thread?

For the second issue, maybe it happens too often in the code to bother doing this, but I wonder if a code comment would help clear this up as well.

casey’s picture

Status: Needs review » Fixed

Per #174 in #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems a lot has changed and I don't think this issue is relevant any more. Free to reopen when you disagree.

David_Rothstein’s picture

Status: Fixed » Needs review

Looks to me like the code is still there...

casey’s picture

FileSize
2.68 KB

You're right.

I tested the keyhandlers without timeout and they seem to be working fine without it.

Also included ksenzee's improved comment.

aspilicious’s picture

Improves things for me (google chrome 4)

aspilicious’s picture

FileSize
2.68 KB

Hopefully this cleanup gets committed now!
We have to clean the overlay bug list.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Stupid me #6 was still applying...
Never mind my patch in #8 then.

#6 is rtbc

Status: Reviewed & tested by the community » Needs work

The last submitted patch, javascript_cleanup.patch, failed testing.

aspilicious’s picture

Status: Needs work » Reviewed & tested by the community

Random testfailures and like I said in #9 go for #6 (or #8 as it is the same patch)

aspilicious’s picture

Simple cleanup, bumping this as this is ages rtbc and still applies...

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.