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.
Comment | File | Size | Author |
---|---|---|---|
#8 | javascript_cleanup.patch | 2.68 KB | aspilicious |
#6 | overlaycleanup.patch | 2.68 KB | casey |
#1 | 655740-comment-clarification.patch | 834 bytes | ksenzee |
Comments
Comment #1
ksenzeeCan 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 toself
. Closures are tricky little beasts.Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedAfter 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.
Comment #3
casey CreditAttribution: casey commentedPer #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.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedLooks to me like the code is still there...
Comment #6
casey CreditAttribution: casey commentedYou're right.
I tested the keyhandlers without timeout and they seem to be working fine without it.
Also included ksenzee's improved comment.
Comment #7
aspilicious CreditAttribution: aspilicious commentedImproves things for me (google chrome 4)
Comment #8
aspilicious CreditAttribution: aspilicious commentedHopefully this cleanup gets committed now!
We have to clean the overlay bug list.
Comment #9
aspilicious CreditAttribution: aspilicious commentedStupid me #6 was still applying...
Never mind my patch in #8 then.
#6 is rtbc
Comment #11
aspilicious CreditAttribution: aspilicious commentedRandom testfailures and like I said in #9 go for #6 (or #8 as it is the same patch)
Comment #12
aspilicious CreditAttribution: aspilicious commentedSimple cleanup, bumping this as this is ages rtbc and still applies...
Comment #13
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.