Remove Enter/LeaveModalState calls from nsGlobalWindow alert/confirm/prompt

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

unspecified
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
Bug 61098 added calls to Enter/LeaveModalState in the alert/confirm/prompt methods in nsGlobalWindow. They're technically not needed (since they call prompt service, and code within that call ensures the modal state is updated), but are basically harmless because the EMS/LMS code can handle multiple calls.

I happened to ask jst about this, he said these were just added as a convenience for testing.

But now I have need to remove them. Bug 59314 is adding tab-modal dialogs, and the latest patch revision is making nested calls [main() --> page A alert() --> page B alert()] work better by having all the dialog closing code run when the dialog's button is clicked, instead of waiting for the it's event loop to notice the dialog is done. EG, in the example above clicking on page A's alert() should do something without waiting for a response to page B's alert().

Alas, because of these modal state calls in nsGlobalWindow, the content remains frozen until the event loop spins and the prompt service returns.
(Assignee)

Comment 1

7 years ago
Created attachment 491024 [details] [diff] [review]
Patch v.1

The core of this patch is the obvious change to nsGlobalWindow.cpp, and the addition of _toggleModalState() to test_bug61098.html.

I also refactored the mock prompt stuff in the test, while I was there, because bug 59314 will need it to implement nsIPromptFactory. Basically the existing test code gets moved into a MockPrompt object (which is tied to a specific DOM window), and the MockPromptService just creates one of these on demand and calls it... This is basically how the existing code in nsPrompter.js works.
Attachment #491024 - Flags: review?(jst)

Updated

7 years ago
Attachment #491024 - Flags: review?(jst) → review+
blocking2.0: --- → betaN+
(Assignee)

Comment 2

7 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/579f63017921
Status: NEW → RESOLVED
blocking2.0: betaN+ → ---
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.