Closed
Bug 841312
Opened 12 years ago
Closed 12 years ago
Remove Termination Functions
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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. :-)
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
Seems sane to me at first glance...
Assignee | ||
Comment 5•12 years ago
|
||
I want to clean up the modal dialog stuff in order to make it easier to debug the test failure here.
Depends on: 860941
Assignee | ||
Comment 6•12 years ago
|
||
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
Assignee | ||
Comment 7•12 years ago
|
||
Booya, this is green! Uploading patches and flagging bz for review.
Assignee | ||
Comment 8•12 years ago
|
||
This is a more jaded version of my previous attempt.
Attachment #751854 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 751857 [details] [diff] [review]
Part 3 - Remove the termination function API. v1
r=me
Attachment #751857 -
Flags: review?(bzbarsky) → review+
Comment 14•12 years ago
|
||
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+
Assignee | ||
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c7553d739ea
https://hg.mozilla.org/mozilla-central/rev/7d3eb1cf0ac2
https://hg.mozilla.org/mozilla-central/rev/75abbd7e7e24
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Comment 18•11 years ago
|
||
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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•