841312 - Remove Termination Functions

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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. :-)

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=636a56dfd3de

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Seems sane to me at first glance...

Comment 5

[Bobby Holley (:bholley)]

I want to clean up the modal dialog stuff in order to make it easier to debug the test failure here.

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

Booya, this is green! Uploading patches and flagging bz for review.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove window close termination function, and fall through to PostCloseEvent(). v1

This is a more jaded version of my previous attempt.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Replace the scripted closing unblocker termination function with an runnable. v2

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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove the termination function API. v1

\o/

Comment 11

[Boris Zbarsky [: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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 751857 [details] [diff] [review]
Part 3 - Remove the termination function API. v1

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7553d739ea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3eb1cf0ac2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/75abbd7e7e24

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c7553d739ea
https://hg.mozilla.org/mozilla-central/rev/7d3eb1cf0ac2
https://hg.mozilla.org/mozilla-central/rev/75abbd7e7e24

Comment 17

[Bobby Holley (:bholley)]

Attachment: Remove IsCallerChrome path for tearing down windows synchronously. v1

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 794756 [details] [diff] [review]
Remove IsCallerChrome path for tearing down windows synchronously. v1

Er, wrong bug.