Closed Bug 841312 Opened 7 years ago Closed 6 years ago

Remove Termination Functions

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files, 1 obsolete file)

So, I _think_ I can get rid of these, but there's a lot of black magic I don't necessarily grok in full. Hopefully bz can help me straighten this out, especially since he wants one of these patches for bug 835643. :-)
Hm, that may have been too ambitious. I scaled back the behavioral changes to the minimum needed to remove the API, and pushed again:

https://tbpl.mozilla.org/?tree=Try&rev=1562cceca8f9
Nice, looks like there's only one failure. I'll look at it soon, though it might be a bit since I'm traveling tomorrow.

Boris, can you have a quick look at what that test is doing and tell me if it's sane?
Seems sane to me at first glance...
I want to clean up the modal dialog stuff in order to make it easier to debug the test failure here.
Depends on: 860941
Now that I've done the modal dialog rewrite, as well as the cx lifetime work in bug 860438, let's see if we still have any failures here:

https://tbpl.mozilla.org/?tree=Try&rev=96ebaf7e7e67
Booya, this is green! Uploading patches and flagging bz for review.
This is a more jaded version of my previous attempt.
Attachment #751854 - Flags: review?(bzbarsky)
The main idea behind this thing seems to be that we don't want script to quickly
close the window before the user has time to read the notification. Given the
fuzziness of the constraint here, I think we can (and maybe even should) unblock
a little bit later in the event loop, rather than immediately after the script
terminates.

Note that, due to modal dialogs and their event loop spinning shenanigans, we
want to do this only when the stack frame is popped.
Attachment #751855 - Flags: review?(bzbarsky)
\o/
Attachment #751857 - Flags: review?(bzbarsky)
Comment on attachment 751854 [details] [diff] [review]
Part 1 - Remove window close termination function, and fall through to PostCloseEvent(). v1

Hrm.

r=me, I guess, but this won't help my situation in bug 862627 any: the JSContext there stops being the current one but the code is still depending on the window not going away immediately...  That happens in hundreds of tests, as far as I could tell.

What actually depends on the immediate closure behavior, and how hard would it be to fix it?
Attachment #751854 - Flags: review?(bzbarsky) → review+
Comment on attachment 751855 [details] [diff] [review]
Part 2 - Replace the scripted closing unblocker termination function with an runnable. v2

I don't follow this patch.  In the old setup if we do window.open() and then put up an alert, the window close won't happen until after the alert comes down, right?

But in the new world, seems like it would, no?
Comment on attachment 751857 [details] [diff] [review]
Part 3 - Remove the termination function API. v1

r=me
Attachment #751857 - Flags: review?(bzbarsky) → review+
Comment on attachment 751855 [details] [diff] [review]
Part 2 - Replace the scripted closing unblocker termination function with an runnable. v2

r=me per irc discussion.  The key is that close() calls while mBlockScriptedClosingFlag are just ignored, not queued.
Attachment #751855 - Flags: review?(bzbarsky) → review+
Depends on: 877390
Comment on attachment 794756 [details] [diff] [review]
Remove IsCallerChrome path for tearing down windows synchronously. v1

Er, wrong bug.
Attachment #794756 - Attachment is obsolete: true
Depends on: 914521
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.